Skip to content

[perf] FlutterExternalIdeActionGroup: make action update thread explicit #8236

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

Merged
merged 1 commit into from
May 29, 2025

Conversation

pq
Copy link
Contributor

@pq pq commented May 29, 2025

Making the action update thread explicit saves the platform from the work of figuring it out for itself (required caching of potentially unneeded data). From a related inspection:

Reports actions, action groups and other ActionUpdateThreadAware classes that do not explicitly state their mode.
When an action or an action group defines its own update() method, IntelliJ Platform tries to mimic the old synchronous way of calling update() and getChildren() methods in the UI thread and supply it with all the data in AnActionEvent.dataContext(). To do that, it caches all the possible data on a background thread beforehand, even if it is not needed.
Provide one of the two modes ActionUpdateThread.EDT or ActionUpdateThread.BGT by overriding the getActionUpdateThread() method.

image


  • I’ve reviewed the contributor guide and applied the relevant portions to this PR.
Contribution guidelines:

@pq pq requested review from jwren and helin24 May 29, 2025 20:59
Copy link
Member

@jwren jwren left a comment

Choose a reason for hiding this comment

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

LGTM. We will want to make sure to manually test our actions before we release this to ensure that this doesn't cause any funny business in the plugin. While I completely appreciate their documentation and the warning that this resolves, I also don't completely trust that the documentation is what happens in practice in the framework.

@pq
Copy link
Contributor Author

pq commented May 29, 2025

Right, thanks! In local testing I confirmed that EDT is the thread (also the default so unsurprising). I should also add that this action group is disabled as of dd5a521 so it's kind of academic until we enable these actions again. @jwren: have you thought about this at all? See also #7975 (and related issues).

@pq pq merged commit 80f6795 into flutter:master May 29, 2025
7 checks passed
@pq pq deleted the pe_actionUpdateThread branch May 29, 2025 21:30
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.

3 participants