Declare ActionUpdateThread as EDT for RunFlutterAction

[bc-lee]

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

• [x] I read the Contributor Guide and followed the process outlined there for submitting PRs.
• [x] I read the Tree Hygiene wiki page, which explains my responsibilities.
• [x] I read the Flutter Style Guide recently, and have followed its advice.
• [x] I signed the CLA.
• [x] 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.

[helin24]

@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: https://github.com/flutter/flutter-intellij/issues/7330#issuecomment-2043523014.

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]

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 https://github.com/JetBrains/intellij-community/commit/a45d0161609b0ea036252fcc87c10d1bd186f22e 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]

@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]

From my search result, we have 85 classes including anonymous classes that extend AnAction. See below image for the search result:

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

[helin24]

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]

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.

[alexander-doroshko]

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]

@alexander-doroshko thanks for the advice!

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