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:
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
For further information about the NetBeans mailing lists, visit:
https://cwiki.apache.org/confluence/display/NETBEANS/Mailing+lists
