You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@netbeans.apache.org by "rossluk (via GitHub)" <gi...@apache.org> on 2023/04/29 11:00:48 UTC
[GitHub] [netbeans] rossluk opened a new pull request, #5900: [PHP] FixUsesPerformer and AddUseImportSuggestion improvement
rossluk opened a new pull request, #5900:
URL: https://github.com/apache/netbeans/pull/5900
Hi,
This PR adds "append uses" logic (import use one by one) for FixUsesPerformer to have ability to use the same logic (classes) for inline "use import" like in AddUseImportSuggestion. Also it includes refactoring for AddUseImportSuggestion to use FixUsesPerformer instead of its own logic. I know that implementation is not an ideal, but I believe it would be more convenient and much easier to maintain same logic in one place, and such changes could allow to implement, for instance, something like PhpStorm auto import in the feature if needed.
Changes are pretty complex and I think there weren't much people using AddUseImportSuggestion logic (because it was useless for a long time due to wrong class detection, ignoring traits, types, return types, functions etc.), but I hope now it's more useful, however I am still not sure that changes will be approved, so I committed final logic (only with initial tests) and decided to discuss my changes and additional change to previously committed logic here:
- While I was working on the PR there was added PSR12 formatting and it's fine, I aligned the logic due to it, but I'd like to suggest moving the "psr12 sorting checkbox" to the editor general uses options (where "prefer grouping" and other such option are situated) and change it to something like "Sort uses radiobutton - Alphabetical/PSR12", what do you think? Such change needed to have ability using the same option globally (like prefereGroupings etc) for AddUseImportSuggestion as well.
If it is approved I am going to add more tests for both classes FixUsesPerformer and AddUseImportSuggestion.
--
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
[GitHub] [netbeans] rossluk commented on a diff in pull request #5900: [PHP] FixUsesPerformer and AddUseImportSuggestion improvement
Posted by "rossluk (via GitHub)" <gi...@apache.org>.
rossluk commented on code in PR #5900:
URL: https://github.com/apache/netbeans/pull/5900#discussion_r1187214639
##########
php/php.editor/src/org/netbeans/modules/php/editor/actions/FixUsesPerformer.java:
##########
@@ -631,13 +757,221 @@ private int getOffsetWithoutLeadingWhitespaces(final int startOffset) {
return result;
}
+ private int getOffsetWithoutFollowingWhitespaces(final int endOffset) {
+ int result = endOffset;
+ baseDocument.readLock();
+ try {
+ TokenSequence<PHPTokenId> ts = LexUtilities.getPHPTokenSequence(baseDocument, endOffset);
+ if (ts != null) {
+ ts.move(endOffset);
+ while (ts.moveNext() && ts.token().id().equals(PHPTokenId.WHITESPACE)) {
+ result = ts.offset() + ts.token().length();
+ }
+ }
+ } finally {
+ baseDocument.readUnlock();
+ }
+ return result;
+ }
+
+ private class UsesInsertStringHelper {
Review Comment:
But in this case it shouldn't exist without outer class because it's using outer class attribute and, although it isn't so critical for short living objects, but the static classes are increasing the memory consumption. I will put editList directly to the helper constructor, but the logic still isn't clear enough for me.
--
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
[GitHub] [netbeans] junichi11 commented on pull request #5900: [PHP] FixUsesPerformer and AddUseImportSuggestion improvement
Posted by "junichi11 (via GitHub)" <gi...@apache.org>.
junichi11 commented on PR #5900:
URL: https://github.com/apache/netbeans/pull/5900#issuecomment-1534466796
@tmysik Thank you for your advice and help :) I just looked at the changes. So, I need more time to understand them.
> In general, I am not against this change if all the tests are still passing and newly added tests make sense (and cover the change nicely).
Yes, I agree with this.
> The question is, how maintainable the code will be
My concern is this. If it's hard to maintain, we need to improve this, I think.
@rossluk Thank you for working on this :) I'll read the code later when I have time. So, let's improve this with us little by little.
--
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
[GitHub] [netbeans] rossluk commented on a diff in pull request #5900: [PHP] FixUsesPerformer and AddUseImportSuggestion improvement
Posted by "rossluk (via GitHub)" <gi...@apache.org>.
rossluk commented on code in PR #5900:
URL: https://github.com/apache/netbeans/pull/5900#discussion_r1185464994
##########
php/php.editor/src/org/netbeans/modules/php/editor/actions/FixUsesPerformer.java:
##########
@@ -631,13 +757,221 @@ private int getOffsetWithoutLeadingWhitespaces(final int startOffset) {
return result;
}
+ private int getOffsetWithoutFollowingWhitespaces(final int endOffset) {
+ int result = endOffset;
+ baseDocument.readLock();
+ try {
+ TokenSequence<PHPTokenId> ts = LexUtilities.getPHPTokenSequence(baseDocument, endOffset);
+ if (ts != null) {
+ ts.move(endOffset);
+ while (ts.moveNext() && ts.token().id().equals(PHPTokenId.WHITESPACE)) {
Review Comment:
As I understand ts.moveNext() should return false if token is null.
--
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
[GitHub] [netbeans] rossluk commented on a diff in pull request #5900: [PHP] FixUsesPerformer and AddUseImportSuggestion improvement
Posted by "rossluk (via GitHub)" <gi...@apache.org>.
rossluk commented on code in PR #5900:
URL: https://github.com/apache/netbeans/pull/5900#discussion_r1185476692
##########
php/php.editor/src/org/netbeans/modules/php/editor/actions/FixUsesPerformer.java:
##########
@@ -425,28 +432,44 @@ private String createInsertString(final List<UsePart> useParts) {
} else {
Collections.sort(useParts);
}
- if (!useParts.isEmpty()) {
- insertString.append(NEW_LINE);
- }
+
String indentString = null;
if (options.preferGroupUses()
|| options.preferMultipleUseStatementsCombined()) {
CodeStyle codeStyle = CodeStyle.get(baseDocument);
indentString = IndentUtils.createIndentString(codeStyle.getIndentSize(), codeStyle.expandTabToSpaces(), codeStyle.getTabSize());
}
- if (options.preferGroupUses()
+ usesInsertStringHelper.setWrapInNewLinesOnAppend(useParts.size() == 1);
+ if (options.preferGroupUses() && usesInsertStringHelper.canGroupUses()
&& options.getPhpVersion().compareTo(PhpVersion.PHP_70) >= 0) {
- insertString.append(createStringForGroupUse(useParts, indentString));
+ createStringForGroupUse(useParts, indentString, usesInsertStringHelper);
} else if (options.preferMultipleUseStatementsCombined()) {
- insertString.append(createStringForMultipleUse(useParts, indentString));
+ createStringForMultipleUse(useParts, indentString, usesInsertStringHelper);
} else {
- insertString.append(createStringForCommonUse(useParts));
+ createStringForCommonUse(useParts, usesInsertStringHelper);
+ }
+
+ if (!usesInsertStringHelper.isAppendUses()) {
+ try {
+ String replaceString = baseDocument.getText(usesInsertStringHelper.getInitialStartOffset(), usesInsertStringHelper.getStartOffset() - usesInsertStringHelper.getInitialStartOffset());
+ if (replaceString.equals(usesInsertStringHelper.getResultString())) {
+ usesInsertStringHelper.resetResultString();
+ return;
+ }
+ } catch (BadLocationException ex) {
+ Exceptions.printStackTrace(ex);
Review Comment:
Could you clarify please? As I see there is some log inside of printStackTrace.
--
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
[GitHub] [netbeans] rossluk commented on pull request #5900: [PHP] FixUsesPerformer and AddUseImportSuggestion improvement
Posted by "rossluk (via GitHub)" <gi...@apache.org>.
rossluk commented on PR #5900:
URL: https://github.com/apache/netbeans/pull/5900#issuecomment-1590003373
@junichi11 I could try to simplify the whole logic (remove helper class, omit PSR12/newline checks, regroups etc.) and always fallback to createStringForCommonUse for a single use import. Is it make any sense?
--
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
[GitHub] [netbeans] junichi11 commented on a diff in pull request #5900: [PHP] FixUsesPerformer and AddUseImportSuggestion improvement
Posted by "junichi11 (via GitHub)" <gi...@apache.org>.
junichi11 commented on code in PR #5900:
URL: https://github.com/apache/netbeans/pull/5900#discussion_r1184562897
##########
php/php.editor/src/org/netbeans/modules/php/editor/actions/FixUsesPerformer.java:
##########
@@ -105,36 +110,56 @@ public FixUsesPerformer(
this.options = options;
}
- public void perform() {
+ private void perform(boolean append) {
final Document document = parserResult.getSnapshot().getSource().getDocument(false);
if (document instanceof BaseDocument) {
baseDocument = (BaseDocument) document;
editList = new EditList(baseDocument);
namespaceScope = ModelUtils.getNamespaceScope(parserResult.getModel().getFileScope(), importData.caretPosition);
assert namespaceScope != null;
- processSelections();
+ processSelections(append);
editList.apply();
}
}
+ public void perform()
+ {
Review Comment:
Formatting
##########
php/php.editor/src/org/netbeans/modules/php/editor/actions/FixUsesPerformer.java:
##########
@@ -425,28 +432,44 @@ private String createInsertString(final List<UsePart> useParts) {
} else {
Collections.sort(useParts);
}
- if (!useParts.isEmpty()) {
- insertString.append(NEW_LINE);
- }
+
String indentString = null;
if (options.preferGroupUses()
|| options.preferMultipleUseStatementsCombined()) {
CodeStyle codeStyle = CodeStyle.get(baseDocument);
indentString = IndentUtils.createIndentString(codeStyle.getIndentSize(), codeStyle.expandTabToSpaces(), codeStyle.getTabSize());
}
- if (options.preferGroupUses()
+ usesInsertStringHelper.setWrapInNewLinesOnAppend(useParts.size() == 1);
+ if (options.preferGroupUses() && usesInsertStringHelper.canGroupUses()
&& options.getPhpVersion().compareTo(PhpVersion.PHP_70) >= 0) {
- insertString.append(createStringForGroupUse(useParts, indentString));
+ createStringForGroupUse(useParts, indentString, usesInsertStringHelper);
} else if (options.preferMultipleUseStatementsCombined()) {
- insertString.append(createStringForMultipleUse(useParts, indentString));
+ createStringForMultipleUse(useParts, indentString, usesInsertStringHelper);
} else {
- insertString.append(createStringForCommonUse(useParts));
+ createStringForCommonUse(useParts, usesInsertStringHelper);
+ }
+
+ if (!usesInsertStringHelper.isAppendUses()) {
+ try {
+ String replaceString = baseDocument.getText(usesInsertStringHelper.getInitialStartOffset(), usesInsertStringHelper.getStartOffset() - usesInsertStringHelper.getInitialStartOffset());
+ if (replaceString.equals(usesInsertStringHelper.getResultString())) {
+ usesInsertStringHelper.resetResultString();
+ return;
+ }
+ } catch (BadLocationException ex) {
+ Exceptions.printStackTrace(ex);
Review Comment:
Maybe, I would add a log.
##########
php/php.editor/src/org/netbeans/modules/php/editor/actions/FixUsesPerformer.java:
##########
@@ -501,7 +522,7 @@ private List<String> usePartsToNamespaces(List<UsePart> useParts) {
.collect(Collectors.toList());
}
- private void createStringForGroupUse(StringBuilder insertString, String indentString, String usePrefix, List<UsePart> useParts) {
+ private void createStringForGroupUse(String indentString, String usePrefix, List<UsePart> useParts, final UsesInsertStringHelper usesInsertStringHelper) {
Review Comment:
I would add it to the first param. (Easy to see the diff.)
##########
php/php.editor/src/org/netbeans/modules/php/editor/actions/FixUsesPerformer.java:
##########
@@ -518,42 +539,56 @@ private void createStringForGroupUse(StringBuilder insertString, String indentSt
if (groupUsePrefix != null) {
if (lastGroupUsePrefix != null
&& !lastGroupUsePrefix.equals(groupUsePrefix)) {
- processGroupedUseParts(insertString, indentString, usePrefix, lastGroupUsePrefix, groupedUseParts);
+ processGroupedUseParts(indentString, usePrefix, lastGroupUsePrefix, groupedUseParts, usesInsertStringHelper);
}
lastGroupUsePrefix = groupUsePrefix;
- processNonGroupedUseParts(insertString, indentString, nonGroupedUseParts);
+ processNonGroupedUseParts(indentString, nonGroupedUseParts, usesInsertStringHelper);
groupedUseParts.add(usePart);
} else {
- processGroupedUseParts(insertString, indentString, usePrefix, lastGroupUsePrefix, groupedUseParts);
+ processGroupedUseParts(indentString, usePrefix, lastGroupUsePrefix, groupedUseParts, usesInsertStringHelper);
nonGroupedUseParts.add(usePart);
}
}
- processNonGroupedUseParts(insertString, indentString, nonGroupedUseParts);
- processGroupedUseParts(insertString, indentString, usePrefix, lastGroupUsePrefix, groupedUseParts);
+ processNonGroupedUseParts(indentString, nonGroupedUseParts, usesInsertStringHelper);
+ processGroupedUseParts(indentString, usePrefix, lastGroupUsePrefix, groupedUseParts, usesInsertStringHelper);
}
- private void processNonGroupedUseParts(StringBuilder insertString, String indentString, List<UsePart> nonGroupedUseParts) {
+ private void processNonGroupedUseParts(String indentString, List<UsePart> nonGroupedUseParts, final UsesInsertStringHelper usesInsertStringHelper) {
if (nonGroupedUseParts.isEmpty()) {
return;
}
- if (options.preferMultipleUseStatementsCombined()) {
- insertString.append(createStringForMultipleUse(nonGroupedUseParts, indentString));
+ // it is hard to handle multiple use and group at once on appendUses
+ // because there is a probability of recursive changes, so we just fallback
+ // to common use insertion in such case to avoid complexity of code
+ if (options.preferMultipleUseStatementsCombined() && !usesInsertStringHelper.isAppendUses()) {
+ createStringForMultipleUse(nonGroupedUseParts, indentString, usesInsertStringHelper);
} else {
- insertString.append(createStringForCommonUse(nonGroupedUseParts));
+ createStringForCommonUse(nonGroupedUseParts, usesInsertStringHelper);
}
nonGroupedUseParts.clear();
}
- private void processGroupedUseParts(StringBuilder insertString, String indentString, String usePrefix, String groupUsePrefix, List<UsePart> groupedUseParts) {
+ private void processGroupedUseParts(String indentString, String usePrefix, String groupUsePrefix, List<UsePart> groupedUseParts, final UsesInsertStringHelper usesInsertStringHelper) {
Review Comment:
I would add it to the first param. (Easy to see the diff.)
##########
php/php.editor/src/org/netbeans/modules/php/editor/actions/FixUsesPerformer.java:
##########
@@ -595,23 +666,78 @@ private String createStringForMultipleUse(List<UsePart> useParts, String indentS
}
insertString.append(usePart.getTextPart());
}
- if (!useParts.isEmpty()) {
- insertString.append(SEMICOLON);
+ insertString.append(SEMICOLON);
+ if (usesInsertStringHelper.isAppendUses()) {
+ if (hasChanges) {
+ usesInsertStringHelper.applyDirectChangeAt(firstElementOffset, insertString.toString());
+ } else {
+ usesInsertStringHelper.resetPossibleRemoves();
+ usesInsertStringHelper.moveStartOffsetToTheEnd();
+ }
+ } else {
+ usesInsertStringHelper.appendToResultString(insertString.toString());
}
- return insertString.toString();
}
- private String createStringForCommonUse(List<UsePart> useParts) {
- StringBuilder result = new StringBuilder();
+ private void createStringForCommonUse(List<UsePart> useParts, final UsesInsertStringHelper usesInsertStringHelper) {
+ StringBuilder insertString = new StringBuilder();
UsePart.Type lastUseType = null;
+ boolean hasChanges = false;
+ int typeElementsCount = 0;
+ int firstElementOffset = 0;
for (UsePart usePart : useParts) {
if (putInPSR12Order && lastUseType != null && lastUseType != usePart.getType()) {
- appendNewLine(result);
+ if (usesInsertStringHelper.isAppendUses()) {
+ if (hasChanges) {
+ usesInsertStringHelper.setWrapInNewLinesOnAppend(typeElementsCount == 1);
+ usesInsertStringHelper.confirmResultStringAt(firstElementOffset);
+ } else {
+ usesInsertStringHelper.resetResultString();
+ usesInsertStringHelper.moveStartOffsetToTheEnd();
+ firstElementOffset = 0;
+ }
+ hasChanges = false;
+ } else {
+ usesInsertStringHelper.appendToResultString(NEW_LINE);
+ }
+ typeElementsCount = 0;
}
- result.append(usePart.getUsePrefix()).append(usePart.getTextPart()).append(SEMICOLON);
+ // here we check changes in whole type block for PSR12Order
+ hasChanges = hasChanges || usePart.getOffset() == 0;
+ firstElementOffset = usePart.getOffset() == 0 || firstElementOffset != 0 && firstElementOffset < usePart.getOffset()
+ ? firstElementOffset : usePart.getOffset();
lastUseType = usePart.getType();
+ typeElementsCount++;
+
+ insertString.append(usePart.getUsePrefix()).append(usePart.getTextPart()).append(SEMICOLON);
+ if (usesInsertStringHelper.isAppendUses() && !putInPSR12Order) {
+ if (usesInsertStringHelper.moveEndOffset(usePart.getOffset())) {
+ usesInsertStringHelper.addToPossibleRemoveMap(usePart.getOffset());
+ usesInsertStringHelper.moveStartOffsetToTheEnd();
+ }
+ // here we check inline change only
+ hasChanges = usePart.getOffset() == 0;
+ if (hasChanges) {
+ usesInsertStringHelper.applyDirectChange(insertString.toString());
+ } else {
+ usesInsertStringHelper.resetPossibleRemoves();
+ }
+ } else {
+ usesInsertStringHelper.addToPossibleRemoveMap(usePart.getOffset());
+ usesInsertStringHelper.moveEndOffset(usePart.getOffset());
+ usesInsertStringHelper.appendToResultString(insertString.toString());
+ }
+ insertString = new StringBuilder();
Review Comment:
The same as above.
##########
php/php.editor/src/org/netbeans/modules/php/editor/actions/FixUsesPerformer.java:
##########
@@ -631,13 +757,221 @@ private int getOffsetWithoutLeadingWhitespaces(final int startOffset) {
return result;
}
+ private int getOffsetWithoutFollowingWhitespaces(final int endOffset) {
+ int result = endOffset;
+ baseDocument.readLock();
+ try {
+ TokenSequence<PHPTokenId> ts = LexUtilities.getPHPTokenSequence(baseDocument, endOffset);
+ if (ts != null) {
+ ts.move(endOffset);
+ while (ts.moveNext() && ts.token().id().equals(PHPTokenId.WHITESPACE)) {
+ result = ts.offset() + ts.token().length();
+ }
+ }
+ } finally {
+ baseDocument.readUnlock();
+ }
+ return result;
+ }
+
+ private class UsesInsertStringHelper {
+ private final Map<Integer, OffsetRange> usedRanges;
+ private final boolean appendUses;
+ private final boolean hasMultipleUses;
+ private StringBuilder resultString = new StringBuilder();
+ private Set<Integer> followingWhitespaceReplaceOffset = new HashSet<>();
+ private Set<Integer> savedHeadingWhitespaceOffset = new HashSet<>();
+ private int startOffset;
+ private int initialStartOffset;
+ private int endOffset;
+ private boolean wrapInNewLines = false;
+ private Map<Integer, Integer> usesToRemove = new HashMap<>();
+ private List<Integer> usesToRemoveOffsetTmp = new ArrayList<>();
+
+ UsesInsertStringHelper(Map<Integer, OffsetRange> usedRanges, boolean hasMultipleUses, boolean appendUses, int startOffset) {
+ this.usedRanges = usedRanges;
+ this.startOffset = startOffset;
+ this.initialStartOffset = startOffset;
+ this.hasMultipleUses = hasMultipleUses;
+ this.appendUses = appendUses;
+ }
+
+ public OffsetRange getUsedRangeOffset(int offset) {
+ return usedRanges.get(offset);
+ }
+
+ public void addToPossibleRemoveMap(int offset) {
+ if (usedRanges.get(offset) != null) {
+ usesToRemoveOffsetTmp.add(offset);
+ }
+ }
+
+ public Map<Integer, Integer> getUsesToRemove() {
+ return usesToRemove;
+ }
+
+ public int getStartOffset() {
+ return startOffset;
+ }
+
+ public int getInitialStartOffset() {
+ return initialStartOffset;
+ }
+
+ public void namespaceChange(int startOffset) {
+ this.startOffset = startOffset;
+ this.initialStartOffset = startOffset;
+ this.endOffset = startOffset;
+ }
+
+ public java.util.Set<Integer> getFollowingWhitespaceReplaceOffsets() {
+ return followingWhitespaceReplaceOffset;
+ }
+
+ public boolean needToSaveHeadingWhitespace(int offset) {
+ return savedHeadingWhitespaceOffset.contains(offset);
+ }
+
+ public boolean needToReplaceFollowingWhitespace(int offset) {
+ return followingWhitespaceReplaceOffset.contains(offset);
+ }
+
+ public void skipFollowingWhitespaceReplace(int offset) {
+ followingWhitespaceReplaceOffset.remove((Integer) offset);
+ }
+
+ public boolean moveEndOffset(int removedUseOffset) {
+ OffsetRange removed = this.getUsedRangeOffset(removedUseOffset);
+ if (removed != null) {
+ this.endOffset = removed.getEnd() > endOffset ? removed.getEnd() : endOffset;
+ return true;
+ }
+ return false;
+ }
+
+ public void moveStartOffsetToTheEnd() {
Review Comment:
Nitpick: Can use shorter name (`moveStartOffsetToEnd`)
##########
php/php.editor/src/org/netbeans/modules/php/editor/actions/FixUsesPerformer.java:
##########
@@ -631,13 +757,221 @@ private int getOffsetWithoutLeadingWhitespaces(final int startOffset) {
return result;
}
+ private int getOffsetWithoutFollowingWhitespaces(final int endOffset) {
+ int result = endOffset;
+ baseDocument.readLock();
+ try {
+ TokenSequence<PHPTokenId> ts = LexUtilities.getPHPTokenSequence(baseDocument, endOffset);
+ if (ts != null) {
+ ts.move(endOffset);
+ while (ts.moveNext() && ts.token().id().equals(PHPTokenId.WHITESPACE)) {
+ result = ts.offset() + ts.token().length();
+ }
+ }
+ } finally {
+ baseDocument.readUnlock();
+ }
+ return result;
+ }
+
+ private class UsesInsertStringHelper {
+ private final Map<Integer, OffsetRange> usedRanges;
+ private final boolean appendUses;
+ private final boolean hasMultipleUses;
+ private StringBuilder resultString = new StringBuilder();
+ private Set<Integer> followingWhitespaceReplaceOffset = new HashSet<>();
+ private Set<Integer> savedHeadingWhitespaceOffset = new HashSet<>();
+ private int startOffset;
+ private int initialStartOffset;
+ private int endOffset;
+ private boolean wrapInNewLines = false;
+ private Map<Integer, Integer> usesToRemove = new HashMap<>();
+ private List<Integer> usesToRemoveOffsetTmp = new ArrayList<>();
+
+ UsesInsertStringHelper(Map<Integer, OffsetRange> usedRanges, boolean hasMultipleUses, boolean appendUses, int startOffset) {
+ this.usedRanges = usedRanges;
+ this.startOffset = startOffset;
+ this.initialStartOffset = startOffset;
+ this.hasMultipleUses = hasMultipleUses;
+ this.appendUses = appendUses;
+ }
+
+ public OffsetRange getUsedRangeOffset(int offset) {
+ return usedRanges.get(offset);
+ }
+
+ public void addToPossibleRemoveMap(int offset) {
+ if (usedRanges.get(offset) != null) {
+ usesToRemoveOffsetTmp.add(offset);
+ }
+ }
+
+ public Map<Integer, Integer> getUsesToRemove() {
+ return usesToRemove;
+ }
+
+ public int getStartOffset() {
+ return startOffset;
+ }
+
+ public int getInitialStartOffset() {
+ return initialStartOffset;
+ }
+
+ public void namespaceChange(int startOffset) {
+ this.startOffset = startOffset;
+ this.initialStartOffset = startOffset;
+ this.endOffset = startOffset;
+ }
+
+ public java.util.Set<Integer> getFollowingWhitespaceReplaceOffsets() {
+ return followingWhitespaceReplaceOffset;
+ }
+
+ public boolean needToSaveHeadingWhitespace(int offset) {
+ return savedHeadingWhitespaceOffset.contains(offset);
+ }
+
+ public boolean needToReplaceFollowingWhitespace(int offset) {
+ return followingWhitespaceReplaceOffset.contains(offset);
+ }
+
+ public void skipFollowingWhitespaceReplace(int offset) {
+ followingWhitespaceReplaceOffset.remove((Integer) offset);
+ }
+
+ public boolean moveEndOffset(int removedUseOffset) {
+ OffsetRange removed = this.getUsedRangeOffset(removedUseOffset);
+ if (removed != null) {
+ this.endOffset = removed.getEnd() > endOffset ? removed.getEnd() : endOffset;
+ return true;
+ }
+ return false;
+ }
+
+ public void moveStartOffsetToTheEnd() {
+ this.startOffset = this.endOffset;
+ }
+
+ public void applyDirectChangeAt(int offset, String insertString) {
+ if (offset == 0 || this.getUsedRangeOffset(offset) == null) {
+ applyDirectChange(insertString);
+ return;
+ }
+ int currentStart = this.startOffset;
+ this.startOffset = this.getUsedRangeOffset(offset).getEnd();
+ savedHeadingWhitespaceOffset.add(this.getUsedRangeOffset(offset).getStart());
+ // remove new line because of insert on existing place
+ insertString = insertString.substring(1);
+ applyDirectChange(insertString);
+ this.startOffset = currentStart;
+ }
+
+ public void applyDirectChange(String insertString)
+ {
+ for (Integer offset : usesToRemoveOffsetTmp) {
+ addToRemoveMap(offset);
+ }
+
+ if (shouldAppendLineBefore()) {
+ editList.replace(getStartOffset(), 0, NEW_LINE, false, 0);
+ followingWhitespaceReplaceOffset.add(getStartOffset());
+ editList.replace(getStartOffset(), 0, insertString, false, 0);
+ } else {
+ editList.replace(getStartOffset(), 0, insertString, false, 0);
+ }
+
+ if (wrapInNewLines) {
+ editList.replace(getStartOffset(), 0, NEW_LINE, false, 0);
+ wrapInNewLines = false;
+ }
+ }
+
+ public void setWrapInNewLinesOnAppend(boolean value) {
+ wrapInNewLines = value;
+ }
+
+ public void resetPossibleRemoves() {
+ usesToRemoveOffsetTmp.clear();
+ }
+
+ public void confirmResultStringAt(int offset) {
+ if (offset == 0 || this.getUsedRangeOffset(offset) == null) {
+ confirmResultString();
+ return;
+ }
+ int currentStart = this.startOffset;
+ this.startOffset = this.getUsedRangeOffset(offset).getEnd();
+ savedHeadingWhitespaceOffset.add(this.getUsedRangeOffset(offset).getStart());
+ // remove new line because of insert on existing place
+ this.resultString.deleteCharAt(0);
+ confirmResultString();
+ this.startOffset = currentStart;
+ }
+
+ public void confirmResultString() {
+ if (resultString.length() > 0) {
+ for (Integer offset : usesToRemoveOffsetTmp) {
+ addToRemoveMap(offset);
+ }
+ if (appendUses && shouldAppendLineBefore()) {
+ editList.replace(getStartOffset(), 0, NEW_LINE, false, 0);
+ followingWhitespaceReplaceOffset.add(getStartOffset());
+ }
+ editList.replace(getStartOffset(), 0, resultString.toString(), false, 0);
+ if (appendUses && wrapInNewLines) {
+ editList.replace(getStartOffset(), 0, NEW_LINE, false, 0);
+ wrapInNewLines = false;
+ }
+ resultString = new StringBuilder();
Review Comment:
The same as above.
##########
php/php.editor/src/org/netbeans/modules/php/editor/actions/FixUsesPerformer.java:
##########
@@ -105,36 +110,56 @@ public FixUsesPerformer(
this.options = options;
}
- public void perform() {
+ private void perform(boolean append) {
final Document document = parserResult.getSnapshot().getSource().getDocument(false);
if (document instanceof BaseDocument) {
baseDocument = (BaseDocument) document;
editList = new EditList(baseDocument);
namespaceScope = ModelUtils.getNamespaceScope(parserResult.getModel().getFileScope(), importData.caretPosition);
assert namespaceScope != null;
- processSelections();
+ processSelections(append);
editList.apply();
}
}
+ public void perform()
+ {
+ perform(false);
+ }
+
+ public void performAppend()
+ {
Review Comment:
Formatting
##########
php/php.editor/src/org/netbeans/modules/php/editor/actions/FixUsesPerformer.java:
##########
@@ -595,23 +666,78 @@ private String createStringForMultipleUse(List<UsePart> useParts, String indentS
}
insertString.append(usePart.getTextPart());
}
- if (!useParts.isEmpty()) {
- insertString.append(SEMICOLON);
+ insertString.append(SEMICOLON);
+ if (usesInsertStringHelper.isAppendUses()) {
+ if (hasChanges) {
+ usesInsertStringHelper.applyDirectChangeAt(firstElementOffset, insertString.toString());
+ } else {
+ usesInsertStringHelper.resetPossibleRemoves();
+ usesInsertStringHelper.moveStartOffsetToTheEnd();
+ }
+ } else {
+ usesInsertStringHelper.appendToResultString(insertString.toString());
Review Comment:
The same as above.
##########
php/php.editor/src/org/netbeans/modules/php/editor/actions/FixUsesPerformer.java:
##########
@@ -562,19 +597,55 @@ private void processGroupedUseParts(StringBuilder insertString, String indentStr
insertString.append(indentString).append(groupUsePart.getTextPart().substring(prefixLength));
}
insertString.append(NEW_LINE).append(CURLY_CLOSE).append(SEMICOLON);
+ if (usesInsertStringHelper.isAppendUses()) {
+ if (hasChanges) {
+ usesInsertStringHelper.applyDirectChangeAt(firstElementOffset, insertString.toString());
+ } else {
+ usesInsertStringHelper.resetPossibleRemoves();
+ usesInsertStringHelper.moveStartOffsetToTheEnd();
+ }
+ } else {
+ usesInsertStringHelper.appendToResultString(insertString.toString());
+ }
groupedUseParts.clear();
}
- private String createStringForMultipleUse(List<UsePart> useParts, String indentString) {
+ private void createStringForMultipleUse(List<UsePart> useParts, String indentString, final UsesInsertStringHelper usesInsertStringHelper) {
+ if (useParts.isEmpty()) {
+ return;
+ }
StringBuilder insertString = new StringBuilder();
UsePart.Type lastUsePartType = null;
+ boolean hasChanges = false;
+ int firstElementOffset = 0;
for (Iterator<UsePart> it = useParts.iterator(); it.hasNext(); ) {
UsePart usePart = it.next();
+ usesInsertStringHelper.addToPossibleRemoveMap(usePart.getOffset());
+ usesInsertStringHelper.moveEndOffset(usePart.getOffset());
+ if (usePart.getOffset() == 0) {
+ hasChanges = true;
+ } else {
+ firstElementOffset = usePart.getOffset() == 0 || firstElementOffset != 0 && firstElementOffset < usePart.getOffset()
+ ? firstElementOffset : usePart.getOffset();
+ }
if (lastUsePartType != null) {
if (lastUsePartType == usePart.getType()) {
insertString.append(COMMA).append(NEW_LINE).append(indentString);
} else {
insertString.append(SEMICOLON);
+ if (usesInsertStringHelper.isAppendUses()) {
+ if (hasChanges) {
+ usesInsertStringHelper.applyDirectChangeAt(firstElementOffset, insertString.toString());
+ } else {
+ usesInsertStringHelper.resetPossibleRemoves();
+ usesInsertStringHelper.moveStartOffsetToTheEnd();
+ firstElementOffset = 0;
+ }
+ } else {
+ usesInsertStringHelper.appendToResultString(insertString.toString());
+ }
Review Comment:
Maybe, we should be able to introduce the method because there is a similar code above.
##########
php/php.editor/src/org/netbeans/modules/php/editor/actions/FixUsesPerformer.java:
##########
@@ -562,19 +597,55 @@ private void processGroupedUseParts(StringBuilder insertString, String indentStr
insertString.append(indentString).append(groupUsePart.getTextPart().substring(prefixLength));
}
insertString.append(NEW_LINE).append(CURLY_CLOSE).append(SEMICOLON);
+ if (usesInsertStringHelper.isAppendUses()) {
+ if (hasChanges) {
+ usesInsertStringHelper.applyDirectChangeAt(firstElementOffset, insertString.toString());
+ } else {
+ usesInsertStringHelper.resetPossibleRemoves();
+ usesInsertStringHelper.moveStartOffsetToTheEnd();
+ }
+ } else {
+ usesInsertStringHelper.appendToResultString(insertString.toString());
+ }
groupedUseParts.clear();
}
- private String createStringForMultipleUse(List<UsePart> useParts, String indentString) {
+ private void createStringForMultipleUse(List<UsePart> useParts, String indentString, final UsesInsertStringHelper usesInsertStringHelper) {
+ if (useParts.isEmpty()) {
+ return;
+ }
StringBuilder insertString = new StringBuilder();
UsePart.Type lastUsePartType = null;
+ boolean hasChanges = false;
+ int firstElementOffset = 0;
for (Iterator<UsePart> it = useParts.iterator(); it.hasNext(); ) {
UsePart usePart = it.next();
+ usesInsertStringHelper.addToPossibleRemoveMap(usePart.getOffset());
+ usesInsertStringHelper.moveEndOffset(usePart.getOffset());
+ if (usePart.getOffset() == 0) {
+ hasChanges = true;
+ } else {
+ firstElementOffset = usePart.getOffset() == 0 || firstElementOffset != 0 && firstElementOffset < usePart.getOffset()
+ ? firstElementOffset : usePart.getOffset();
+ }
if (lastUsePartType != null) {
if (lastUsePartType == usePart.getType()) {
insertString.append(COMMA).append(NEW_LINE).append(indentString);
} else {
insertString.append(SEMICOLON);
+ if (usesInsertStringHelper.isAppendUses()) {
+ if (hasChanges) {
+ usesInsertStringHelper.applyDirectChangeAt(firstElementOffset, insertString.toString());
+ } else {
+ usesInsertStringHelper.resetPossibleRemoves();
+ usesInsertStringHelper.moveStartOffsetToTheEnd();
+ firstElementOffset = 0;
+ }
+ } else {
+ usesInsertStringHelper.appendToResultString(insertString.toString());
+ }
+ hasChanges = false;
+ insertString = new StringBuilder();
Review Comment:
Please use`insertString.delete(0, insertString.length())` or 'insertString.setLength(0)'. Maybe, `new` is expensive.
##########
php/php.editor/src/org/netbeans/modules/php/editor/actions/FixUsesPerformer.java:
##########
@@ -631,13 +757,221 @@ private int getOffsetWithoutLeadingWhitespaces(final int startOffset) {
return result;
}
+ private int getOffsetWithoutFollowingWhitespaces(final int endOffset) {
+ int result = endOffset;
+ baseDocument.readLock();
+ try {
+ TokenSequence<PHPTokenId> ts = LexUtilities.getPHPTokenSequence(baseDocument, endOffset);
+ if (ts != null) {
+ ts.move(endOffset);
+ while (ts.moveNext() && ts.token().id().equals(PHPTokenId.WHITESPACE)) {
Review Comment:
Need check `ts.token() != null`, I suppose.
##########
php/php.editor/src/org/netbeans/modules/php/editor/actions/FixUsesPerformer.java:
##########
@@ -518,42 +539,56 @@ private void createStringForGroupUse(StringBuilder insertString, String indentSt
if (groupUsePrefix != null) {
if (lastGroupUsePrefix != null
&& !lastGroupUsePrefix.equals(groupUsePrefix)) {
- processGroupedUseParts(insertString, indentString, usePrefix, lastGroupUsePrefix, groupedUseParts);
+ processGroupedUseParts(indentString, usePrefix, lastGroupUsePrefix, groupedUseParts, usesInsertStringHelper);
}
lastGroupUsePrefix = groupUsePrefix;
- processNonGroupedUseParts(insertString, indentString, nonGroupedUseParts);
+ processNonGroupedUseParts(indentString, nonGroupedUseParts, usesInsertStringHelper);
groupedUseParts.add(usePart);
} else {
- processGroupedUseParts(insertString, indentString, usePrefix, lastGroupUsePrefix, groupedUseParts);
+ processGroupedUseParts(indentString, usePrefix, lastGroupUsePrefix, groupedUseParts, usesInsertStringHelper);
nonGroupedUseParts.add(usePart);
}
}
- processNonGroupedUseParts(insertString, indentString, nonGroupedUseParts);
- processGroupedUseParts(insertString, indentString, usePrefix, lastGroupUsePrefix, groupedUseParts);
+ processNonGroupedUseParts(indentString, nonGroupedUseParts, usesInsertStringHelper);
+ processGroupedUseParts(indentString, usePrefix, lastGroupUsePrefix, groupedUseParts, usesInsertStringHelper);
}
- private void processNonGroupedUseParts(StringBuilder insertString, String indentString, List<UsePart> nonGroupedUseParts) {
+ private void processNonGroupedUseParts(String indentString, List<UsePart> nonGroupedUseParts, final UsesInsertStringHelper usesInsertStringHelper) {
Review Comment:
I would add it to the first param. (Easy to see the diff.)
##########
php/php.editor/src/org/netbeans/modules/php/editor/actions/FixUsesPerformer.java:
##########
@@ -631,13 +757,221 @@ private int getOffsetWithoutLeadingWhitespaces(final int startOffset) {
return result;
}
+ private int getOffsetWithoutFollowingWhitespaces(final int endOffset) {
+ int result = endOffset;
+ baseDocument.readLock();
+ try {
+ TokenSequence<PHPTokenId> ts = LexUtilities.getPHPTokenSequence(baseDocument, endOffset);
+ if (ts != null) {
+ ts.move(endOffset);
+ while (ts.moveNext() && ts.token().id().equals(PHPTokenId.WHITESPACE)) {
+ result = ts.offset() + ts.token().length();
+ }
+ }
+ } finally {
+ baseDocument.readUnlock();
+ }
+ return result;
+ }
+
+ private class UsesInsertStringHelper {
+ private final Map<Integer, OffsetRange> usedRanges;
+ private final boolean appendUses;
+ private final boolean hasMultipleUses;
+ private StringBuilder resultString = new StringBuilder();
+ private Set<Integer> followingWhitespaceReplaceOffset = new HashSet<>();
+ private Set<Integer> savedHeadingWhitespaceOffset = new HashSet<>();
+ private int startOffset;
+ private int initialStartOffset;
+ private int endOffset;
+ private boolean wrapInNewLines = false;
+ private Map<Integer, Integer> usesToRemove = new HashMap<>();
+ private List<Integer> usesToRemoveOffsetTmp = new ArrayList<>();
+
+ UsesInsertStringHelper(Map<Integer, OffsetRange> usedRanges, boolean hasMultipleUses, boolean appendUses, int startOffset) {
+ this.usedRanges = usedRanges;
+ this.startOffset = startOffset;
+ this.initialStartOffset = startOffset;
+ this.hasMultipleUses = hasMultipleUses;
+ this.appendUses = appendUses;
+ }
+
+ public OffsetRange getUsedRangeOffset(int offset) {
+ return usedRanges.get(offset);
+ }
+
+ public void addToPossibleRemoveMap(int offset) {
+ if (usedRanges.get(offset) != null) {
+ usesToRemoveOffsetTmp.add(offset);
+ }
+ }
+
+ public Map<Integer, Integer> getUsesToRemove() {
+ return usesToRemove;
+ }
+
+ public int getStartOffset() {
+ return startOffset;
+ }
+
+ public int getInitialStartOffset() {
+ return initialStartOffset;
+ }
+
+ public void namespaceChange(int startOffset) {
+ this.startOffset = startOffset;
+ this.initialStartOffset = startOffset;
+ this.endOffset = startOffset;
+ }
+
+ public java.util.Set<Integer> getFollowingWhitespaceReplaceOffsets() {
+ return followingWhitespaceReplaceOffset;
+ }
+
+ public boolean needToSaveHeadingWhitespace(int offset) {
+ return savedHeadingWhitespaceOffset.contains(offset);
+ }
+
+ public boolean needToReplaceFollowingWhitespace(int offset) {
+ return followingWhitespaceReplaceOffset.contains(offset);
+ }
+
+ public void skipFollowingWhitespaceReplace(int offset) {
+ followingWhitespaceReplaceOffset.remove((Integer) offset);
+ }
+
+ public boolean moveEndOffset(int removedUseOffset) {
+ OffsetRange removed = this.getUsedRangeOffset(removedUseOffset);
+ if (removed != null) {
+ this.endOffset = removed.getEnd() > endOffset ? removed.getEnd() : endOffset;
+ return true;
+ }
+ return false;
+ }
+
+ public void moveStartOffsetToTheEnd() {
+ this.startOffset = this.endOffset;
+ }
+
+ public void applyDirectChangeAt(int offset, String insertString) {
+ if (offset == 0 || this.getUsedRangeOffset(offset) == null) {
+ applyDirectChange(insertString);
+ return;
+ }
+ int currentStart = this.startOffset;
+ this.startOffset = this.getUsedRangeOffset(offset).getEnd();
+ savedHeadingWhitespaceOffset.add(this.getUsedRangeOffset(offset).getStart());
+ // remove new line because of insert on existing place
+ insertString = insertString.substring(1);
+ applyDirectChange(insertString);
+ this.startOffset = currentStart;
+ }
+
+ public void applyDirectChange(String insertString)
+ {
Review Comment:
Formatting
##########
php/php.editor/src/org/netbeans/modules/php/editor/actions/FixUsesPerformer.java:
##########
@@ -631,13 +757,221 @@ private int getOffsetWithoutLeadingWhitespaces(final int startOffset) {
return result;
}
+ private int getOffsetWithoutFollowingWhitespaces(final int endOffset) {
+ int result = endOffset;
+ baseDocument.readLock();
+ try {
+ TokenSequence<PHPTokenId> ts = LexUtilities.getPHPTokenSequence(baseDocument, endOffset);
+ if (ts != null) {
+ ts.move(endOffset);
+ while (ts.moveNext() && ts.token().id().equals(PHPTokenId.WHITESPACE)) {
+ result = ts.offset() + ts.token().length();
+ }
+ }
+ } finally {
+ baseDocument.readUnlock();
+ }
+ return result;
+ }
+
+ private class UsesInsertStringHelper {
+ private final Map<Integer, OffsetRange> usedRanges;
+ private final boolean appendUses;
+ private final boolean hasMultipleUses;
+ private StringBuilder resultString = new StringBuilder();
+ private Set<Integer> followingWhitespaceReplaceOffset = new HashSet<>();
+ private Set<Integer> savedHeadingWhitespaceOffset = new HashSet<>();
+ private int startOffset;
+ private int initialStartOffset;
+ private int endOffset;
+ private boolean wrapInNewLines = false;
+ private Map<Integer, Integer> usesToRemove = new HashMap<>();
+ private List<Integer> usesToRemoveOffsetTmp = new ArrayList<>();
+
+ UsesInsertStringHelper(Map<Integer, OffsetRange> usedRanges, boolean hasMultipleUses, boolean appendUses, int startOffset) {
+ this.usedRanges = usedRanges;
+ this.startOffset = startOffset;
+ this.initialStartOffset = startOffset;
+ this.hasMultipleUses = hasMultipleUses;
+ this.appendUses = appendUses;
+ }
+
+ public OffsetRange getUsedRangeOffset(int offset) {
+ return usedRanges.get(offset);
+ }
+
+ public void addToPossibleRemoveMap(int offset) {
+ if (usedRanges.get(offset) != null) {
+ usesToRemoveOffsetTmp.add(offset);
+ }
+ }
+
+ public Map<Integer, Integer> getUsesToRemove() {
+ return usesToRemove;
+ }
+
+ public int getStartOffset() {
+ return startOffset;
+ }
+
+ public int getInitialStartOffset() {
+ return initialStartOffset;
+ }
+
+ public void namespaceChange(int startOffset) {
+ this.startOffset = startOffset;
+ this.initialStartOffset = startOffset;
+ this.endOffset = startOffset;
+ }
+
+ public java.util.Set<Integer> getFollowingWhitespaceReplaceOffsets() {
+ return followingWhitespaceReplaceOffset;
+ }
+
+ public boolean needToSaveHeadingWhitespace(int offset) {
+ return savedHeadingWhitespaceOffset.contains(offset);
+ }
+
+ public boolean needToReplaceFollowingWhitespace(int offset) {
+ return followingWhitespaceReplaceOffset.contains(offset);
+ }
+
+ public void skipFollowingWhitespaceReplace(int offset) {
+ followingWhitespaceReplaceOffset.remove((Integer) offset);
+ }
+
+ public boolean moveEndOffset(int removedUseOffset) {
+ OffsetRange removed = this.getUsedRangeOffset(removedUseOffset);
+ if (removed != null) {
+ this.endOffset = removed.getEnd() > endOffset ? removed.getEnd() : endOffset;
+ return true;
+ }
+ return false;
+ }
+
+ public void moveStartOffsetToTheEnd() {
+ this.startOffset = this.endOffset;
+ }
+
+ public void applyDirectChangeAt(int offset, String insertString) {
+ if (offset == 0 || this.getUsedRangeOffset(offset) == null) {
+ applyDirectChange(insertString);
+ return;
+ }
+ int currentStart = this.startOffset;
+ this.startOffset = this.getUsedRangeOffset(offset).getEnd();
+ savedHeadingWhitespaceOffset.add(this.getUsedRangeOffset(offset).getStart());
+ // remove new line because of insert on existing place
+ insertString = insertString.substring(1);
+ applyDirectChange(insertString);
Review Comment:
should not reassign it to the parameter.
`applyDirectChange(insertString.substring(1));`
##########
php/php.editor/src/org/netbeans/modules/php/editor/actions/FixUsesPerformer.java:
##########
@@ -562,19 +597,55 @@ private void processGroupedUseParts(StringBuilder insertString, String indentStr
insertString.append(indentString).append(groupUsePart.getTextPart().substring(prefixLength));
}
insertString.append(NEW_LINE).append(CURLY_CLOSE).append(SEMICOLON);
+ if (usesInsertStringHelper.isAppendUses()) {
+ if (hasChanges) {
+ usesInsertStringHelper.applyDirectChangeAt(firstElementOffset, insertString.toString());
+ } else {
+ usesInsertStringHelper.resetPossibleRemoves();
+ usesInsertStringHelper.moveStartOffsetToTheEnd();
+ }
+ } else {
+ usesInsertStringHelper.appendToResultString(insertString.toString());
+ }
groupedUseParts.clear();
}
- private String createStringForMultipleUse(List<UsePart> useParts, String indentString) {
+ private void createStringForMultipleUse(List<UsePart> useParts, String indentString, final UsesInsertStringHelper usesInsertStringHelper) {
+ if (useParts.isEmpty()) {
+ return;
+ }
StringBuilder insertString = new StringBuilder();
UsePart.Type lastUsePartType = null;
+ boolean hasChanges = false;
+ int firstElementOffset = 0;
for (Iterator<UsePart> it = useParts.iterator(); it.hasNext(); ) {
UsePart usePart = it.next();
+ usesInsertStringHelper.addToPossibleRemoveMap(usePart.getOffset());
+ usesInsertStringHelper.moveEndOffset(usePart.getOffset());
+ if (usePart.getOffset() == 0) {
+ hasChanges = true;
+ } else {
+ firstElementOffset = usePart.getOffset() == 0 || firstElementOffset != 0 && firstElementOffset < usePart.getOffset()
Review Comment:
Please add `()` to prevent misunderstanding.
##########
php/php.editor/src/org/netbeans/modules/php/editor/actions/FixUsesPerformer.java:
##########
@@ -631,13 +757,221 @@ private int getOffsetWithoutLeadingWhitespaces(final int startOffset) {
return result;
}
+ private int getOffsetWithoutFollowingWhitespaces(final int endOffset) {
+ int result = endOffset;
+ baseDocument.readLock();
+ try {
+ TokenSequence<PHPTokenId> ts = LexUtilities.getPHPTokenSequence(baseDocument, endOffset);
+ if (ts != null) {
+ ts.move(endOffset);
+ while (ts.moveNext() && ts.token().id().equals(PHPTokenId.WHITESPACE)) {
+ result = ts.offset() + ts.token().length();
+ }
+ }
+ } finally {
+ baseDocument.readUnlock();
+ }
+ return result;
+ }
+
+ private class UsesInsertStringHelper {
+ private final Map<Integer, OffsetRange> usedRanges;
+ private final boolean appendUses;
+ private final boolean hasMultipleUses;
+ private StringBuilder resultString = new StringBuilder();
+ private Set<Integer> followingWhitespaceReplaceOffset = new HashSet<>();
+ private Set<Integer> savedHeadingWhitespaceOffset = new HashSet<>();
+ private int startOffset;
+ private int initialStartOffset;
+ private int endOffset;
+ private boolean wrapInNewLines = false;
+ private Map<Integer, Integer> usesToRemove = new HashMap<>();
+ private List<Integer> usesToRemoveOffsetTmp = new ArrayList<>();
+
+ UsesInsertStringHelper(Map<Integer, OffsetRange> usedRanges, boolean hasMultipleUses, boolean appendUses, int startOffset) {
+ this.usedRanges = usedRanges;
+ this.startOffset = startOffset;
+ this.initialStartOffset = startOffset;
+ this.hasMultipleUses = hasMultipleUses;
+ this.appendUses = appendUses;
+ }
+
+ public OffsetRange getUsedRangeOffset(int offset) {
+ return usedRanges.get(offset);
+ }
+
+ public void addToPossibleRemoveMap(int offset) {
+ if (usedRanges.get(offset) != null) {
+ usesToRemoveOffsetTmp.add(offset);
+ }
+ }
+
+ public Map<Integer, Integer> getUsesToRemove() {
+ return usesToRemove;
Review Comment:
`return Collections.unmodifiableMap(usesToRemove);`
##########
php/php.editor/src/org/netbeans/modules/php/editor/actions/FixUsesPerformer.java:
##########
@@ -631,13 +757,221 @@ private int getOffsetWithoutLeadingWhitespaces(final int startOffset) {
return result;
}
+ private int getOffsetWithoutFollowingWhitespaces(final int endOffset) {
+ int result = endOffset;
+ baseDocument.readLock();
+ try {
+ TokenSequence<PHPTokenId> ts = LexUtilities.getPHPTokenSequence(baseDocument, endOffset);
+ if (ts != null) {
+ ts.move(endOffset);
+ while (ts.moveNext() && ts.token().id().equals(PHPTokenId.WHITESPACE)) {
+ result = ts.offset() + ts.token().length();
+ }
+ }
+ } finally {
+ baseDocument.readUnlock();
+ }
+ return result;
+ }
+
+ private class UsesInsertStringHelper {
+ private final Map<Integer, OffsetRange> usedRanges;
+ private final boolean appendUses;
+ private final boolean hasMultipleUses;
+ private StringBuilder resultString = new StringBuilder();
+ private Set<Integer> followingWhitespaceReplaceOffset = new HashSet<>();
+ private Set<Integer> savedHeadingWhitespaceOffset = new HashSet<>();
Review Comment:
`final`
##########
php/php.editor/src/org/netbeans/modules/php/editor/actions/FixUsesPerformer.java:
##########
@@ -631,13 +757,221 @@ private int getOffsetWithoutLeadingWhitespaces(final int startOffset) {
return result;
}
+ private int getOffsetWithoutFollowingWhitespaces(final int endOffset) {
+ int result = endOffset;
+ baseDocument.readLock();
+ try {
+ TokenSequence<PHPTokenId> ts = LexUtilities.getPHPTokenSequence(baseDocument, endOffset);
+ if (ts != null) {
+ ts.move(endOffset);
+ while (ts.moveNext() && ts.token().id().equals(PHPTokenId.WHITESPACE)) {
+ result = ts.offset() + ts.token().length();
+ }
+ }
+ } finally {
+ baseDocument.readUnlock();
+ }
+ return result;
+ }
+
+ private class UsesInsertStringHelper {
+ private final Map<Integer, OffsetRange> usedRanges;
+ private final boolean appendUses;
+ private final boolean hasMultipleUses;
+ private StringBuilder resultString = new StringBuilder();
+ private Set<Integer> followingWhitespaceReplaceOffset = new HashSet<>();
+ private Set<Integer> savedHeadingWhitespaceOffset = new HashSet<>();
+ private int startOffset;
+ private int initialStartOffset;
+ private int endOffset;
+ private boolean wrapInNewLines = false;
+ private Map<Integer, Integer> usesToRemove = new HashMap<>();
+ private List<Integer> usesToRemoveOffsetTmp = new ArrayList<>();
+
+ UsesInsertStringHelper(Map<Integer, OffsetRange> usedRanges, boolean hasMultipleUses, boolean appendUses, int startOffset) {
+ this.usedRanges = usedRanges;
+ this.startOffset = startOffset;
+ this.initialStartOffset = startOffset;
+ this.hasMultipleUses = hasMultipleUses;
+ this.appendUses = appendUses;
+ }
+
+ public OffsetRange getUsedRangeOffset(int offset) {
+ return usedRanges.get(offset);
+ }
+
+ public void addToPossibleRemoveMap(int offset) {
+ if (usedRanges.get(offset) != null) {
+ usesToRemoveOffsetTmp.add(offset);
+ }
+ }
+
+ public Map<Integer, Integer> getUsesToRemove() {
+ return usesToRemove;
+ }
+
+ public int getStartOffset() {
+ return startOffset;
+ }
+
+ public int getInitialStartOffset() {
+ return initialStartOffset;
+ }
+
+ public void namespaceChange(int startOffset) {
+ this.startOffset = startOffset;
+ this.initialStartOffset = startOffset;
+ this.endOffset = startOffset;
+ }
+
+ public java.util.Set<Integer> getFollowingWhitespaceReplaceOffsets() {
+ return followingWhitespaceReplaceOffset;
Review Comment:
`return Collections.unmodifiableSet(followingWhitespaceReplaceOffset);`
##########
php/php.editor/src/org/netbeans/modules/php/editor/actions/FixUsesPerformer.java:
##########
@@ -631,13 +757,221 @@ private int getOffsetWithoutLeadingWhitespaces(final int startOffset) {
return result;
}
+ private int getOffsetWithoutFollowingWhitespaces(final int endOffset) {
+ int result = endOffset;
+ baseDocument.readLock();
+ try {
+ TokenSequence<PHPTokenId> ts = LexUtilities.getPHPTokenSequence(baseDocument, endOffset);
+ if (ts != null) {
+ ts.move(endOffset);
+ while (ts.moveNext() && ts.token().id().equals(PHPTokenId.WHITESPACE)) {
+ result = ts.offset() + ts.token().length();
+ }
+ }
+ } finally {
+ baseDocument.readUnlock();
+ }
+ return result;
+ }
+
+ private class UsesInsertStringHelper {
+ private final Map<Integer, OffsetRange> usedRanges;
+ private final boolean appendUses;
+ private final boolean hasMultipleUses;
+ private StringBuilder resultString = new StringBuilder();
+ private Set<Integer> followingWhitespaceReplaceOffset = new HashSet<>();
+ private Set<Integer> savedHeadingWhitespaceOffset = new HashSet<>();
+ private int startOffset;
+ private int initialStartOffset;
+ private int endOffset;
+ private boolean wrapInNewLines = false;
+ private Map<Integer, Integer> usesToRemove = new HashMap<>();
+ private List<Integer> usesToRemoveOffsetTmp = new ArrayList<>();
+
+ UsesInsertStringHelper(Map<Integer, OffsetRange> usedRanges, boolean hasMultipleUses, boolean appendUses, int startOffset) {
+ this.usedRanges = usedRanges;
+ this.startOffset = startOffset;
+ this.initialStartOffset = startOffset;
+ this.hasMultipleUses = hasMultipleUses;
+ this.appendUses = appendUses;
+ }
+
+ public OffsetRange getUsedRangeOffset(int offset) {
+ return usedRanges.get(offset);
+ }
+
+ public void addToPossibleRemoveMap(int offset) {
+ if (usedRanges.get(offset) != null) {
+ usesToRemoveOffsetTmp.add(offset);
+ }
+ }
+
+ public Map<Integer, Integer> getUsesToRemove() {
+ return usesToRemove;
+ }
+
+ public int getStartOffset() {
+ return startOffset;
+ }
+
+ public int getInitialStartOffset() {
+ return initialStartOffset;
+ }
+
+ public void namespaceChange(int startOffset) {
+ this.startOffset = startOffset;
+ this.initialStartOffset = startOffset;
+ this.endOffset = startOffset;
+ }
+
+ public java.util.Set<Integer> getFollowingWhitespaceReplaceOffsets() {
+ return followingWhitespaceReplaceOffset;
+ }
+
+ public boolean needToSaveHeadingWhitespace(int offset) {
+ return savedHeadingWhitespaceOffset.contains(offset);
+ }
+
+ public boolean needToReplaceFollowingWhitespace(int offset) {
+ return followingWhitespaceReplaceOffset.contains(offset);
+ }
+
+ public void skipFollowingWhitespaceReplace(int offset) {
+ followingWhitespaceReplaceOffset.remove((Integer) offset);
+ }
+
+ public boolean moveEndOffset(int removedUseOffset) {
+ OffsetRange removed = this.getUsedRangeOffset(removedUseOffset);
+ if (removed != null) {
+ this.endOffset = removed.getEnd() > endOffset ? removed.getEnd() : endOffset;
+ return true;
+ }
+ return false;
+ }
+
+ public void moveStartOffsetToTheEnd() {
+ this.startOffset = this.endOffset;
+ }
+
+ public void applyDirectChangeAt(int offset, String insertString) {
+ if (offset == 0 || this.getUsedRangeOffset(offset) == null) {
+ applyDirectChange(insertString);
+ return;
+ }
+ int currentStart = this.startOffset;
+ this.startOffset = this.getUsedRangeOffset(offset).getEnd();
+ savedHeadingWhitespaceOffset.add(this.getUsedRangeOffset(offset).getStart());
+ // remove new line because of insert on existing place
+ insertString = insertString.substring(1);
+ applyDirectChange(insertString);
+ this.startOffset = currentStart;
+ }
+
+ public void applyDirectChange(String insertString)
+ {
+ for (Integer offset : usesToRemoveOffsetTmp) {
+ addToRemoveMap(offset);
+ }
+
+ if (shouldAppendLineBefore()) {
+ editList.replace(getStartOffset(), 0, NEW_LINE, false, 0);
+ followingWhitespaceReplaceOffset.add(getStartOffset());
+ editList.replace(getStartOffset(), 0, insertString, false, 0);
+ } else {
+ editList.replace(getStartOffset(), 0, insertString, false, 0);
+ }
+
+ if (wrapInNewLines) {
+ editList.replace(getStartOffset(), 0, NEW_LINE, false, 0);
+ wrapInNewLines = false;
+ }
+ }
+
+ public void setWrapInNewLinesOnAppend(boolean value) {
+ wrapInNewLines = value;
+ }
+
+ public void resetPossibleRemoves() {
+ usesToRemoveOffsetTmp.clear();
+ }
+
+ public void confirmResultStringAt(int offset) {
+ if (offset == 0 || this.getUsedRangeOffset(offset) == null) {
+ confirmResultString();
+ return;
+ }
+ int currentStart = this.startOffset;
+ this.startOffset = this.getUsedRangeOffset(offset).getEnd();
+ savedHeadingWhitespaceOffset.add(this.getUsedRangeOffset(offset).getStart());
+ // remove new line because of insert on existing place
+ this.resultString.deleteCharAt(0);
+ confirmResultString();
+ this.startOffset = currentStart;
+ }
+
+ public void confirmResultString() {
+ if (resultString.length() > 0) {
+ for (Integer offset : usesToRemoveOffsetTmp) {
+ addToRemoveMap(offset);
+ }
+ if (appendUses && shouldAppendLineBefore()) {
+ editList.replace(getStartOffset(), 0, NEW_LINE, false, 0);
+ followingWhitespaceReplaceOffset.add(getStartOffset());
+ }
+ editList.replace(getStartOffset(), 0, resultString.toString(), false, 0);
+ if (appendUses && wrapInNewLines) {
+ editList.replace(getStartOffset(), 0, NEW_LINE, false, 0);
+ wrapInNewLines = false;
+ }
+ resultString = new StringBuilder();
+ }
+ }
+
+ public void resetResultString() {
+ resetPossibleRemoves();
+ resultString = new StringBuilder();
Review Comment:
The same as above.
##########
php/php.editor/src/org/netbeans/modules/php/editor/actions/FixUsesPerformer.java:
##########
@@ -631,13 +757,221 @@ private int getOffsetWithoutLeadingWhitespaces(final int startOffset) {
return result;
}
+ private int getOffsetWithoutFollowingWhitespaces(final int endOffset) {
+ int result = endOffset;
+ baseDocument.readLock();
+ try {
+ TokenSequence<PHPTokenId> ts = LexUtilities.getPHPTokenSequence(baseDocument, endOffset);
+ if (ts != null) {
+ ts.move(endOffset);
+ while (ts.moveNext() && ts.token().id().equals(PHPTokenId.WHITESPACE)) {
+ result = ts.offset() + ts.token().length();
+ }
+ }
+ } finally {
+ baseDocument.readUnlock();
+ }
+ return result;
+ }
+
+ private class UsesInsertStringHelper {
+ private final Map<Integer, OffsetRange> usedRanges;
+ private final boolean appendUses;
+ private final boolean hasMultipleUses;
+ private StringBuilder resultString = new StringBuilder();
+ private Set<Integer> followingWhitespaceReplaceOffset = new HashSet<>();
+ private Set<Integer> savedHeadingWhitespaceOffset = new HashSet<>();
+ private int startOffset;
+ private int initialStartOffset;
+ private int endOffset;
+ private boolean wrapInNewLines = false;
+ private Map<Integer, Integer> usesToRemove = new HashMap<>();
+ private List<Integer> usesToRemoveOffsetTmp = new ArrayList<>();
+
+ UsesInsertStringHelper(Map<Integer, OffsetRange> usedRanges, boolean hasMultipleUses, boolean appendUses, int startOffset) {
+ this.usedRanges = usedRanges;
+ this.startOffset = startOffset;
+ this.initialStartOffset = startOffset;
+ this.hasMultipleUses = hasMultipleUses;
+ this.appendUses = appendUses;
+ }
+
+ public OffsetRange getUsedRangeOffset(int offset) {
+ return usedRanges.get(offset);
+ }
+
+ public void addToPossibleRemoveMap(int offset) {
+ if (usedRanges.get(offset) != null) {
+ usesToRemoveOffsetTmp.add(offset);
+ }
+ }
+
+ public Map<Integer, Integer> getUsesToRemove() {
+ return usesToRemove;
+ }
+
+ public int getStartOffset() {
+ return startOffset;
+ }
+
+ public int getInitialStartOffset() {
+ return initialStartOffset;
+ }
+
+ public void namespaceChange(int startOffset) {
+ this.startOffset = startOffset;
+ this.initialStartOffset = startOffset;
+ this.endOffset = startOffset;
+ }
+
+ public java.util.Set<Integer> getFollowingWhitespaceReplaceOffsets() {
Review Comment:
`public Set<Integer> getFollowingWhitespaceReplaceOffsets() {`
--
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
[GitHub] [netbeans] junichi11 commented on pull request #5900: [PHP] FixUsesPerformer and AddUseImportSuggestion improvement
Posted by "junichi11 (via GitHub)" <gi...@apache.org>.
junichi11 commented on PR #5900:
URL: https://github.com/apache/netbeans/pull/5900#issuecomment-1543305671
Well... Honestly, I don't want to merge this as it is now. I don't think this is maintainable. I'm trying to improve this, but I need a lot of time to do it... So, please wait a while. (I'm not sure whether I can finish it. I can't promise anything.)
--
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
[GitHub] [netbeans] junichi11 commented on pull request #5900: [PHP] FixUsesPerformer and AddUseImportSuggestion improvement
Posted by "junichi11 (via GitHub)" <gi...@apache.org>.
junichi11 commented on PR #5900:
URL: https://github.com/apache/netbeans/pull/5900#issuecomment-1534235765
> to maintain same logic in one place
I agree with this but I'm not sure whether we should use this implementation...
--
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
[GitHub] [netbeans] rossluk commented on pull request #5900: [PHP] FixUsesPerformer and AddUseImportSuggestion improvement
Posted by "rossluk (via GitHub)" <gi...@apache.org>.
rossluk commented on PR #5900:
URL: https://github.com/apache/netbeans/pull/5900#issuecomment-1546604991
@junichi11 Let me know if I can help. I've spent much time on it and don't think it's easy to make it more understandable (at least without crucial changes in FixUsesPerformer original code). It was much clearer and straightforward before PSR12 introduction, because main issue is empty line handling (especially on regroups), which are pretty complicated task. I could try to remove all the single import logic related to PSR12 just to reduce maintenance cost (it still should work good enough if USES were formatted properly by user before an import). NetBeans for Java has single import functionality, PhpStorm as well, so would be nice to implement it somehow, especially when AddUseImportSuggestion is broken and FastImport does obsolete things by importing files in times when we have composer autoloading.
--
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
[GitHub] [netbeans] rossluk closed pull request #5900: [PHP] FixUsesPerformer and AddUseImportSuggestion improvement
Posted by "rossluk (via GitHub)" <gi...@apache.org>.
rossluk closed pull request #5900: [PHP] FixUsesPerformer and AddUseImportSuggestion improvement
URL: https://github.com/apache/netbeans/pull/5900
--
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
[GitHub] [netbeans] tmysik commented on pull request #5900: [PHP] FixUsesPerformer and AddUseImportSuggestion improvement
Posted by "tmysik (via GitHub)" <gi...@apache.org>.
tmysik commented on PR #5900:
URL: https://github.com/apache/netbeans/pull/5900#issuecomment-1547774633
@rossluk @junichi11
> Well... Honestly, I don't want to merge this as it is now. I don't think this is maintainable.
Andrei, since Junichi is, in fact, the maintainer of the PHP support in NetBeans, I am afraid that if he does not like the current state of this change, we cannot merge it, sorry. But continue reading, please:
> I'm trying to improve this, but I need a lot of time to do it... So, please wait a while.
It would be great if any kind of cooperation could be created here. Junichi, let us know if we can help somehow. For example, would any call (e.g. Google Meet) be useful here? Maybe, discussing face to face the biggest issues of this change could help and some way how to improve the current state could be created and agreed on?
Thank you both.
--
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
[GitHub] [netbeans] rossluk commented on pull request #5900: [PHP] FixUsesPerformer and AddUseImportSuggestion improvement
Posted by "rossluk (via GitHub)" <gi...@apache.org>.
rossluk commented on PR #5900:
URL: https://github.com/apache/netbeans/pull/5900#issuecomment-1534516713
@junichi11 @junichi11 Thank you guys for your work, I'll make all the changes as soon as I have some time. About maintainability: complexity is increasing, but it make sense if we need such a logic like an import of singular uses (propably as junichi11 said it should be another implementation). If the logic is not necessary I suppose it would be better without changes, but in such case we also should remove AddUseImportSuggestion.
--
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
[GitHub] [netbeans] tmysik commented on a diff in pull request #5900: [PHP] FixUsesPerformer and AddUseImportSuggestion improvement
Posted by "tmysik (via GitHub)" <gi...@apache.org>.
tmysik commented on code in PR #5900:
URL: https://github.com/apache/netbeans/pull/5900#discussion_r1187193092
##########
php/php.editor/src/org/netbeans/modules/php/editor/actions/FixUsesPerformer.java:
##########
@@ -631,13 +757,221 @@ private int getOffsetWithoutLeadingWhitespaces(final int startOffset) {
return result;
}
+ private int getOffsetWithoutFollowingWhitespaces(final int endOffset) {
+ int result = endOffset;
+ baseDocument.readLock();
+ try {
+ TokenSequence<PHPTokenId> ts = LexUtilities.getPHPTokenSequence(baseDocument, endOffset);
+ if (ts != null) {
+ ts.move(endOffset);
+ while (ts.moveNext() && ts.token().id().equals(PHPTokenId.WHITESPACE)) {
+ result = ts.offset() + ts.token().length();
+ }
+ }
+ } finally {
+ baseDocument.readUnlock();
+ }
+ return result;
+ }
+
+ private class UsesInsertStringHelper {
Review Comment:
The non-static inner class cannot exist without it's outer class. Therefore, it is preferred to make inner classes static, if possible.
--
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
[GitHub] [netbeans] junichi11 commented on a diff in pull request #5900: [PHP] FixUsesPerformer and AddUseImportSuggestion improvement
Posted by "junichi11 (via GitHub)" <gi...@apache.org>.
junichi11 commented on code in PR #5900:
URL: https://github.com/apache/netbeans/pull/5900#discussion_r1186755753
##########
php/php.editor/src/org/netbeans/modules/php/editor/actions/FixUsesPerformer.java:
##########
@@ -631,13 +757,221 @@ private int getOffsetWithoutLeadingWhitespaces(final int startOffset) {
return result;
}
+ private int getOffsetWithoutFollowingWhitespaces(final int endOffset) {
+ int result = endOffset;
+ baseDocument.readLock();
+ try {
+ TokenSequence<PHPTokenId> ts = LexUtilities.getPHPTokenSequence(baseDocument, endOffset);
+ if (ts != null) {
+ ts.move(endOffset);
+ while (ts.moveNext() && ts.token().id().equals(PHPTokenId.WHITESPACE)) {
Review Comment:
Ah, OK.
--
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
[GitHub] [netbeans] rossluk commented on pull request #5900: [PHP] FixUsesPerformer and AddUseImportSuggestion improvement
Posted by "rossluk (via GitHub)" <gi...@apache.org>.
rossluk commented on PR #5900:
URL: https://github.com/apache/netbeans/pull/5900#issuecomment-1589978735
@junichi11 Thanks for a try and for your time. Hope it would be implemented somehow in the future. Should I close the request?
> I think we can add a single use name with the current implementation. (We can replace the whole use names instead of appending a single use name to existing use names.)
I suggest to left it intact until there is no proper logic for a single use import. For Java it works as a single import, in PhpStorm as well, and it's good to have expected/consistent behavior when people are switching between tools. For example in my case I often put uses grouped by vendor packages, then uses of our private vendor packages and then current project uses, so with a single use import after manual formatting it will go in proper place, but with whole replace it will break the formatting. Fix imports already can do it this way so it is unnecessary.
--
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
[GitHub] [netbeans] junichi11 commented on a diff in pull request #5900: [PHP] FixUsesPerformer and AddUseImportSuggestion improvement
Posted by "junichi11 (via GitHub)" <gi...@apache.org>.
junichi11 commented on code in PR #5900:
URL: https://github.com/apache/netbeans/pull/5900#discussion_r1188078689
##########
php/php.editor/src/org/netbeans/modules/php/editor/actions/FixUsesPerformer.java:
##########
@@ -631,13 +757,221 @@ private int getOffsetWithoutLeadingWhitespaces(final int startOffset) {
return result;
}
+ private int getOffsetWithoutFollowingWhitespaces(final int endOffset) {
+ int result = endOffset;
+ baseDocument.readLock();
+ try {
+ TokenSequence<PHPTokenId> ts = LexUtilities.getPHPTokenSequence(baseDocument, endOffset);
+ if (ts != null) {
+ ts.move(endOffset);
+ while (ts.moveNext() && ts.token().id().equals(PHPTokenId.WHITESPACE)) {
+ result = ts.offset() + ts.token().length();
+ }
+ }
+ } finally {
+ baseDocument.readUnlock();
+ }
+ return result;
+ }
+
+ private class UsesInsertStringHelper {
Review Comment:
I agree with @tmysik .
The member class should be `static` if it does not need access to the enclosing class instance.
If you are interested in it, I suggest trying to read Effective Java(Item 24: Favor static member classes over nonstatic).
--
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
[GitHub] [netbeans] junichi11 commented on pull request #5900: [PHP] FixUsesPerformer and AddUseImportSuggestion improvement
Posted by "junichi11 (via GitHub)" <gi...@apache.org>.
junichi11 commented on PR #5900:
URL: https://github.com/apache/netbeans/pull/5900#issuecomment-1548807620
I'll let you know if I need your help. Thanks!
--
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
[GitHub] [netbeans] junichi11 commented on pull request #5900: [PHP] FixUsesPerformer and AddUseImportSuggestion improvement
Posted by "junichi11 (via GitHub)" <gi...@apache.org>.
junichi11 commented on PR #5900:
URL: https://github.com/apache/netbeans/pull/5900#issuecomment-1576078023
@rossluk I'm sorry I'm late.
First of all, thank you for your work. However, I can't approve this, sorry. This implementation is so complicated. As I wrote above, I don't think this is maintainable... (I can improve some parts only a bit but not maintainable.) It would be more complicated if we add more options in the future.
I think we can add a single use name with the current implementation. (We can replace the whole use names instead of appending a single use name to existing use names.)
--
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
[GitHub] [netbeans] rossluk commented on a diff in pull request #5900: [PHP] FixUsesPerformer and AddUseImportSuggestion improvement
Posted by "rossluk (via GitHub)" <gi...@apache.org>.
rossluk commented on code in PR #5900:
URL: https://github.com/apache/netbeans/pull/5900#discussion_r1186823070
##########
php/php.editor/src/org/netbeans/modules/php/editor/actions/FixUsesPerformer.java:
##########
@@ -425,28 +432,44 @@ private String createInsertString(final List<UsePart> useParts) {
} else {
Collections.sort(useParts);
}
- if (!useParts.isEmpty()) {
- insertString.append(NEW_LINE);
- }
+
String indentString = null;
if (options.preferGroupUses()
|| options.preferMultipleUseStatementsCombined()) {
CodeStyle codeStyle = CodeStyle.get(baseDocument);
indentString = IndentUtils.createIndentString(codeStyle.getIndentSize(), codeStyle.expandTabToSpaces(), codeStyle.getTabSize());
}
- if (options.preferGroupUses()
+ usesInsertStringHelper.setWrapInNewLinesOnAppend(useParts.size() == 1);
+ if (options.preferGroupUses() && usesInsertStringHelper.canGroupUses()
&& options.getPhpVersion().compareTo(PhpVersion.PHP_70) >= 0) {
- insertString.append(createStringForGroupUse(useParts, indentString));
+ createStringForGroupUse(useParts, indentString, usesInsertStringHelper);
} else if (options.preferMultipleUseStatementsCombined()) {
- insertString.append(createStringForMultipleUse(useParts, indentString));
+ createStringForMultipleUse(useParts, indentString, usesInsertStringHelper);
} else {
- insertString.append(createStringForCommonUse(useParts));
+ createStringForCommonUse(useParts, usesInsertStringHelper);
+ }
+
+ if (!usesInsertStringHelper.isAppendUses()) {
+ try {
+ String replaceString = baseDocument.getText(usesInsertStringHelper.getInitialStartOffset(), usesInsertStringHelper.getStartOffset() - usesInsertStringHelper.getInitialStartOffset());
+ if (replaceString.equals(usesInsertStringHelper.getResultString())) {
+ usesInsertStringHelper.resetResultString();
+ return;
+ }
+ } catch (BadLocationException ex) {
+ Exceptions.printStackTrace(ex);
Review Comment:
Done
--
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
[GitHub] [netbeans] tmysik commented on a diff in pull request #5900: [PHP] FixUsesPerformer and AddUseImportSuggestion improvement
Posted by "tmysik (via GitHub)" <gi...@apache.org>.
tmysik commented on code in PR #5900:
URL: https://github.com/apache/netbeans/pull/5900#discussion_r1184696397
##########
php/php.editor/src/org/netbeans/modules/php/editor/actions/FixUsesPerformer.java:
##########
@@ -631,13 +757,221 @@ private int getOffsetWithoutLeadingWhitespaces(final int startOffset) {
return result;
}
+ private int getOffsetWithoutFollowingWhitespaces(final int endOffset) {
+ int result = endOffset;
+ baseDocument.readLock();
+ try {
+ TokenSequence<PHPTokenId> ts = LexUtilities.getPHPTokenSequence(baseDocument, endOffset);
+ if (ts != null) {
+ ts.move(endOffset);
+ while (ts.moveNext() && ts.token().id().equals(PHPTokenId.WHITESPACE)) {
+ result = ts.offset() + ts.token().length();
+ }
+ }
+ } finally {
+ baseDocument.readUnlock();
+ }
+ return result;
+ }
+
+ private class UsesInsertStringHelper {
+ private final Map<Integer, OffsetRange> usedRanges;
+ private final boolean appendUses;
+ private final boolean hasMultipleUses;
+ private StringBuilder resultString = new StringBuilder();
+ private Set<Integer> followingWhitespaceReplaceOffset = new HashSet<>();
+ private Set<Integer> savedHeadingWhitespaceOffset = new HashSet<>();
Review Comment:
Yes, please, make all the fields `final`, if possible (and call `.clear()` and similarly, if needed).
--
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
[GitHub] [netbeans] rossluk commented on a diff in pull request #5900: [PHP] FixUsesPerformer and AddUseImportSuggestion improvement
Posted by "rossluk (via GitHub)" <gi...@apache.org>.
rossluk commented on code in PR #5900:
URL: https://github.com/apache/netbeans/pull/5900#discussion_r1189024743
##########
php/php.editor/src/org/netbeans/modules/php/editor/actions/FixUsesPerformer.java:
##########
@@ -425,28 +432,45 @@ private String createInsertString(final List<UsePart> useParts) {
} else {
Collections.sort(useParts);
}
- if (!useParts.isEmpty()) {
- insertString.append(NEW_LINE);
- }
+
String indentString = null;
if (options.preferGroupUses()
|| options.preferMultipleUseStatementsCombined()) {
CodeStyle codeStyle = CodeStyle.get(baseDocument);
indentString = IndentUtils.createIndentString(codeStyle.getIndentSize(), codeStyle.expandTabToSpaces(), codeStyle.getTabSize());
}
- if (options.preferGroupUses()
+ usesInsertStringHelper.setWrapInNewLinesOnAppend(useParts.size() == 1);
+ if (options.preferGroupUses() && usesInsertStringHelper.canGroupUses()
&& options.getPhpVersion().compareTo(PhpVersion.PHP_70) >= 0) {
- insertString.append(createStringForGroupUse(useParts, indentString));
+ createStringForGroupUse(usesInsertStringHelper, useParts, indentString);
} else if (options.preferMultipleUseStatementsCombined()) {
- insertString.append(createStringForMultipleUse(useParts, indentString));
+ createStringForMultipleUse(usesInsertStringHelper, useParts, indentString);
} else {
- insertString.append(createStringForCommonUse(useParts));
+ createStringForCommonUse(usesInsertStringHelper, useParts);
+ }
+
+ if (!usesInsertStringHelper.isAppendUses()) {
+ try {
+ String replaceString = baseDocument.getText(usesInsertStringHelper.getInitialStartOffset(), usesInsertStringHelper.getStartOffset() - usesInsertStringHelper.getInitialStartOffset());
+ if (replaceString.equals(usesInsertStringHelper.getResultString())) {
+ usesInsertStringHelper.resetResultString();
+ return;
+ }
+ } catch (BadLocationException ex) {
+ Exceptions.printStackTrace(ex);
Review Comment:
Done
--
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
[GitHub] [netbeans] junichi11 commented on a diff in pull request #5900: [PHP] FixUsesPerformer and AddUseImportSuggestion improvement
Posted by "junichi11 (via GitHub)" <gi...@apache.org>.
junichi11 commented on code in PR #5900:
URL: https://github.com/apache/netbeans/pull/5900#discussion_r1186755714
##########
php/php.editor/src/org/netbeans/modules/php/editor/actions/FixUsesPerformer.java:
##########
@@ -425,28 +432,44 @@ private String createInsertString(final List<UsePart> useParts) {
} else {
Collections.sort(useParts);
}
- if (!useParts.isEmpty()) {
- insertString.append(NEW_LINE);
- }
+
String indentString = null;
if (options.preferGroupUses()
|| options.preferMultipleUseStatementsCombined()) {
CodeStyle codeStyle = CodeStyle.get(baseDocument);
indentString = IndentUtils.createIndentString(codeStyle.getIndentSize(), codeStyle.expandTabToSpaces(), codeStyle.getTabSize());
}
- if (options.preferGroupUses()
+ usesInsertStringHelper.setWrapInNewLinesOnAppend(useParts.size() == 1);
+ if (options.preferGroupUses() && usesInsertStringHelper.canGroupUses()
&& options.getPhpVersion().compareTo(PhpVersion.PHP_70) >= 0) {
- insertString.append(createStringForGroupUse(useParts, indentString));
+ createStringForGroupUse(useParts, indentString, usesInsertStringHelper);
} else if (options.preferMultipleUseStatementsCombined()) {
- insertString.append(createStringForMultipleUse(useParts, indentString));
+ createStringForMultipleUse(useParts, indentString, usesInsertStringHelper);
} else {
- insertString.append(createStringForCommonUse(useParts));
+ createStringForCommonUse(useParts, usesInsertStringHelper);
+ }
+
+ if (!usesInsertStringHelper.isAppendUses()) {
+ try {
+ String replaceString = baseDocument.getText(usesInsertStringHelper.getInitialStartOffset(), usesInsertStringHelper.getStartOffset() - usesInsertStringHelper.getInitialStartOffset());
+ if (replaceString.equals(usesInsertStringHelper.getResultString())) {
+ usesInsertStringHelper.resetResultString();
+ return;
+ }
+ } catch (BadLocationException ex) {
+ Exceptions.printStackTrace(ex);
Review Comment:
Please add the Logger, then leave helpful information as a log: e.g. `ex.offsetRequested()`.
--
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
[GitHub] [netbeans] tmysik commented on pull request #5900: [PHP] FixUsesPerformer and AddUseImportSuggestion improvement
Posted by "tmysik (via GitHub)" <gi...@apache.org>.
tmysik commented on PR #5900:
URL: https://github.com/apache/netbeans/pull/5900#issuecomment-1534302714
@rossluk
First of all - respect for this PR, it is definitely not trivial, and you did a lot of work, thanks for it!
@junichi11
Since this PR is very big, I am not sure whether we can review it properly; therefore, I would suggest this - could you, please:
- try to understand the big picture of this change and whether it makes sense to you;
- verify that this PR cannot break any other area; and
- review all the updated and new tests, whether they are correct?
I will try to do the same but frankly, my knowledge is quite limited, it is already a long time :sweat_smile:
In general, I am not against this change if all the tests are still passing and newly added tests make sense (and cover the change nicely). The question is, how maintainable the code will be - will it be better or worse in comparison to the current state? Based on the initial description, it should be better, I hope :grin:
Please, give me some time, so I can try to understand the change, at least a bit :smile:
Thank you both!
--
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
[GitHub] [netbeans] rossluk commented on pull request #5900: [PHP] FixUsesPerformer and AddUseImportSuggestion improvement
Posted by "rossluk (via GitHub)" <gi...@apache.org>.
rossluk commented on PR #5900:
URL: https://github.com/apache/netbeans/pull/5900#issuecomment-1534488403
> I looked at only changes of `FixUsesPerformer.java`. I don't understand what the changes are doing yet at the moment. (Large changes and complicated.)
If you add some unimported class to the file and put cursor on it you'll see the bulb with suggestion to add new "use" for the class (previously it didn't work properly for classes, but for constants) and when you click "Generate use NS/YourClass" it will add it to the "uses" (previously it wasn't respect the options and order and tried to add the value to the end of "uses" list, but now it even tries to regroup old uses due to options selected).
--
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
[GitHub] [netbeans] junichi11 commented on pull request #5900: [PHP] FixUsesPerformer and AddUseImportSuggestion improvement
Posted by "junichi11 (via GitHub)" <gi...@apache.org>.
junichi11 commented on PR #5900:
URL: https://github.com/apache/netbeans/pull/5900#issuecomment-1590309386
@rossluk Please submit it as a new issue with an example once. I'll try to look at it when I have time. (although I can't promise anything.)
If you are familiar with Java and NetBeans modules, I can also leave it to you against a BIG change. However, if not so, we have to take a lot of time to review. (We are welcome to PRs, but our time is limited.) We may overlook something.
(Actually, there are a bug and a performance problem as regression now.)
Thank you for your understanding.
--
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
[GitHub] [netbeans] junichi11 commented on a diff in pull request #5900: [PHP] FixUsesPerformer and AddUseImportSuggestion improvement
Posted by "junichi11 (via GitHub)" <gi...@apache.org>.
junichi11 commented on code in PR #5900:
URL: https://github.com/apache/netbeans/pull/5900#discussion_r1188057021
##########
php/php.editor/src/org/netbeans/modules/php/editor/actions/FixUsesPerformer.java:
##########
@@ -425,28 +432,45 @@ private String createInsertString(final List<UsePart> useParts) {
} else {
Collections.sort(useParts);
}
- if (!useParts.isEmpty()) {
- insertString.append(NEW_LINE);
- }
+
String indentString = null;
if (options.preferGroupUses()
|| options.preferMultipleUseStatementsCombined()) {
CodeStyle codeStyle = CodeStyle.get(baseDocument);
indentString = IndentUtils.createIndentString(codeStyle.getIndentSize(), codeStyle.expandTabToSpaces(), codeStyle.getTabSize());
}
- if (options.preferGroupUses()
+ usesInsertStringHelper.setWrapInNewLinesOnAppend(useParts.size() == 1);
+ if (options.preferGroupUses() && usesInsertStringHelper.canGroupUses()
&& options.getPhpVersion().compareTo(PhpVersion.PHP_70) >= 0) {
- insertString.append(createStringForGroupUse(useParts, indentString));
+ createStringForGroupUse(usesInsertStringHelper, useParts, indentString);
} else if (options.preferMultipleUseStatementsCombined()) {
- insertString.append(createStringForMultipleUse(useParts, indentString));
+ createStringForMultipleUse(usesInsertStringHelper, useParts, indentString);
} else {
- insertString.append(createStringForCommonUse(useParts));
+ createStringForCommonUse(usesInsertStringHelper, useParts);
+ }
+
+ if (!usesInsertStringHelper.isAppendUses()) {
+ try {
+ String replaceString = baseDocument.getText(usesInsertStringHelper.getInitialStartOffset(), usesInsertStringHelper.getStartOffset() - usesInsertStringHelper.getInitialStartOffset());
+ if (replaceString.equals(usesInsertStringHelper.getResultString())) {
+ usesInsertStringHelper.resetResultString();
+ return;
+ }
+ } catch (BadLocationException ex) {
+ Exceptions.printStackTrace(ex);
Review Comment:
We can remove `Exceptions.printStackTrace(ex);`.
--
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
[GitHub] [netbeans] rossluk commented on pull request #5900: [PHP] FixUsesPerformer and AddUseImportSuggestion improvement
Posted by "rossluk (via GitHub)" <gi...@apache.org>.
rossluk commented on PR #5900:
URL: https://github.com/apache/netbeans/pull/5900#issuecomment-1590656428
@junichi11 Understand, thanks.
--
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
[GitHub] [netbeans] tmysik commented on a diff in pull request #5900: [PHP] FixUsesPerformer and AddUseImportSuggestion improvement
Posted by "tmysik (via GitHub)" <gi...@apache.org>.
tmysik commented on code in PR #5900:
URL: https://github.com/apache/netbeans/pull/5900#discussion_r1184688675
##########
php/php.editor/src/org/netbeans/modules/php/editor/actions/FixUsesPerformer.java:
##########
@@ -105,36 +110,56 @@ public FixUsesPerformer(
this.options = options;
}
- public void perform() {
+ private void perform(boolean append) {
final Document document = parserResult.getSnapshot().getSource().getDocument(false);
if (document instanceof BaseDocument) {
baseDocument = (BaseDocument) document;
editList = new EditList(baseDocument);
namespaceScope = ModelUtils.getNamespaceScope(parserResult.getModel().getFileScope(), importData.caretPosition);
assert namespaceScope != null;
- processSelections();
+ processSelections(append);
editList.apply();
}
}
+ public void perform()
+ {
+ perform(false);
+ }
+
+ public void performAppend()
+ {
+ assert this.selections.size() == 1;
Review Comment:
```suggestion
assert selections.size() == 1 : "Expected size == 1 but got: " + selections.size();
```
--
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
[GitHub] [netbeans] tmysik commented on a diff in pull request #5900: [PHP] FixUsesPerformer and AddUseImportSuggestion improvement
Posted by "tmysik (via GitHub)" <gi...@apache.org>.
tmysik commented on code in PR #5900:
URL: https://github.com/apache/netbeans/pull/5900#discussion_r1184695512
##########
php/php.editor/src/org/netbeans/modules/php/editor/actions/FixUsesPerformer.java:
##########
@@ -631,13 +757,221 @@ private int getOffsetWithoutLeadingWhitespaces(final int startOffset) {
return result;
}
+ private int getOffsetWithoutFollowingWhitespaces(final int endOffset) {
+ int result = endOffset;
+ baseDocument.readLock();
+ try {
+ TokenSequence<PHPTokenId> ts = LexUtilities.getPHPTokenSequence(baseDocument, endOffset);
+ if (ts != null) {
+ ts.move(endOffset);
+ while (ts.moveNext() && ts.token().id().equals(PHPTokenId.WHITESPACE)) {
+ result = ts.offset() + ts.token().length();
+ }
+ }
+ } finally {
+ baseDocument.readUnlock();
+ }
+ return result;
+ }
+
+ private class UsesInsertStringHelper {
Review Comment:
Can the class be `static`?
--
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
[GitHub] [netbeans] rossluk commented on a diff in pull request #5900: [PHP] FixUsesPerformer and AddUseImportSuggestion improvement
Posted by "rossluk (via GitHub)" <gi...@apache.org>.
rossluk commented on code in PR #5900:
URL: https://github.com/apache/netbeans/pull/5900#discussion_r1184819431
##########
php/php.editor/src/org/netbeans/modules/php/editor/actions/FixUsesPerformer.java:
##########
@@ -631,13 +757,221 @@ private int getOffsetWithoutLeadingWhitespaces(final int startOffset) {
return result;
}
+ private int getOffsetWithoutFollowingWhitespaces(final int endOffset) {
+ int result = endOffset;
+ baseDocument.readLock();
+ try {
+ TokenSequence<PHPTokenId> ts = LexUtilities.getPHPTokenSequence(baseDocument, endOffset);
+ if (ts != null) {
+ ts.move(endOffset);
+ while (ts.moveNext() && ts.token().id().equals(PHPTokenId.WHITESPACE)) {
+ result = ts.offset() + ts.token().length();
+ }
+ }
+ } finally {
+ baseDocument.readUnlock();
+ }
+ return result;
+ }
+
+ private class UsesInsertStringHelper {
Review Comment:
It can, if I make editList static or put it directly to the constructor. Could you suggest what the advantages in such case? Sorry for probably silly question, but I am not very good in Java for now. Thanks.
--
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