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

Replaced the usage of deprecated method DumbService.runReadActionInSmartMode(Computable) #1207

Conversation

dessina-devasia
Copy link
Contributor

@dessina-devasia dessina-devasia commented Jan 3, 2025

Fixes #1176 and #1179

The deprecated method DumbService.runReadActionInSmartMode(Computable) has been removed.
Its replacement, ReadAction.nonBlocking(...).inSmartMode(project).executeSynchronously(), is not explicitly required here because the methods ProjectLabelManager.getProjectLabelInfo(), PropertiesManager.findPropertyLocation(), and DiagnosticsHandler.collectDiagnostics() are already executed within a NonBlocking ReadAction.

Copy link
Contributor

@mrglavas mrglavas left a comment

Choose a reason for hiding this comment

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

I've noticed that all the changes in this PR are to classes in the lsp4mp4ij package. These classes have been copied from Intellij Quarkus. We should check whether Red Hat has addressed these issues with using deprecated methods in their current code and consider contributing a PR if they have not. We periodically update LSP4MP4IJ from IntelliJ Quarkus. It is best from a maintenance perspective to be as close to the Red Hat version as possible as there is risk each time we update that we lose changes that have only been locally made in Liberty Tools for IntelliJ.

@dessina-devasia
Copy link
Contributor Author

Sure @mrglavas. Let me have a look on the same

@dessina-devasia
Copy link
Contributor Author

dessina-devasia commented Jan 6, 2025

@mrglavas The deprecated method DumbService.runReadActionInSmartMode(Computable) used in ProjectLabelManager.class, PropertiesManager.class and DiagnosticsHandler.class.

Intellij Quarkus didn't replaced the deprecated references from ProjectLabelManager.class and PropertiesManager.class.

LTI introduced DiagnosticsHandler.class, so it is not available in Intellij Quarkus.

Could you please let me know whether we need to raise a PR on ProjectLabelManager.class and PropertiesManager.class in Intellij Quarkus?

Also can we raise a seperate PR in LTI to handle deprecation in DiagnosticsHandler.class?

@mrglavas
Copy link
Contributor

mrglavas commented Jan 6, 2025

@mrglavas The deprecated method DumbService.runReadActionInSmartMode(Computable) used in ProjectLabelManager.class, PropertiesManager.class and DiagnosticsHandler.class.

Intellij Quarkus didn't replaced the deprecated references from ProjectLabelManager.class and PropertiesManager.class.

LTI introduced DiagnosticsHandler.class, so it is not available in Intellij Quarkus.

Could you please let me know whether we need to raise a PR on ProjectLabelManager.class and PropertiesManager.class in Intellij Quarkus?

Also can we raise a seperate PR in LTI to handle deprecation in DiagnosticsHandler.class?

@dessina-devasia DiagnosticsHandler is a class I refactored out from PropertiesManagerForJava so that it could be shared between the language server support for both MicroProfile and Jakarta EE. The origin of the body of that class would still be from IntelliJ Quarkus (see https://github.com/redhat-developer/intellij-quarkus/blob/main/src/main/java/com/redhat/devtools/intellij/lsp4mp4ij/psi/core/PropertiesManagerForJava.java).

I do think it would be a good idea to raise a PR against IntelliJ Quarkus for all of these changes.

@dessina-devasia
Copy link
Contributor Author

sure @mrglavas

@dessina-devasia
Copy link
Contributor Author

@mrglavas As discussed I've created a related issue #1420 and its PR in IntelliJ Quarkus. Please have a look.

@mrglavas
Copy link
Contributor

mrglavas commented Jan 8, 2025

@mrglavas As discussed I've created a related issue #1420 and its PR in IntelliJ Quarkus. Please have a look.

@dessina-devasia With the green builds I'm going to approve this one, but please keep in synch with the PR you opened in IntelliJ Quarkus if there are any further changes required. Also, can you update the description of this PR to describe the current changes? You don't seem to be using ReadAction.nonBlocking(...).inSmartMode(project).executeSynchronously().

Copy link
Contributor

@mrglavas mrglavas left a comment

Choose a reason for hiding this comment

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

Looks good. Thanks for following-up and opening the PR for IntelliJ Quarkus.

@dessina-devasia
Copy link
Contributor Author

dessina-devasia commented Jan 9, 2025

Thank you @mrglavas. I've updated this PR description as well

Copy link
Contributor

@vaisakhkannan vaisakhkannan left a comment

Choose a reason for hiding this comment

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

Looks good

@dessina-devasia
Copy link
Contributor Author

@mrglavas, should we wait for the PR raised in IntelliJ Quarkus to be merged before proceeding with this PR?

@TrevCraw
Copy link
Contributor

@mrglavas, should we wait for the PR raised in IntelliJ Quarkus to be merged before proceeding with this PR?

I think it would be best to wait. Once the PR is finalized in IntelliJ Quarkus, you can then make sure any adjustments from the IntelliJ Quarkus PR are also applied to this PR.

@dessina-devasia
Copy link
Contributor Author

Sure. Thank you @TrevCraw

@dessina-devasia
Copy link
Contributor Author

Hi @TrevCraw @mrglavas,

Seems the PR merged in IntelliJ Quarkus and the issue #1420 is closed.

So, I'm merging this PR. Thank you!

@dessina-devasia dessina-devasia merged commit 091d9cf into OpenLiberty:main Jan 13, 2025
3 checks passed
@TrevCraw
Copy link
Contributor

Hi @dessina-devasia , please update the PR description to state that this PR also fixes #1179

@dessina-devasia
Copy link
Contributor Author

Sure @TrevCraw

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Address usage of deprecated method 'DumbService.runReadActionInSmartMode(Computable)'
5 participants