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

[Intellij plugin] Prompt user to update plugin when installed version is lower than minVersion #410

Open
wants to merge 13 commits into
base: develop
Choose a base branch
from

Conversation

crogoz
Copy link
Contributor

@crogoz crogoz commented Sep 11, 2024

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.

Screenshot 2024-09-11 at 4 57 26 PM

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 window
Note: 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?

@changelog-app
Copy link

changelog-app bot commented Sep 11, 2024

Generate changelog in changelog/@unreleased

What do the change types mean?
  • feature: A new feature of the service.
  • improvement: An incremental improvement in the functionality or operation of the service.
  • fix: Remedies the incorrect behaviour of a component of the service in a backwards-compatible way.
  • break: Has the potential to break consumers of this service's API, inclusive of both Palantir services
    and external consumers of the service's API (e.g. customer-written software or integrations).
  • deprecation: Advertises the intention to remove service functionality without any change to the
    operation of the service itself.
  • manualTask: Requires the possibility of manual intervention (running a script, eyeballing configuration,
    performing database surgery, ...) at the time of upgrade for it to succeed.
  • migration: A fully automatic upgrade migration task with no engineer input required.

Note: only one type should be chosen.

How are new versions calculated?
  • ❗The break and manual task changelog types will result in a major release!
  • 🐛 The fix changelog type will result in a minor release in most cases, and a patch release version for patch branches. This behaviour is configurable in autorelease.
  • ✨ All others will result in a minor version release.

Type

  • Feature
  • Improvement
  • Fix
  • Break
  • Deprecation
  • Manual task
  • Migration

Description

[Intellij plugin] Prompt user to update plugin when installed version is lower than minVersion

Check the box to generate changelog(s)

  • Generate changelog entry

maybeMinVersion.get()),
NotificationType.ERROR);
notification.notify(project);
PluginsAdvertiser.installAndEnablePlugins(Set.of(PLUGIN_ID), notification::expire);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

unfortunately I couldn't test this properly when running ./gradlew runIde since the new Intellij instance has already the latest versions of all the plugins installed. I do expect to see a similar window to (see below the "Update" button) if there is a newer version of the plugin available.
Screenshot 2024-09-12 at 1 47 52 PM

@@ -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">
Copy link
Contributor Author

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

"Please update the plugin in the Settings window to a version higher than '%s'",
maybeMinVersion.get()),
NotificationType.ERROR);
notification.notify(project);
Copy link
Contributor

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. But I don't think that's super obvious it's going to do that. Can we add a link or button like the install extra plugins button?

Screenshot 2024-09-12 at 12 56 23

Copy link
Contributor Author

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 ?

Copy link
Contributor

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) {
Copy link
Contributor

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);
}
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we still get the other not so useful (see below) notification? Or is it suppressed somehow.

Screenshot 2024-09-06 at 16 44 27

Copy link
Contributor Author

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();
Copy link
Contributor

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
Copy link
Contributor

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?

@CRogers
Copy link
Contributor

CRogers commented Sep 12, 2024

👍

ExternalDependenciesManager.getInstance(project).getDependencies(DependencyOnPlugin.class).stream()
.filter(dependencyOnPlugin ->
dependencyOnPlugin.getPluginId().equals(pluginId.getIdString()))
.map(DependencyOnPlugin::getMinVersion)
Copy link
Contributor

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:

  1. We release a new version of a gradle + intellij plugin, set the min-version of intellij plugin to "force" people to upgrade.
  2. The Gradle plugin update immediately gets picked up by excavator, applied to people's repos
  3. Jetbrains takes two business days to approve our intellij plugin.
  4. Every single time Gradle is refreshed, people get the popup to update to the new IntelliJ version that is not available yet.

Copy link
Contributor Author

@crogoz crogoz Sep 12, 2024

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.

Copy link
Contributor Author

@crogoz crogoz Sep 13, 2024

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants