Re: [PR] Add HTML rule and hint+fix for missing alt attribute for tag img, applet and area [netbeans]

["matthiasblaesing (via GitHub)" <gi...@apache.org>]

matthiasblaesing commented on PR #4485:
URL: https://github.com/apache/netbeans/pull/4485#issuecomment-1927895147

   My gut feeling is, that the HtmlHintsProvider and the expectations of the CslTestBase/GsfHintsManager don't match. It seems suggestions were never really supported. It might be worthy looking into JS/CSS whether there are suggestions being checked. I'm not sure, that modifying CslTestBase is really a good idea.
   
   General comments:
   
   1. The unstable test you observed is because you dispatch the text change into another thread (`org.netbeans.modules.html.editor.hints.other.AddMissingAltAttributeHint.AddMissingAltAttributeHintFix.implement()`. That (a) makes it unstable and is IMHO wrong. I suggest to adjust to this:
       ```java
           @Override
           public void implement() throws Exception {
               BaseDocument document = (BaseDocument) context.getSnapshot().getSource().getDocument(true);
               document.runAtomic(() -> {
                   try {
                       OffsetRange adjustedRange = HtmlTagContextUtils.adjustContextRange(document, range.getStart(), range.getEnd(), true);
                       String tagContent = document.getText(adjustedRange.getStart(), adjustedRange.getLength());
   
                       // Find last self-closing or non self-closing tag
                       Pattern closingTagPattern = Pattern.compile("(/?>)");
                       Matcher closingTagMatcher = closingTagPattern.matcher(tagContent);
   
                       if (closingTagMatcher.find()) {
                           int altInsertPosition = adjustedRange.getStart() + closingTagMatcher.start(1);
   
                           // Check whether a space before alt is needed or not
                           boolean needsSpaceBefore = altInsertPosition == 0 || tagContent.charAt(closingTagMatcher.start(1) - 1) != ' ';
                           boolean isSelfClosing = closingTagMatcher.group(0).endsWith("/>");
                           String altAttribute = (needsSpaceBefore ? " " : "") + "alt=\"\"" + (isSelfClosing ? " " : ""); // NOI18N
   
                           document.insertString(altInsertPosition, altAttribute, null);
                       }
                   } catch (BadLocationException ex) {
                       // Ignore
                   }
               });
           }
       ```
       There is no need to do this on the EDT. The locking infrastructure of the documents should handle this.
   
   2. The `null` check in `CslTestBase#findApplicableFix` begins to make sense, when you write it like this: 
   
       ![image](https://github.com/apache/netbeans/assets/2179736/38711d1e-8dcf-40ae-a42f-33a2cd7597b8)
   
       The only difference compared to master is in line 4603, 4621 is only modified when comparing with your version.
   
       Then `org.netbeans.modules.csl.api.test.CslTestBase.applyHint(NbTestCase, Rule, String, String, String)` can be called with `null` as final parameter, which would invoke the found fix without checking for the description.
   
   3. Merges make it hard to compare against the base revision. Until this is changed incrementally/being reviewed I suggest to either not merge master or use rebases to cleanup/update.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For additional commands, e-mail: notifications-help@netbeans.apache.org

For further information about the NetBeans mailing lists, visit:
https://cwiki.apache.org/confluence/display/NETBEANS/Mailing+lists