Support installing plugins from specific ref; and from local path #1743

emlys · 2025-01-21T22:36:25Z

Description

Fixes #1651

Updates the "Manage plugins" modal and the plugin installation function to support installing plugins from a branch or revision on a remote git server, or from a local filepath.

The ReadTheDocs build has been failing on branches off of feature/plugins for a while now, so it's unrelated to these changes.

Checklist

Updated HISTORY.rst and link to any relevant issue (if these changes are user-facing)
Updated the user's guide (if needed)
Tested the Workbench UI (if relevant)

emilyanndavis

Looks good, @emlys! Just a handful of small suggestions.

emilyanndavis · 2025-01-23T22:21:54Z

workbench/src/renderer/components/PluginModal/index.jsx

+            <Form.Control
+              id="url"
+              type="text"
+              placeholder="https://github.com/foo/bar.git"


I think a placeholder is useful here so that people understand the expected format (and don't try use a URL prefixed with git@, for example), but could we change it slightly to make it a little clearer what the parts of the URL are? Maybe something like https://github.com/owner/repo.git?

Good suggestion!

emilyanndavis · 2025-01-23T22:25:18Z

workbench/src/renderer/components/PluginModal/index.jsx

+            <Form.Control
+              id="branch"
+              type="text"
+              placeholder={t('default')}


Here I'm not sure we need a placeholder, and it might even be confusing, leading some people to think they need to enter default or main when they can just leave it blank. If it were me, I'd remove the placeholder and also add (optional) to the label, just to make it extra clear.

emilyanndavis · 2025-01-23T22:30:10Z

workbench/src/renderer/components/PluginModal/index.jsx

+          id="path"
+          type="text"
+          placeholder={
+            window.Workbench.OS === 'darwin' ? '/Users/natcap/foo/bar/' : 'C:\\Documents\\foo\\bar\\'


Like with the sample git URL, I'd recommend updating these sample paths as well. Maybe something along the lines of /Users/username/path/to/plugin/ and C:\\Documents\\path\\to\\plugin\\.

That does look better!

emilyanndavis · 2025-01-23T22:32:13Z

workbench/src/renderer/components/PluginModal/index.jsx

+      <>
+        <Form.Row>
+          <Form.Group as={Col} xs={7} className="mb-1">
+            <Form.Label>Git URL</Form.Label>


Let's make sure this label is associated with its form field with htmlFor="url".

emilyanndavis · 2025-01-23T22:33:07Z

workbench/src/renderer/components/PluginModal/index.jsx

+            />
+          </Form.Group>
+          <Form.Group as={Col} className="mb-1">
+            <Form.Label>Branch, tag, or commit</Form.Label>


This label needs an htmlFor attribute as well.

emilyanndavis · 2025-01-23T22:33:17Z

workbench/src/renderer/components/PluginModal/index.jsx

+  } else {
+    pluginFields = (
+      <Form.Group className="px-0 mb-1">
+        <Form.Label>Local absolute path</Form.Label>


This label needs an htmlFor attribute as well.

Making a note to pay attention to htmlFor!

emilyanndavis · 2025-01-23T22:47:18Z

workbench/src/renderer/components/PluginModal/index.jsx

-          <Form.Text className="text-muted">
-            {t('This may take several minutes')}
-          </Form.Text>
+          <Form.Label className="col-form-label-lg" htmlFor="url">{t('Add a plugin')}</Form.Label>


Looks like this label's htmlFor is still pointing to the url form field.

Now that the form's been restructured, I'd almost argue that "Add a plugin" isn't really a label anymore; it's more of a heading. But if we made it a heading, we'd want to add a separate label for the "Install from..." dropdown, and we'd want to make similar changes to the "Remove a plugin" section, for consistency. Happy to talk through those details (and the motivations behind them) if you want! But...

...for now, the simplest thing would be to update the "Add a plugin" label's htmlFor to point to the "Install from..." dropdown.

I agree it makes more sense as a heading. I changed "Add a plugin" and "Remove a plugin" to headings, and made a label "Install from" for the install options dropdown. I made the dropdown in line with the "Install from" label, but I couldn't get it perfectly aligned - which do you think looks better?

Awesome! I would vote for the second option, with the label stacked above the form field, since that's consistent with the other fields in this form. If the alignment isn't quite working like you expect, you might try mimicking the structure you've used elsewhere in the form, i.e., adding as={Col} to the Form.Group and nesting that inside a Form.Row ... and/or seeing if you are using any utility classes (such as mx-1) you no longer need.

Okay, I changed it to the second option, removed some unneeded styling, and it does look better now!

emilyanndavis · 2025-01-23T22:56:33Z

workbench/src/main/setupAddRemovePlugin.js

+        } else { // install from local path
+          logger.info(`adding plugin from ${path}`);
+          installString = path;
+          // Read in the plugin's pyproject.toml, then delete it


It looks like you're not deleting pyproject.toml when installing from a local path... which seems correct. Does this comment just need an update?

Ooh yep, thanks

emilyanndavis · 2025-01-23T23:06:06Z

workbench/src/main/setupAddRemovePlugin.js

        // Access plugin metadata from the pyproject.toml
        const pluginID = pyprojectTOML.tool.natcap.invest.model_id;
        const pluginName = pyprojectTOML.tool.natcap.invest.model_name;
        const pluginPyName = pyprojectTOML.tool.natcap.invest.pyname;
-
        // Create a conda env containing the plugin and its dependencies
        const envName = `invest_plugin_${pluginID}`;
        await spawnWithLogging(


Forgive me: this isn't related to this PR, and you might even have created an issue for this already, in which case, please ignore!

It looks like this call to create the micromamba env is referencing specific version restrictions for Python & GDAL. Seems like it would be a good idea to store these args someplace that's easier to keep up to date rather than hard-coding them here.

You're absolutely right, we hard-coded those just to get something working, but as of #1733 we'll read them from the pyproject.toml.

emilyanndavis · 2025-01-24T21:40:23Z

workbench/src/renderer/components/PluginModal/index.jsx

        </Form.Group>
        <hr />
        <Form.Group className="mb-3">
-          <Form.Label htmlFor="plugin-select">{t('Remove a plugin')}</Form.Label>
+          <h5 className="mb-3">{t('Remove a plugin')}</h5>


I think it's great that you converted this (and "Add a plugin") to a heading, but now the plugin dropdown is technically missing a label. A visible label (marked up as a <label>) is usually best, but we do have a few options in this case:

Add a <Form.Label> with htmlFor="plugin-select". What that label says is up to you, as long as it describes the purpose of the form field ("Select a plugin", "Plugin to remove", etc.).

OR add an aria-label to the <Form.Control>, with its value set to something that describes the purpose of the form field (i.e., whatever you would use for a visible label, if you chose option 1).

OR add an id to the "Remove a plugin" heading, and add aria-labelledby to the <Form.Control>, with its value set to the heading's id. This tells assistive technologies (screen readers, voice input control, etc.) to associate the form field with the heading, essentially allowing the "Remove a plugin" heading to act both as a heading and as a label for the form field.

I went with option 1 (and changed the id to be a little more specific)

Great, thank you!

emilyanndavis

Thanks for making those changes, @emlys! Just one more request!

… styling

emilyanndavis · 2025-01-24T23:48:18Z

Looks like all those label changes broke some of the workbench tests. As soon as those are passing, I'll happily approve!

dcdenu4

Thanks @emlys ! I had one quick curiosity question. And just noting that I merged the other git PR which has introduced some conflicts!

workbench/src/main/setupAddRemovePlugin.js

dcdenu4

Thanks @emlys and @emilyanndavis !

update plugin modal form to support branch or ref, and local path

364ab62

emlys changed the base branch from main to feature/plugins January 21, 2025 22:36

emlys added 3 commits January 21, 2025 16:53

support installing from a specific revision on remote

ed7f006

support installing plugin from local path

31672ff

add platform-dependent placeholder text for local plugin path

26301f4

emlys self-assigned this Jan 23, 2025

emlys requested review from emilyanndavis and dcdenu4 January 23, 2025 00:01

emlys marked this pull request as ready for review January 23, 2025 00:01

emilyanndavis requested changes Jan 23, 2025

View reviewed changes

emlys added 3 commits January 24, 2025 10:03

improve placeholder and label text in plugin modal form

8f9c967

fix labels and htmlFors in plugin modal form

665d511

update comment

5e33768

emlys requested a review from emilyanndavis January 24, 2025 18:36

emilyanndavis reviewed Jan 24, 2025

View reviewed changes

emilyanndavis requested changes Jan 24, 2025

View reviewed changes

stack plugin 'install from' label over dropdown; remove some unneeded…

4be6f50

… styling

emlys requested a review from emilyanndavis January 24, 2025 22:45

update plugin modal component labels in plugin tests

d5d3e64

dcdenu4 requested changes Jan 27, 2025

View reviewed changes

workbench/src/main/setupAddRemovePlugin.js Show resolved Hide resolved

emlys and others added 2 commits January 27, 2025 11:21

Merge branch 'feature/plugins' into feature/1651

33765f3

move variable definition

823ea7f

emlys requested a review from dcdenu4 January 27, 2025 20:30

emilyanndavis approved these changes Jan 27, 2025

View reviewed changes

dcdenu4 approved these changes Jan 30, 2025

View reviewed changes

dcdenu4 merged commit 764477b into natcap:feature/plugins Jan 30, 2025
28 of 29 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support installing plugins from specific ref; and from local path #1743

Support installing plugins from specific ref; and from local path #1743

emlys commented Jan 21, 2025 •

edited

Loading

emilyanndavis left a comment

emilyanndavis Jan 23, 2025

emlys Jan 24, 2025

emilyanndavis Jan 23, 2025

emilyanndavis Jan 23, 2025

emlys Jan 24, 2025

emilyanndavis Jan 23, 2025

emilyanndavis Jan 23, 2025

emilyanndavis Jan 23, 2025

emlys Jan 24, 2025

emilyanndavis Jan 23, 2025

emlys Jan 24, 2025

emilyanndavis Jan 24, 2025

emlys Jan 24, 2025

emilyanndavis Jan 23, 2025

emlys Jan 24, 2025

emilyanndavis Jan 23, 2025

emlys Jan 24, 2025

emilyanndavis Jan 24, 2025

emlys Jan 24, 2025

emilyanndavis Jan 24, 2025

emilyanndavis left a comment

emilyanndavis commented Jan 24, 2025 •

edited

Loading

dcdenu4 left a comment

dcdenu4 left a comment

Support installing plugins from specific ref; and from local path #1743

Support installing plugins from specific ref; and from local path #1743

Conversation

emlys commented Jan 21, 2025 • edited Loading

Description

Checklist

emilyanndavis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

emilyanndavis left a comment

Choose a reason for hiding this comment

emilyanndavis commented Jan 24, 2025 • edited Loading

dcdenu4 left a comment

Choose a reason for hiding this comment

dcdenu4 left a comment

Choose a reason for hiding this comment

emlys commented Jan 21, 2025 •

edited

Loading

emilyanndavis commented Jan 24, 2025 •

edited

Loading