Migrate plugin to be dynamic #24
Conversation
JetBrains has deprecated components to make IDE startup more performant. The commit migrates the DarkModeSync component into a service and post startup activity. Here are some links from JetBrains docs about dynamic plugin migrations. - https://www.jetbrains.org/intellij/sdk/docs/basics/plugin_structure/dynamic_plugins.html - https://jetbrains.org/intellij/sdk/docs/basics/plugin_structure/plugin_components.html
There was a problem hiding this comment.
This is awesome! I was unaware of the new APIs like Service before reading the links you shared.
One thing I learned is that the postStartupActivity listener pertains to the project lifecycle, but Dark Mode Sync operates at the application level. If I understand this correctly, this change creates a new DarkModeSync service each time a new project is opened. Instead, we want one DarkModeSync service singleton at the Application level.
Before we merge this improvement, let's figure out how to listen for the right AppLifecycleListener events to initialize the service as an Application level singleton.
| } | ||
|
|
||
| public static DarkModeSync getInstance(@NotNull Project project) { | ||
| return project.getService(DarkModeSync.class); |
There was a problem hiding this comment.
This creates a new DarkModeSync service for each Project, right? We want one DarkModeSync singleton at the application level
There was a problem hiding this comment.
You are right, this will end up creating a service instance for each project. Good catch!
| </application-components> | ||
| <extensions defaultExtensionNs="com.intellij"> | ||
| <postStartupActivity implementation="com.github.gilday.darkmode.DarkModeSync$MyStartupActivity"/> |
There was a problem hiding this comment.
postStartupActivity pertains to the Project lifecycle. Can we use an AppLifecycleListener instead? I'm thinking we may need to initialize the singleton in both the welcomeScreenDisplayed and appStarting callbacks. Thoughts?
There was a problem hiding this comment.
I've migrated MyStartupActivity into a MyLifecycleListener which implements AppLifecycleListener.
Also DarkModeSync no longer implements Disposable. Instead it is all handled in the following lifecycle methods.
appStarting()-> callsonStartup()appWillBeClosed()->calls onClose()
And there's no longer a need to pass a project to getInstance(), which just returns a singleton.
|
|
||
| /** @param lafManager IDEA look-and-feel manager for getting and setting the current theme */ | ||
| public DarkModeSync(final LafManager lafManager) { | ||
| private static final class MyStartupActivity implements StartupActivity.DumbAware { |
There was a problem hiding this comment.
Can we make this listener public? I assume private works because the platform is using setAccessible before using reflection to create a new instance of this listener, but I would rather not rely on that behavior.
Instead of using StartupActivity, use AppLifecycleListener and make the DarkModeSync class a singleton.
|
Hi Jonathan I'm glad that you found this PR useful 😄 . JetBrains does make a lot of changes to their platform APIs and it is really difficult to figure out what they are and how that will impact things 😦 . I've made the changes you requested. I totally missed that a new service instance would be created for every project opened! Great catch 👏 . Please let me know what you think on this commit that should address these changes: https://github.com/nazmulidris/dark-mode-sync-plugin/commit/47323eb5c35c934e702e1e3180e96b9b38377491 Thanks |
|
FWIW, I would love to see this PR land! The UX of dynamically loading / unloading plugins is really nice as a user! :) |
|
Thanks for making those changes! I tested it out tonight and it's not working as expected. I ran I'll dig in and see if I can figure out why |
JetBrains has deprecated components to make IDE startup more performant.
The commit migrates the DarkModeSync component into a service and post
startup activity. Here are some links from JetBrains docs about dynamic
plugin migrations.