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

Use npe2 adaptor when discovering plugins for the installed list #131

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

DragaDoncila
Copy link
Contributor

@DragaDoncila DragaDoncila commented Feb 20, 2025

Prior to this PR, installing an npe1 plugin with Use npe2 adaptor turned on in napari didn't recognize it as a shimmed plugin. As a result, it didn't ask the user to restart (even though a restart is needed to load the plugin), and didn't display the npe2 (adapted) badge.

This PR updates the manager to read the state of the setting and use it appropriately.

To test that this PR works:

  1. Install napari from main, launch a viewer
  2. Turn on Use npe2 adaptor in Preferences. Restart napari (skip this if you already had the setting turned on)
  3. Open the plugin manager using Install/Uninstall plugins
  4. Install an npe1 plugin e.g. Label-Creator or anything off this list
  5. ❌ Plugin installs, you're not told to restart, the plugin doesn't get the (npe1) adapted label, the plugin widget is not listed in Plugins
  6. (Optional) Restart napari, plugin gets the (npe1) adapted label, plugin widget is listed in Plugins
  7. Uninstall your plugin
  8. Install napari-plugin-manager from this branch
  9. Launch a viewer, install your plugin again
  10. ✔️ You're told to restart, plugin gets (npe1) adapted label, and upon restarting, your widget is listed

Copy link

codecov bot commented Feb 20, 2025

Codecov Report

Attention: Patch coverage is 91.66667% with 1 line in your changes missing coverage. Please review.

Project coverage is 94.97%. Comparing base (3da07a3) to head (ad7d541).

Files with missing lines Patch % Lines
src/napari_plugin_manager/qt_plugin_dialog.py 90.90% 1 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##             main     #131   +/-   ##
=======================================
  Coverage   94.96%   94.97%           
=======================================
  Files          13       13           
  Lines        2107     2109    +2     
=======================================
+ Hits         2001     2003    +2     
  Misses        106      106           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@DragaDoncila
Copy link
Contributor Author

@Czaki I tried looking at ways to test this PR but I can't quite understand whether the current tests are actually covering the showing of the restart message or not. I think they're not... It would be awesome if you could take a look at this and see how I might add a test for this behaviour...

I want to test that when an npe1 plugin is installed with Use npe2 adaptor turned on, the show_message function is called. So, this line here. I've confirmed it works visually.

@Czaki
Copy link
Contributor

Czaki commented Feb 28, 2025

No it is not covered
obraz

@Czaki
Copy link
Contributor

Czaki commented Feb 28, 2025

I want to test that when an npe1 plugin is installed with Use npe2 adaptor turned on, the show_message function is called. So, this line here. I've confirmed it works visually.

you mean that it should be called when npe1 plugin is installed?

@DragaDoncila
Copy link
Contributor Author

you mean that it should be called when npe1 plugin is installed?

Yes, the show_message function should be called when an npe1 plugin is installed and use_npe2_adaptor is True. I've confirmed visually that it works, but couldn't figure out a test

jni pushed a commit to napari/napari that referenced this pull request Mar 4, 2025
# Description
This PR turns on the `Use npe2 adaptor` setting by default, and adds a
start-up warning dialog listing adapted plugins for the user.

After releasing this PR, installed npe1 plugins will be automatically
converted to use the npe2 plugin engine. Users can still turn off the
setting if they notice any issues with the plugins.

The warning dialog will pop up on every napari start-up by default, or
only when a new plugin is installed, if the user selects the checkbox or
changes the warning level in the plugin preferences.

Users will never see this dialog if:
- they only have npe2 plugins installed
- they turn off the `Use npe2 adaptor` setting

If installing npe1 plugins from the plugin manager,
[napari-plugin-manager/#131](napari/napari-plugin-manager#131),
once merged, will ensure the user is told they need to restart (same as
with native npe2 plugins). Once they restart, they will see the warning
dialog.

Warning dialog screenshot:


![image](https://github.com/user-attachments/assets/8048431d-9ab3-48e4-a160-fff358b3eef5)

Preference dialog screenshot:


![image](https://github.com/user-attachments/assets/1886d237-5f12-47f3-966c-418e99b265b8)

---------

Co-authored-by: Draga Doncila <[email protected]>
Co-authored-by: Carol Willing <[email protected]>
Co-authored-by: Grzegorz Bokota <[email protected]>
Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
@DragaDoncila DragaDoncila marked this pull request as ready for review March 14, 2025 05:47
@DragaDoncila
Copy link
Contributor Author

I don't understand why this is failing now 😭 all I did was merge main smh @Czaki any ideas...

@DragaDoncila
Copy link
Contributor Author

Hmm I wonder if it's because we merged napari/#7627 in the meantime... Will take a look

@psobolewskiPhD
Copy link
Member

psobolewskiPhD commented Mar 14, 2025

Tests are broken currently because of changes in the python matrix.
See my PR for an approach to fixing them:
#140

(there may be other issues too of course!)

@DragaDoncila
Copy link
Contributor Author

OK if this passes CI I think we should merge without tests, assuming a couple of people actually pull it down to try it.

If it fails I will try take a look at what the issue is, since it'll probs be related to the napari PR

@psobolewskiPhD
Copy link
Member

psobolewskiPhD commented Mar 15, 2025

All of the fails are with napari from the repo. All of the release (0.5.6) pass.
I suspect the warning from the napari PR is messing up the test in this repo.

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.

4 participants