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

fix: ActionUpdateThread deprecation warnings #122

Merged
merged 2 commits into from
Oct 4, 2024

Conversation

dw-0
Copy link
Contributor

@dw-0 dw-0 commented Oct 3, 2024

First time contributor checklist

Contributor checklist

  • I am targeting the master branch (and not the release branch)
  • I am using the provided codeStyleConfig.xml
  • I have tested my contribution on these versions of PyCharm/IDEA:
  • IntelliJ IDEA 2024.2.3
  • My contribution is fully baked and ready to be merged as is
  • I ensure that all the open issues my contribution fixes are mentioned in the commit message of my first commit
    using the Fixes #1234 syntax

Description

This PR fixes the ActionUpdateThread deprecation warnings appearing in newer IntelliJ IDEA IDEs while using this Plugin.
During build of the project, there are still some other deprecation warnings. Those were not fixed in the scope of this PR.

I decided to target a 2023 PyCharm version, because any 2024 versions would require usage of the org.jetbrains.intellij.platform plugin version 2.1.0 which would require even more refactorings i was not comfortable with doing them yet. So i decided to bump the org.jetbrains.intellij to the latest version 1.17.4.

Type of Changes

Type
🐛 Bug fix
✨ New feature
🔨 Refactoring
📜 Docs

Related Issue

Closes #120

@leinardi
Copy link
Owner

leinardi commented Oct 4, 2024

Hi @dw-0, thank you for your contribution and for addressing the ActionUpdateThread deprecation warnings! I appreciate the effort you've put into it.

Before I proceed with reviewing the PR, I’d like to kindly ask if you could rebase your branch on top of my old-edt branch. I’ve been working on similar changes there, including fixes for some deprecation warnings, which might overlap with what you’ve tackled. Rebasing will make it easier for me to identify what specifically resolves the issue that I wasn’t able to fix in my branch.

dw-0 added 2 commits October 4, 2024 14:25
fixes leinardi#120

Signed-off-by: Dominik Willner <th33xitus@gmail.com>
Signed-off-by: Dominik Willner <th33xitus@gmail.com>
@dw-0
Copy link
Contributor Author

dw-0 commented Oct 4, 2024

There you go.
I had to build the project twice before i was able to run buildPlugin for some reason, no idea why.
I guess you simply did not add the overwrite to all classes which require the overwrite :)

I built the plugin, installed it from disk, loaded up a python project and did a full project scan. No IDE errors popping up for deprecation in the plugin so far.

@leinardi leinardi changed the base branch from master to old-edt October 4, 2024 12:59
@leinardi
Copy link
Owner

leinardi commented Oct 4, 2024

Hey thanks again, I tried your changes and I can confirm that it seems to fix the issue!
I just have one question before merging it: I saw that you sometimes use ActionUpdateThread.EDT and sometimes use ActionUpdateThread.BGT, can you tell me which logic are you applying between one or the other?

@dw-0
Copy link
Contributor Author

dw-0 commented Oct 4, 2024

As far as i understand the difference, EDT is the UI thread while BGT is the background thread. I read somewhere that choosing EDT for long running tasks can freeze the UI. So i looked at your descriptions of each action and decided if this is something that should be targeted for the background thread. So i decided to go with BGT for any action that involves scanning, and any action that involves displaying something in a menu or similar should be run on the EDT thread.

This is not the actual documentation i read when i made my decision, but at least something you can have a look at if you want:
https://plugins.jetbrains.com/docs/intellij/threading-model.html#avoiding-ui-freezes
The actual doc i read was a javadoc string in the code itself, but i cannot find it for some reason right now.

@leinardi
Copy link
Owner

leinardi commented Oct 4, 2024

Thank you for the explanation and I think that the use of the BGT is actually what is properly fixing the issue because, if you look at my changes carefully, you'll see that I always applied the EDT via the BaseAction, but I was then getting some Slow operations are prohibited on EDT errors due to the use of EDT instead of BGT: #120 (comment)

Final question, do you mind if I add this to the changelog?

- Fixed #120: Defined action update thread for actions (thanks @dw-0 for the help!)

@dw-0
Copy link
Contributor Author

dw-0 commented Oct 4, 2024

Thank you for the explanation and I think that the use of the BGT is actually what is properly fixing the issue because, if you look at my changes carefully, you'll see that I always applied the EDT via the BaseAction, but I was then getting some Slow operations are prohibited on EDT errors due to the use of EDT instead of BGT: #120 (comment)

Ah, yeah that makes sense now! I removed the overwrite from the BaseAction again, because i thought that adding it explicitly in each action makes more sense and a developer has to really think about which action should be handled in which thread. You could of course keep the overwrite in the BaseAction but then you have to overwrite it AGAIN in some actions where you need the other thread. Thats kind of odd.

Final question, do you mind if I add this to the changelog?

- Fixed #120: Defined action update thread for actions (thanks @dw-0 for the help!)

Sure! Feel free to do so :)

@leinardi
Copy link
Owner

leinardi commented Oct 4, 2024

I removed the overwrite from the BaseAction again, because i thought that adding it explicitly in each action makes more sense and a developer has to really think about which action should be handled in which thread.

I would 100% agree with you if the override was for an abstract method but, unfortunately, it is not abstract and it will build totally fine if the developer completely forget to override it and, since the majority of actions are EDT and you still get errors for slow threads on EDT, I would still prefer to add the override to the BaseAction, just to avoid some code duplication. Hope you don't mind.

@leinardi leinardi merged commit 7c6576b into leinardi:old-edt Oct 4, 2024
1 check failed
@dw-0
Copy link
Contributor Author

dw-0 commented Oct 4, 2024

Okay i didn't view at it from this perspective, you are right there. I don't mind if you add it back to the BaseAction class then. Its safer, as you explained.

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.

2 participants