-
Notifications
You must be signed in to change notification settings - Fork 5
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
[Intellij plugin] Prompt user to update plugin when installed version is lower than minVersion #410
base: develop
Are you sure you want to change the base?
Conversation
Generate changelog in
|
maybeMinVersion.get()), | ||
NotificationType.ERROR); | ||
notification.notify(project); | ||
PluginsAdvertiser.installAndEnablePlugins(Set.of(PLUGIN_ID), notification::expire); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@@ -1,4 +1,4 @@ | |||
<idea-plugin url="https://github.com/palantir/gradle-jdks"> | |||
<idea-plugin url="https://github.com/palantir/gradle-jdks" require-restart="false"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
following: https://plugins.jetbrains.com/docs/intellij/dynamic-plugins.html
Ran: Plugin DevKit | Plugin descriptor | Plugin.xml dynamic plugin verification from https://plugins.jetbrains.com/docs/intellij/dynamic-plugins.html#restrictions
98b0084
to
b023536
Compare
"Please update the plugin in the Settings window to a version higher than '%s'", | ||
maybeMinVersion.get()), | ||
NotificationType.ERROR); | ||
notification.notify(project); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From the video it looks like if you click the notification it takes you to the update page.
It opens up both the notification and the window to install the plugins. In the video I was just trying to highlight that there is also a pop-up that shows up, but I didn't click on it.
Do you think it is better to just have the pop-up with an "Update" button and if the user clicks on it to actually open the Installation window ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's just best to put the update settings screen in people's faces - I think what you've done here is probably best, I just didn't fully understand from the video.
.map(minVersion -> VersionComparatorUtil.compare(pluginDescriptor.getVersion(), minVersion) > 0) | ||
.orElse(true); | ||
|
||
if (!isPluginUpToDate) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would probably just invert this if to return early.
PluginsAdvertiser.installAndEnablePlugins(Set.of(PLUGIN_ID), notification::expire); | ||
} | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, we still get it, we'd get both. I can remove my notification, but I thought it might be nice to actually tell you that the you need to use the pop-up window to update the plugin.
@@ -33,6 +33,7 @@ public void onStart(@NotNull ExternalSystemTaskId id, String _workingDir) { | |||
|| id.getType() == ExternalSystemTaskType.EXECUTE_TASK)) { | |||
Project project = id.findProject(); | |||
if (project != null) { | |||
project.getService(PluginUpdateCheckerService.class).checkPluginVersion(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess when projects are being refreshed is the place to do this as that's when the .idea/external-dependencies.xml
is being changed.
@@ -0,0 +1,6 @@ | |||
type: improvement | |||
improvement: | |||
description: '[Intellij plugin] Prompt user to update plugin when installed version |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess this is fine to try out in the this project for now; maybe eventually we can either make it a library each plugin uses (might be bad if they all try to open the update window at once though?) or a separate plugin that updates all the other plugins?
👍 |
ExternalDependenciesManager.getInstance(project).getDependencies(DependencyOnPlugin.class).stream() | ||
.filter(dependencyOnPlugin -> | ||
dependencyOnPlugin.getPluginId().equals(pluginId.getIdString())) | ||
.map(DependencyOnPlugin::getMinVersion) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just thought of a possible complication:
- We release a new version of a gradle + intellij plugin, set the min-version of intellij plugin to "force" people to upgrade.
- The Gradle plugin update immediately gets picked up by excavator, applied to people's repos
- Jetbrains takes two business days to approve our intellij plugin.
- Every single time Gradle is refreshed, people get the popup to update to the new IntelliJ version that is not available yet.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that's a good point, I'll see if I can check that the version exists before opening the popup.
But that means we cannot really rely on the minVersion. Probably the best would be if the plugin will always maintain backwards compatibility with the intellij plugin and I can add a test for the logic here that we won't update the min-version of a plugin if that version doesn't already exist.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added a test that checks that the min-version intellij plugin: https://github.com/palantir/gradle-jdks/pull/410/files#diff-1d01bc54a43b33238be402e57110068b45d10d5b2c7f88104b1c1e67080c1a53R36. This should stop us from bumping the required min-version in externalDependencies.xml file.
I could add the check (probably cached + expiration) here in case we are going to "recall"(delete) a version, but I am not sure if we need it atm.
Before this PR
Users would only get a notification in Intellij when the minVersion specified in externalDependencies.xml file is lower than the installed plugin version.
After this PR
For the
palantir-gradle-jdks
plugin only, we will check first (before running the gradle jdks setup) if the plugin meets the required minimum version. If not, then it will show a notification and it will show the Settings > Plugins windowNote: the check will run when sync'ing the gradle projects or when running gradle tasks (similar to the gradle jdk setup).
Screen.Recording.2024-09-12.at.12.56.19.PM.mov
Notes:
intellij provides a plugin checker: https://github.com/JetBrains/intellij-community/blob/master/platform/platform-impl/src/com/intellij/externalDependencies/impl/CheckRequiredPluginsActivity.java but it doesn't seem to have the option to update plugins (it just prints the error message).
==COMMIT_MSG==
[Intellij plugin] Prompt user to update plugin when installed version is lower than minVersion
==COMMIT_MSG==
Possible downsides?