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

Declare ActionUpdateThread as EDT for RunFlutterAction #7356

Conversation

bc-lee
Copy link

@bc-lee bc-lee commented Apr 17, 2024

From Intellij 2024.1, any actions that doesn't override getActionUpdateThread triggers a warning. This is because the default behavior OLD_EDT will be removed in the future. To suppress the warning, we need to declare this action as EDT, as it's modifying the presentation in update().

Fixes #7331

Pre-launch Checklist

  • I read the Contributor Guide and followed the process outlined there for submitting PRs.
  • I read the Tree Hygiene wiki page, which explains my responsibilities.
  • I read the Flutter Style Guide recently, and have followed its advice.
  • I signed the CLA.
  • I listed at least one issue that this PR fixes in the description above.
  • I updated/added relevant documentation (doc comments with ///).
  • I added new tests to check the change I am making, or this PR is test-exempt.
  • All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

From Intellij 2024.1, any actions that doesn't override
`getActionUpdateThread` triggers a warning. This is because
the default behavior `OLD_EDT` will be removed in the future.
To suppress the warning, we need to declare this action as EDT,
as it's modifying the presentation in `update()`.

Fixes flutter#7331
@bc-lee bc-lee marked this pull request as draft April 17, 2024 06:53
@helin24
Copy link
Member

helin24 commented Apr 17, 2024

@bc-lee thanks for looking into this. I'm not sure this will fix the full problem, as we have other classes that have shown this error as well. See my comment here: #7330 (comment).

Potentially we could implement the same thing you're doing here in other places, but I'm also wondering if this problem should be fixed in the IJ platform. Do you have any insight into this?

@bc-lee
Copy link
Author

bc-lee commented Apr 17, 2024

The warning generated by Intellij 2024.1 is intentional. JetBrains is pushing all actions to override the getActionUpdateThread to ensure that plugin developers specify the thread on which the action should be executed. (See https://youtrack.jetbrains.com/issue/IJPL-143730/Add-assertion-for-OLDEDT-actions and JetBrains/intellij-community@a45d016 for more details) As such, it's necessary for us to address this warning for each of our actions.

This PR is currently in draft because there are several additional actions to fix. I will request a review of this PR once all the warnings have been addressed.

@helin24
Copy link
Member

helin24 commented Apr 17, 2024

@bc-lee Ah thanks so much for clarifying; I didn't realize IJ intended for us to start overriding the function. In that case, yes, supplying overriding threads for all of the affected classes makes sense.

Is this going to be an issue for all classes extending AnAction? (20 classes currently)

I'm also uncertain about how to decide EDT vs BGT here. The IJ doc here suggests that all actions should change Presentation on update, but has some other notes on EDT vs. BGT. In our case, since this class uses Project during update, does that mean this should instead be in a BGT?

@bc-lee
Copy link
Author

bc-lee commented Apr 17, 2024

From my search result, we have 85 classes including anonymous classes that extend AnAction. See below image for the search result:
Screenshot 2024-04-18 at 08 49 21

Maybe it is not easy to fix them all at once.

@helin24
Copy link
Member

helin24 commented Apr 18, 2024

Yeah, I think it would be reasonable to do one at a time / a few at a time.

My main outstanding question is whether this change should be EDT or BGT, as I'm not sure that modifying Presentation counts as changing UI "directly".

@bc-lee
Copy link
Author

bc-lee commented Apr 21, 2024

It turns out that in most cases, the getActionUpdateThread should return BGT.

Official Document

The preferred method is to run the update on the BGT, which has the advantage of guaranteeing application-wide read access to PSI, the virtual file system (VFS), or project models.

Jetbrains Platform Slack

Use BGT always and only if you need some data from UI switch to EDT

However, I cannot judge easily if each case is okay to run on the BGT or not. It appears that many of Intellij API's are okay to use both EDT and BGT and if there is a specific requirement, that API has annotation like @RequiresEdt or @RequiresBackgroundThread. Also, the DevKit Plugin can check if the code is running on the correct thread.

I'm closing this PR for now as I cannot give a clear answer to this problem, but if someone can give me some advice on how to handle this, I would appreciate it.

@bc-lee bc-lee closed this Apr 21, 2024
@bc-lee bc-lee deleted the feature/set-thread-for-run-flutter-action branch April 21, 2024 00:16
@alexander-doroshko
Copy link
Contributor

Hi from JetBrains,

RunFlutterAction should be updated in BGT because it doesn't manipulate Swing components.

Blind guess is that pretty much all actions in the Flutter plugin should be updated in BGT.

@helin24
Copy link
Member

helin24 commented Apr 25, 2024

@alexander-doroshko thanks for the advice!

@bc-lee feel free to reopen this PR. Are you set up to test this?

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.

Exception in plugin Flutter
3 participants