You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@netbeans.apache.org by GitBox <gi...@apache.org> on 2022/05/27 13:47:06 UTC
[GitHub] [netbeans] sdedic commented on a diff in pull request #4091: Implementation of GoTo Symbol for Groovy
sdedic commented on code in PR #4091:
URL: https://github.com/apache/netbeans/pull/4091#discussion_r883397225
##########
groovy/groovy.editor/src/org/netbeans/modules/groovy/editor/api/GroovyIndex.java:
##########
@@ -348,6 +359,25 @@ public Set<IndexedMethod> getMethods(final String name, final String clz, QueryS
return methods;
}
+ private boolean matchInsensitiveCamelCase(String text, String where) {
+ if (text.length() > where.length()) {
+ return false;
+ }
+ String textP = text.toLowerCase();
+ String whereP = where.toLowerCase();
+ int index = -1;
+ for (int i = 0; i < textP.length(); i++) {
+ index = whereP.indexOf(textP.charAt(i), index + 1);
+ if (index == -1) {
+ return false;
+ }
+ if (whereP.length() <= (index + 1)) {
Review Comment:
I don't understand this condition; at line 370, `index` is computed as `whereP.indexOf`, which can be either `< whereP.length()` or `-1`. This condition seems always `false` for me.
##########
groovy/groovy.editor/src/org/netbeans/modules/groovy/editor/api/GroovyIndex.java:
##########
@@ -593,18 +630,28 @@ private IndexedMethod createMethod(String signature, IndexResult map) {
// Extract attributes
int attributeIndex = signature.indexOf(';', typeIndex + 1);
+ int offsetRangeIndex = signature.indexOf(";[", attributeIndex == -1 ? typeIndex + 1 : attributeIndex + 1);
Review Comment:
Hm, since `signature.indexOf(';', typeIndex + 1)` did not find even the `;` (-> attributeIndex == -1), it won't find `;[` either. Or in another way, if `attribute` part would be missing, then the `attributeIndex` will be the first to catch up on the `;[` string since it only looks for `;`. Take as an example `field;int;[100-130]` (missing attribute part).
Consider ordering all mandatory parts at the start and optionals at the end.
##########
groovy/groovy.editor/src/org/netbeans/modules/groovy/editor/api/GroovyIndexer.java:
##########
@@ -321,6 +323,9 @@ private void indexClass(ASTClass element, IndexDocument document) {
document.addPair(FQN_NAME, element.getFqn(), true, true);
document.addPair(CLASS_NAME, name, true, true);
document.addPair(CASE_INSENSITIVE_CLASS_NAME, name.toLowerCase(), true, true);
+ StringBuilder sb = new StringBuilder();
Review Comment:
Q: `addPair(CLASS_NAME,...)` is called multiple times (toplevel classes, inner classes), but [it is read as a single value here](https://github.com/apache/netbeans/pull/4091/files#diff-d59a38cf1daed90d439de7eb0be4bcea950ef015a4f2321d2cfdd9a50b90a104R607) -- will this work OK with fields of inner classes ? If not I'd suggest to file a bug for Groovy & fix it in a later PR.
##########
groovy/groovy.editor/src/org/netbeans/modules/groovy/editor/api/GroovyIndex.java:
##########
@@ -348,6 +359,25 @@ public Set<IndexedMethod> getMethods(final String name, final String clz, QueryS
return methods;
}
+ private boolean matchInsensitiveCamelCase(String text, String where) {
+ if (text.length() > where.length()) {
+ return false;
+ }
+ String textP = text.toLowerCase();
+ String whereP = where.toLowerCase();
+ int index = -1;
+ for (int i = 0; i < textP.length(); i++) {
+ index = whereP.indexOf(textP.charAt(i), index + 1);
+ if (index == -1) {
+ return false;
+ }
+ if (whereP.length() <= (index + 1)) {
Review Comment:
BTW do I understand right that this check basically returns `true`, if the characters present in `text` are found (case-insensitive) in `where` in the same order, so e.g. `matchInsensitiveCamelCase("bfu", "beloved favourite user") == true` (see no CamelCase in either search pattern or the matched text) ?
##########
groovy/groovy.editor/src/org/netbeans/modules/groovy/editor/api/GroovyIndex.java:
##########
@@ -252,10 +253,18 @@ public Set<IndexedMethod> getConstructors(final String className) {
}
}
int flags = 0;
- if (parts.length > 2) {
+ if (parts.length > 3) {
Review Comment:
Please document what the `signature` syntax is; from the code it seems that e.g. `type` and `attribute` part may be optional (there are checks for that, but still better to have it documented), the `range` part is (?) mandatory.
Depending on that various checks may be insufficient; e.g. there are (?) 2 optional parts (type, attributes) separted by the same separator. So from notation `name;a` it's not obvious what 'a' is: a type or an attribute ?
But from the Indexer code, it seems that `type` is always present, so maybe the checks in index reading code can be simplified ?
##########
groovy/groovy.editor/src/org/netbeans/modules/groovy/editor/language/GroovyTypeSearcher.java:
##########
@@ -59,10 +62,38 @@ public class GroovyTypeSearcher implements IndexSearcher {
@Override
public Set<? extends Descriptor> getSymbols(Project project, String textForQuery, Kind kind, Helper helper) {
- // TODO - search for methods too!!
+ GroovyIndex index = GroovyIndex.get(QuerySupport.findRoots(project, Collections.singleton(ClassPath.SOURCE), Collections.<String>emptySet(), Collections.<String>emptySet()));
- // For now, just at a minimum do the types
- return getTypes(project, textForQuery, kind, helper);
+ kind = adjustKind(kind, textForQuery);
+
+ if (kind == QuerySupport.Kind.CASE_INSENSITIVE_PREFIX /*|| kind == QuerySupport.Kind.CASE_INSENSITIVE_REGEXP*/) {
+ textForQuery = textForQuery.toLowerCase(Locale.ENGLISH);
+ }
+
+ Set<GroovyTypeDescriptor> result = new HashSet<GroovyTypeDescriptor>();
+
+
+ if (textForQuery.length() > 0) {
+ Set<IndexedClass> classes = null;
+ classes = index.getClasses(textForQuery, kind);
+ for (IndexedClass cls : classes) {
+ result.add(new GroovyTypeDescriptor(cls, helper));
+ }
+
+ Set<IndexedField> fields = index.getFields(textForQuery, null, kind);
+ for (IndexedField field : fields) {
+ result.add(new GroovyTypeDescriptor(field, helper));
+ }
+
+ Set<IndexedMethod> methods = index.getMethods(textForQuery, null, kind);
+ for (IndexedMethod method : methods) {
+ if (method.getOffsetRange(null) != OffsetRange.NONE) {
Review Comment:
Why is this check done for methods and not for fields + classes ? Synthetic methods ? Shouldn't we rather flag them with an attribute ?
--
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