[GitHub] [netbeans] lahodaj commented on a change in pull request #2282: [NETBEANS-3990] Sealed color and formatting

[GitBox <gi...@apache.org>]

lahodaj commented on a change in pull request #2282:
URL: https://github.com/apache/netbeans/pull/2282#discussion_r475705259



##########
File path: java/java.editor.base/src/org/netbeans/modules/java/editor/base/semantic/SemanticHighlighterBase.java
##########
@@ -871,7 +872,18 @@ public Void visitClass(ClassTree tree, Void p) {
             }
             scan(tree.getExtendsClause(), null);
             scan(tree.getImplementsClause(), null);
-
+            try {
+                while (!TokenUtilities.textEquals(tl.currentToken().text(), "{")) {// NOI18N

Review comment:
       My suggestion here would be to:
   1. read the Permitted subclasses
   2. if non-null/non-empty, do simply firstIdentifierToken("permits"), and add the token to contextKeywords
   3. run scan(<permitted>, null)
   (so that the classes inside the permits clause are highlighted).
   

##########
File path: java/java.lexer/src/org/netbeans/api/java/lexer/JavaTokenId.java
##########
@@ -133,6 +133,9 @@
     TRUE("true", "literal"),
     FALSE("false", "literal"),
     NULL("null", "literal"),
+    PERMITS("permits", "keyword"),

Review comment:
       These new constants do not seem to be really used, right? I.e. the lexer will never produce a token with this kind?

##########
File path: java/java.source.base/src/org/netbeans/modules/java/source/save/Reformatter.java
##########
@@ -1585,7 +1591,7 @@ public Boolean visitModifiers(ModifiersTree node, Void p) {
                 int lblti = lastBlankLinesTokenIndex;
                 Diff lbld = lastBlankLinesDiff;
                 id = accept(PRIVATE, PROTECTED, PUBLIC, STATIC, DEFAULT, TRANSIENT, FINAL,
-                        ABSTRACT, NATIVE, VOLATILE, SYNCHRONIZED, STRICTFP, AT);
+                        ABSTRACT, NATIVE, VOLATILE, SYNCHRONIZED, STRICTFP, AT, SEALED, NONSEALED);

Review comment:
       I see the token ids are useful here - but I would suggest to add a way to do without them. Enhancing API just to simplify one place a little bit will hurt long-term maintenance.

##########
File path: java/java.source.base/src/org/netbeans/modules/java/source/save/Reformatter.java
##########
@@ -954,6 +955,11 @@ public Boolean visitClass(ClassTree node, Void p) {
                         wrapToken(cs.wrapExtendsImplementsKeyword(), 1, id == INTERFACE ? EXTENDS : IMPLEMENTS);
                         wrapList(cs.wrapExtendsImplementsList(), cs.alignMultilineImplements(), true, COMMA, impls);
                     }
+                    List<? extends Tree> perms = TreeShims.getPermits(node);
+                    if (perms != null && !perms.isEmpty()) {
+                        wrapToken(cs.wrapExtendsImplementsKeyword(), 1, PERMITS);

Review comment:
       I think it is sensible to use the same settings as for wrapping extends/implements. We may consider tweaking the UI to also mention permits (this applies to both wrapExtendsImplementsKeyword and wrapExtendsImplementsList).

##########
File path: java/java.editor.base/src/org/netbeans/modules/java/editor/base/semantic/ColoringAttributes.java
##########
@@ -30,7 +30,9 @@
 public enum ColoringAttributes {
 
     UNUSED,
-
+    SEALED,

Review comment:
       These new constants don't seem to be needed, right? The keywords should have attribute KEYWORD, I think, and that should be enough to make them highlighted as a keyword.

##########
File path: java/java.source.base/src/org/netbeans/modules/java/source/save/Reformatter.java
##########
@@ -3524,6 +3530,10 @@ private JavaTokenId accept(JavaTokenId first, JavaTokenId... rest) {
                             contains = true;
                             break;
                         }
+                        if (TokenUtilities.textEquals(tokens.token().text(), "non") && tokens.moveNext() && TokenUtilities.textEquals(tokens.token().text(), "-") && tokens.moveNext() && TokenUtilities.textEquals(tokens.token().text(), "sealed")) {// NOI18N

Review comment:
       What happens if "non" is not followed by "-sealed"?

##########
File path: java/java.editor.base/src/org/netbeans/modules/java/editor/base/semantic/SemanticHighlighterBase.java
##########
@@ -953,6 +965,25 @@ public Void scan(Tree tree, Void p) {
                 handlePossibleIdentifier(tp, true, info.getTrees().getElement(tp));
                 tl.moveToOffset(sourcePositions.getEndPosition(getCurrentPath().getCompilationUnit(), TreeShims.getBindingPatternType(tree)));
                 firstIdentifier(tp, TreeShims.getBinding(tree).toString());
+            } else if (tree != null && tree.getKind().equals(Kind.MODIFIERS)) {

Review comment:
       Here, I would suggest to create a new method "visitModifiers", and put this code into it. Also, it will possibly be a little bit more complex to handle, as "sealed" and "non-sealed" may be intermixed in other modifiers and annotations. I guess I would suggest to avoid doing toString() on the tree - what will happen if the modifiers contain something like:
   @SuppressWarnings("non-sealed")
   ?




----------------------------------------------------------------
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.

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