Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[PHP] Multiple default namespace navigator fix, FixImports for the whole file #5681

Merged
merged 2 commits into from
Mar 22, 2023
Merged

[PHP] Multiple default namespace navigator fix, FixImports for the whole file #5681

merged 2 commits into from
Mar 22, 2023

Conversation

rossluk
Copy link
Contributor

@rossluk rossluk commented Mar 19, 2023

This PR fixes next issues:

  1. Incorrect navigator view when you have bracketed default namespace or multiple default namespaces in file:

-- before fix it shows classes without namespace, and USE from second default namespace is showed as first one
navigator_view

-- after fix
navigator_view_fix

  1. Incorrect fix imports when there are multiple default namespaces and some of them has imported USES:

-- initial state
getns_from_other

-- before fix, it gets all uses from all default namespaces and inserts it in the first one
get_ns_from_other_after
-- after fix
getns_from_other_fix

  1. File state change after each fix import when there is no changes

-- before fix before fix import action (see tab)
file_change

-- before fix after fix import action (see tab)
file_change_after

Also with the fixes I've changed Fix Imports logic. Previously when you use Fix Imports outside bracketed namespace it was changing first available namespace uses, but now if you in global scope it fixes all the file, see below:

-- initial state
Screen Shot 2023-03-19 at 15 40 04
-- after Fix Imports
Screen Shot 2023-03-19 at 13 08 23

To have Fix Imports working for the whole file I had to add scope to UsedNamesCollector's names and it also affected some tests, because for now it is important that DataItems from ImportDataCreator were paired with Selections, but previously Selection could be imported without real DataItem.

@rossluk rossluk changed the title [PHP] Multiple default namespace navigator fix, FixImports for whole file [PHP] Multiple default namespace navigator fix, FixImports for the whole file Mar 19, 2023
@junichi11 junichi11 added the PHP [ci] enable extra PHP tests (php/php.editor) label Mar 19, 2023
@junichi11
Copy link
Member

junichi11 commented Mar 19, 2023 •
edited
Loading

@rossluk This PR contains some fixes as one commit at the same time. So, you should separate to some commits(or separate to PRs).

@junichi11 junichi11 requested a review from tmysik March 19, 2023 22:57
@apache apache locked and limited conversation to collaborators Mar 19, 2023
@apache apache unlocked this conversation Mar 19, 2023
@rossluk rossluk force-pushed the multiple_default_namespace_fixes branch 2 times, most recently from 86acaaa to c4e8be0 Compare March 20, 2023 08:58
@rossluk rossluk force-pushed the multiple_default_namespace_fixes branch from c4e8be0 to 97de183 Compare March 20, 2023 22:28
@rossluk rossluk force-pushed the multiple_default_namespace_fixes branch 4 times, most recently from d49732a to 75aa022 Compare March 21, 2023 00:05
@junichi11 junichi11 added this to the NB18 milestone Mar 21, 2023
@rossluk rossluk force-pushed the multiple_default_namespace_fixes branch from 75aa022 to 8786049 Compare March 21, 2023 09:44
@junichi11
Copy link
Member

Removing trailing whitespaces: It's OK only with your changed parts. (because If there are many changes for that, it makes it harder to review :)) Thanks!

junichi11
junichi11 approved these changes Mar 21, 2023
View reviewed changes
Copy link
Member

@junichi11 junichi11 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for your work.

tmysik
tmysik reviewed Mar 21, 2023
View reviewed changes
php/php.editor/src/org/netbeans/modules/php/editor/actions/FixUsesPerformer.java
@@ -570,7 +614,7 @@ public void visit(DeclareStatement node) {
if (CancelSupport.getDefault().isCancelled()) {
return;
}
declareStatements.add(node);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As @junichi11 wrote, please, avoid changes in unrelated files and lines. The reason is that the Git history is then more difficult to read/follow, thanks. I think that the IDE can be configured to format/change only the changed lines (see IDE Options dialog).

tmysik
tmysik reviewed Mar 21, 2023
View reviewed changes
php/php.editor/src/org/netbeans/modules/php/editor/actions/UsedNamesCollector.java Outdated
@@ -143,10 +106,14 @@ private boolean existsUseForTypeName(final Collection<? extends UseScope> declar

private static class NamespaceNameVisitor extends DefaultVisitor {
private final OffsetRange offsetRange;
private final Map<String, List<UsedNamespaceName>> existingNames = new HashMap<>();
private final NamespaceScope namespaceScope;
private NamespaceScope currentScope;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nitpick: we usually put final fields before the non-final ones.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed.

tmysik
tmysik approved these changes Mar 21, 2023
View reviewed changes
Copy link
Member

@tmysik tmysik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not a small change, perfectly done! Thanks a lot!

@tmysik
Copy link
Member

tmysik commented Mar 21, 2023

@junichi11 @rossluk

Guys, I added just 2 nitpicks, I am fine if we merge this PR as it is now. So, Junichi, please, feel free to resolve all the comments and merge this PR.

Thank you both for your work!

@rossluk
FixUsesPerformer,ImportDataCreator and UsedNamespaceCollector logic u…
1bda109 
…pdate to fix imports in whole file, fix ModelUtils multiple default namespace detection
@rossluk rossluk force-pushed the multiple_default_namespace_fixes branch from 8786049 to 1bda109 Compare March 21, 2023 19:30
@junichi11 junichi11 merged commit eba3855 into apache:master Mar 22, 2023
@rossluk rossluk deleted the multiple_default_namespace_fixes branch March 22, 2023 06:59
@junichi11 junichi11 mentioned this pull request Jul 5, 2023
Merged
junichi11 added a commit to junichi11/netbeans that referenced this pull request Jul 11, 2023
@junichi11
Don't remove existing uses if they are the same as the created string…
c6ce81a 
… for use statements

- apache#5681
- Avoid changing the file state if the created string is the same as the
  existing use statments
junichi11 added a commit to junichi11/netbeans that referenced this pull request Jul 11, 2023
@junichi11
Don't remove existing uses if they are the same as the created string…
c762224 
… for use statements

- apache#5681
- Avoid changing the file state if the created string is the same as the
  existing use statements
@junichi11 junichi11 mentioned this pull request Jul 11, 2023
Merged
junichi11 added a commit to junichi11/netbeans that referenced this pull request Jul 12, 2023
@junichi11
Don't remove existing uses if they are the same as the created string…
26ab50f 
… for use statements

- apache#5681
- Avoid changing the file state if the created string is the same as the
  existing use statements
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tmysik tmysik tmysik approved these changes

@junichi11 junichi11 junichi11 approved these changes

Assignees
No one assigned
Labels
PHP [ci] enable extra PHP tests (php/php.editor)
Projects
None yet
Milestone
NB18
Development

Successfully merging this pull request may close these issues.
3 participants
@rossluk @junichi11 @tmysik