Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

#180 Use a service for Requirement Title and Description expression #194

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

GlennPlou
Copy link

@GlennPlou GlennPlou commented Jul 18, 2024

After #193, we've chosen to limit the impact our changes can have. An htmltoText() service has been added to format a String ( in particular the ReqIFText attribute of a Requirement), and the LabelHelper.transformHTMLToText() post-process used for all fields has been removed for display in diagrams.

@@ -2,8 +2,8 @@ GROUP_TITLE_FOR_LABEL_AREA=Requirement's label
GROUP_TITLE_FOR_CONTENT_AREA=Requirement's content
EXPRESSION_LABEL_TEXT=Expression
LENGTH_LABEL_TEXT=Length (put nothing to display full text):
DefaultValueOfLabelExpression=aql:self.ownedAttributes->select( a | a.definition.ReqIFLongName == 'IE PUID').value
DefaultValueOfContentExpression=aql:OrderedSet{self.ReqIFLongName, self.ReqIFName, self.ReqIFChapterName, self.ReqIFText}->select(s | s != 'null' and s.size() > 0).prefix('- ')->sep('\\n')
DefaultValueOfLabelExpression=aql:self.getDefaultRequirementTitle()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

note that the user wants to customize its expressions. it is possible to call an aql expression for the text displayed in the Project Explorer, and on diagrams, title and content of requirements.
https://github.com/eclipse-capella/capella-requirements-vp/blob/master/plugins/org.polarsys.capella.vp.requirements.doc/html/3_userguide/34_preferences.mediawiki#requirement-label
and
https://github.com/eclipse-capella/capella-requirements-vp/blob/master/plugins/org.polarsys.capella.vp.requirements.doc/html/3_userguide/311_diagrams.mediawiki#label-and-content-configuration

if we use a service like proposed, then it can't customize it easily except by looking at source code on how is it coded.

one possible solution might be to do provide him services like below to ease the customisation.
title:
aql:self.toOneLinexxx("IE PUID", "ReqIfLongName")

and something like for content:
aql:self.separatexxx("reqIFLongName", "reqIFName", "reqIFChapterName", "reqIFText")

note that those services shall work also on Project Explorer expression. (called on https://github.com/eclipse-capella/capella-requirements-vp/blob/master/plugins/org.polarsys.kitalpha.vp.requirements.model.edit.decorators/src/org/polarsys/kitalpha/vp/requirements/Requirements/provider/RequirementItemProviderDecorator.java#L73 )

Dont forget to update the documentation to explain how to call those services.

Note that current models that will migrate to 7.0 must not have to update manually their expressions,
so if you just do a reduceString in CapellaRequirementsOpenJavaService.evaluateExpression
the current models will lose the call to the transformHTML stuff and their display will be broken.
you need to contribute to the migration to migrate the expressions (that may have changed from the default expression)
(to call the removed transformHTML method or to call the new services)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed, you raise a point we hadn't thought of with the expression update and migration ;)

@GlennPlou GlennPlou force-pushed the 180-master-requirement-title-description-services branch 2 times, most recently from 0cdc915 to ee37163 Compare July 23, 2024 13:56
@GlennPlou
Copy link
Author

Hi @pdulth, here is our final proposal for issue #180. This patch allows users to convert HTML content to text within their diagrams. The new htmlToText() operation has been incorporated into the default AQL expression and documented, enabling users to update their custom expressions manually if they choose. If they don't update their expression, they'll see HTML tags if there are any, or if they've used the RichText description.

@GlennPlou GlennPlou requested a review from pdulth July 23, 2024 14:10
Copy link

sonarcloud bot commented Jul 23, 2024

@zgrtls
Copy link

zgrtls commented Jul 26, 2024

If all the expressions pinpointed by @pdulth should output pure text and not HTML, why should the user need to be aware of this and call a new additional method ?
In addition to making it more complex for the user, no migration has been added to the add-on to ensure that the needed calls to this new mandatory method is added to the existing preferences/settings.

As far as I have seen, CapellaRequirementsOpenJavaService.getRequirementTitle() is expected to return a single line of pure text and CapellaRequirementsOpenJavaService.getRequirementContent() is expected to return multiple lines of pure text.
As CapellaRequirementsOpenJavaService.evalutateExpression() is only called by these two methods, why couldn't the method always transform the HTML into pure text and transform or not the BR into linefeeds through a new additional parameter ?

@ldelaigue
Copy link

If all the expressions pinpointed by @pdulth should output pure text and not HTML, why should the user need to be aware of this and call a new additional method ? In addition to making it more complex for the user, no migration has been added to the add-on to ensure that the needed calls to this new mandatory method is added to the existing preferences/settings.

As far as I have seen, CapellaRequirementsOpenJavaService.getRequirementTitle() is expected to return a single line of pure text and CapellaRequirementsOpenJavaService.getRequirementContent() is expected to return multiple lines of pure text. As CapellaRequirementsOpenJavaService.evalutateExpression() is only called by these two methods, why couldn't the method always transform the HTML into pure text and transform or not the BR into linefeeds through a new additional parameter ?

The reason why we've had to introduce explicit conversion of HTML to plain text via a java service is that the AQL expression used in the default value of the Requirement contents uses ->sep('\n') to add line breaks in the returned text.
If the result of this AQL expression contains HTML and needs to be converted to plain text, then all the '\n' will be converted to spaces because in HTML, '\n' does not mean line break.
As an example, you can save the HTML below in a file and open it in a web browser, it will display 'Hello World!' with just one space between 'Hello' and 'World', even though the HTML contains line breaks, tabs and spaces between these 2 words.
So, for this approach to work, the AQL expression would need to be modified to use ->sep('<br/>') instead of ->sep('\n'), which also impacts the end users (
is the HTML element that represents a line break).
In short, we haven't found any clean solution that would not impact the AQL expression, so we have proposed the solution that seemed to us to be the cleanest.

<html>
<head>
</head>
<body>
Hello

		   

World!
</body>
</html>

@pdulth
Copy link
Contributor

pdulth commented Sep 11, 2024

In another tool, we use something like getRequirementTitle and getRequirementContent that get rid or not of the endofline.
Can you look if something similar would fit as aql modifications might be impacting for users as contributing to migration seems difficult.

  public String getRequirementTitle(EObject self) {
    return evaluateExpression(element, xxxxx.getExpression(), xxxxx.getMaxLength(), 
        false);
  }

  public String getRequirementContent(EObject self) {
     return evaluateExpression(element, xxxxx.getExpression(), xxxxx.getMaxLength(), 
        true);
  }

///

  private static String evaluateExpression(EObject element, String expression, int maxLength, boolean keepEndl) {
    try {
      Session session = SessionManager.INSTANCE.getSession(element);

      if (session != null && expression != null) {
        IInterpreter interpreter = session.getInterpreter();
        if (interpreter != null) {
          Object value = interpreter.evaluate(element, expression);
          StringBuilder resultBuilder = new StringBuilder();
          if (value instanceof List<?>) {
            for (Object item : (List<?>) value) {
              resultBuilder.append(item);
            }
          } else {
            resultBuilder.append(value);
          }
          String evaluationResult = resultBuilder.toString();
          String sanytizedResult = getTextFromHtml(evaluationResult, keepEndl);
          return reduceString(sanytizedResult, maxLength);
        }
      }
    } catch (EvaluationException ex) {
      return "<Undefined>";
    }
    return "<Undefined>";
  }

  private static String reduceString(final String value, final int maxLength) {
    if (value.length() > maxLength) {
      return value.substring(0, maxLength).concat("...");
    }
    return value;
  }

  public static String getTextFromHtml(String html, boolean keepMultiline) {
    if (html == null) {
      return null;
    }

    Html2TextVisitor visitor = new Html2TextVisitor(keepMultiline);
    Document htmlDoc = Jsoup.parse(html);
    new NodeTraversor(visitor).traverse(htmlDoc);

    String text = visitor.toString();
    text = text.trim();
    return text;
  }


  private static class Html2TextVisitor implements NodeVisitor { 

    private static final Pattern MULTI_BLANKS_PATTERN = Pattern.compile("\\p{javaWhitespace}+");

    private final StringBuilder str = new StringBuilder();
    private final boolean keepMultiline;

    public Html2TextVisitor(boolean keepMultiline) {
      this.keepMultiline = keepMultiline;
    }

    private void addNewLineIfNoneBefore() {
      if (str.length() == 0 || str.charAt(str.length() - 1) == '\n') {
        return;
      }

      str.append('\n');
    }

    private void addBlankIfNoneBefore() {
      if (str.length() == 0 || Character.isWhitespace(str.charAt(str.length() - 1))) {
        return;
      }

      str.append(' ');
    }

    private void addBlankOrNewLineIfNoneBefore() {
      if (keepMultiline) {
        addNewLineIfNoneBefore();
      } else {
        addBlankIfNoneBefore();
      }
    }

    @Override
    public void head(Node node, int depth) {
      String name = node.nodeName();
      if (node instanceof TextNode) {
        String internalText = ((TextNode) node).text();
        // Internal text does not contains any formatting (as defined by HTML except inside "pre" tag) so multiple
        // blanks are equals to 1 and newline and stuff is the same, so replace them all by a single space
        // pre tags does not seem to be used by Capella editors so do not handle it
        String pureText = MULTI_BLANKS_PATTERN.matcher(internalText).replaceAll(" ");
        pureText = pureText.trim();
        str.append(pureText);
      } else if (name.equals("li")) {
        // Basic handling of list items (no difference between numbered and dotted nor depth)
        if (keepMultiline) {
          addNewLineIfNoneBefore();
          str.append(" * ");
        } else {
          addBlankIfNoneBefore();
          str.append("* ");
        }
      } else if (Arrays.asList("p", "h1", "h2", "h3", "h4", "h5", "tr").contains(name)) {
        addBlankOrNewLineIfNoneBefore();
      }
    }

    @Override
    public void tail(Node node, int depth) {
      String name = node.nodeName();
      if (name.equals("br")) {
        str.append(keepMultiline ? '\n' : ' ');
      } else if (Arrays.asList("li", "p", "h1", "h2", "h3", "h4", "h5").contains(name)) {
        addBlankOrNewLineIfNoneBefore();
      }
    }

    @Override
    public String toString() {
      return str.toString();
    }
}

Requirements fields are displayed as bulleted lists and line breaks are
preserved. HTML content is now handled in Requirements using the
org.jsoup library.

Change-Id: I9d0745bcf2871b94a99a15d8b5c72db156dad64e
Signed-off-by: Glenn Plouhinec <glenn.plouhinec@obeo.fr>
@GlennPlou GlennPlou force-pushed the 180-master-requirement-title-description-services branch from ee37163 to d73b413 Compare September 24, 2024 09:17
@GlennPlou
Copy link
Author

Hi @pdulth, I've analyzed your proposed solution. This solution works well for us, as it doesn't require any migration from users. I've updated the PR with a few modifications to what you proposed. The rendering in Requirements is satisfactory.
image
I've added org.jsoup to the TP, so it's up to you to decide if it's right for you.

@zgrtls
Copy link

zgrtls commented Sep 25, 2024

There is only 1 part that is incorrect I think :
pureText = pureText.replaceAll("- ", "\n- ");

It means that I am not allowed to use a string like "the particularly - IMPORTANT - title" as it will be split on 3 lines.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants