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.

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

New folders do not register in the jdt.ls workspace #10115 #10665

Merged
merged 1 commit into from
Aug 10, 2018

Conversation

vrubezhny
Copy link
Contributor

Signed-off-by: Victor Rubezhny [email protected]

What does this PR do?

The PR fixes the workflow of editing the classpath of a plain java project
ls10115-part1

What issues does this PR fix or reference?

Depends on eclipse-che/che-ls-jdt#66

Release Notes

Docs PR

@riuvshin
Copy link
Contributor

riuvshin commented Aug 3, 2018

Can one of the admins verify this patch?

@tolusha tolusha requested a review from svor August 6, 2018 06:13
private void notifyClientOnProjectUpdate(String projectPath) {
List<Object> parameters = new ArrayList<>();
parameters.add(removePrefixUri(convertToJson(projectPath).getAsString()));
extensionService.executeClientCommand(CLIENT_UPDATE_ON_PROJECT_CLASSPATH_CHANGED, parameters);
Copy link
Contributor

Choose a reason for hiding this comment

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

As for me it is better to initialize CLIENT_UPDATE_ON_PROJECT_CLASSPATH_CHANGED here che/plugins/plugin-java/che-plugin-java-ext-lang-shared/src/main/java/org/eclipse/che/ide/ext/java/shared/Constants.java. che-ls-jdt isn't a good place because it's never used there.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, it's not used in che-ls-jdt, but still it's a good place to store the constant:

  • to reserve its value for the command constant and make it visible for all the users of Commands.java
  • to prevent having this command constant value from being re-defined in different plugins for different commands (which is quite possible)

IMHO

getEntriesFromServer(event.getProject())
.then(
entries -> {
eventBus.fireEvent(new ClasspathChangedEvent(event.getProject(), entries));
Copy link
Contributor

Choose a reason for hiding this comment

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

What is the difference between ProjectClasspathChangedEvent and ClasspathChangedEvent? Pls add some javadoc into ProjectClasspathChangedEvent

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually, no meaningful difference here..
The problem is that I cannot generate correct parameters for ClasspathChangedEvent in ExecuteClientCommandProcessor.execute(ExecuteCommandParams) (and I think it's not a good place to do that). So, I just notify ClasspathResolver on the fact of changed classpath by using ProjectClasspathChangedEvent, so it can correctly create and fill ClasspathChangedEvent strictly after all server-side modification is finished.
OK. I'll add some javadoc to ProjectClasspathChangedEvent.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

PR is rebased

Copy link
Contributor

@dmytro-ndp dmytro-ndp left a comment

Choose a reason for hiding this comment

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

Approved removal of duplicating command projectExplorer.waitItem(PROJECT_NAME + "/" + NEW_FOLDER_NAME); from selenium test MoveJavaFileInNewSourceFolderTest.java.

@benoitf benoitf added status/code-review This issue has a pull request posted for it and is awaiting code review completion by the community. target/branch Indicates that a PR will be merged into a branch other than master. labels Aug 9, 2018
@vrubezhny
Copy link
Contributor Author

I've rebased the PR and resolved conflicts

@tolusha tolusha merged commit e481c42 into eclipse-che:5730_java_ls_poc Aug 10, 2018
@benoitf benoitf removed the status/code-review This issue has a pull request posted for it and is awaiting code review completion by the community. label Aug 10, 2018
tsmaeder pushed a commit that referenced this pull request Aug 22, 2018
tsmaeder pushed a commit that referenced this pull request Aug 29, 2018
tolusha pushed a commit that referenced this pull request Sep 5, 2018
tsmaeder pushed a commit that referenced this pull request Sep 13, 2018
tsmaeder pushed a commit that referenced this pull request Sep 20, 2018
tsmaeder pushed a commit that referenced this pull request Sep 26, 2018
tsmaeder pushed a commit that referenced this pull request Oct 1, 2018
tsmaeder pushed a commit that referenced this pull request Oct 5, 2018
tsmaeder pushed a commit that referenced this pull request Oct 12, 2018
tsmaeder pushed a commit that referenced this pull request Oct 16, 2018
tsmaeder pushed a commit that referenced this pull request Oct 17, 2018
tsmaeder pushed a commit that referenced this pull request Oct 17, 2018
tsmaeder pushed a commit that referenced this pull request Oct 17, 2018
tsmaeder pushed a commit that referenced this pull request Oct 17, 2018
tsmaeder pushed a commit that referenced this pull request Oct 17, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
target/branch Indicates that a PR will be merged into a branch other than master.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants