Use app model for plugins menu by lucyleeow · Pull Request #4991 · napari/napari

lucyleeow · 2022-08-30T06:06:48Z

Description

Migrate plugins menubar and widget creation to app-model. Notable changes:

Amend _provide_viewer to use PublicOnlyProxy
Add _add_plugin_dock_widget processor, which adds the dock widget to the qt main window, to run on the output of widget callbacks.
Move samples menu and plugin widget menu code from _npe2.py to under qt in _qnpe2.py. Allow importing from _qnpe2.py in _npe2.py, see: Use app model for plugins menu #4991 (comment) for more details
Amended/added plugin menu tests and plugin enable/disable tests
Removed add_plugin_dock_widget and _add_plugin_function_widget tests as these functions are slated for removal

for more information, see https://pre-commit.ci

lucyleeow · 2022-09-16T05:22:52Z

This is ready for review, again I hope I got the action callbacks correct.

lucyleeow · 2022-09-16T06:16:16Z

@@ -33,18 +31,6 @@ def __init__(self, window: 'Window'):
        self._build()

    def _build(self, event=None):


I've kept the _build method as it is used here:

napari/napari/plugins/_npe2.py

Lines 311 to 317 in beaa98e

# TODO: after app-model, these QMenus will be evented and self-updating

# and we can remove this... but `_register_manifest_actions` will need to

# add the actions file and plugins menus (since we don't require plugins to

# list them explicitly)

for v in Viewer._instances:

v.window.plugins_menu._build()

v.window.file_menu._rebuild_samples_menu()

but not sure what the comment means with regard to how _build should change. Related: https://github.com/napari/napari/pull/4865/files#r972649032

I see a few plugin widget tests are failing but I kept the _add_registered_widget fun - any ideas @DragaDoncila ?

but not sure what the comment means with regard to how _build should change.

So, I think this is saying that once we build the plugin menu with app_model, it should update itself when plugins are enabled/disabled, so we shouldn't need to update them here. We should then also be adding the plugin menu actions & actions created from plugin widgets in register_manifest_actions...?

I see a few plugin widget tests are failing but I kept the _add_registered_widget fun - any ideas @DragaDoncila ?

Yeah this is just a testing quirk but, now that we're not adding the Install/Uninstall Plugins and Plugin Errors buttons via this menu, there's no separator anymore so these few lines of code clear your action list (and are reproduced in all the failing tests lol). I feel like we don't even need to filter the actions, since we're just checking whether a given widget name is in there?

We should then also be adding the plugin menu actions & actions created from plugin widgets in register_manifest_actions...?

Thanks for explaining. To clarify I can make this change in this PR (since we are moving plugins menu to use app-model)?

Also any thoughts on: https://github.com/napari/napari/pull/4865/files#r972649032

I feel like we don't even need to filter the actions, since we're just checking whether a given widget name is in there

Agreed. I will update the test.

github-actions · 2022-09-16T06:24:39Z

Click here to download the docs artifacts
docs
(zip file)

codecov · 2022-09-27T00:39:39Z

Codecov Report

Attention: Patch coverage is 95.08547% with 23 lines in your changes are missing coverage. Please review.

Project coverage is 92.29%. Comparing base (d691a60) to head (c0c4c55).
Report is 8 commits behind head on main.

Files	Patch %	Lines
napari/_qt/_qplugins/_qnpe2.py	86.61%	19 Missing ⚠️
napari/_qt/_qapp_model/qactions/_plugins.py	85.71%	3 Missing ⚠️
napari/_qt/qt_main_window.py	93.33%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #4991      +/-   ##
==========================================
- Coverage   92.36%   92.29%   -0.08%     
==========================================
  Files         605      611       +6     
  Lines       54469    54683     +214     
==========================================
+ Hits        50312    50470     +158     
- Misses       4157     4213      +56

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

github-actions · 2022-09-27T01:05:58Z

Click here to download the docs artifacts
docs
(zip file)

DragaDoncila

I've left a few comments. Looking good in general, but when I pull the code down the Plugin menu doesn't get populated and I get a TypeError when trying to open the Install/Uninstall Plugins... dialog:

TypeError                                 Traceback (most recent call last)
File /opt/miniconda3/envs/napari-edit/lib/python3.10/site-packages/in_n_out/_store.py:774, in Store.inject.<locals>._inner.<locals>._exec(*args=(), **kwargs={})
    773 try:
--> 774     result = func(**_kwargs)
        _kwargs = {}
        func = <function _show_plugin_install_dialog at 0x7f7bc1e01ea0>
    775 except TypeError as e:
    776     # likely a required argument is still missing.
    777     # show what was injected and raise

TypeError: _show_plugin_install_dialog() missing 1 required positional argument: 'window'

DragaDoncila · 2022-09-27T05:17:26Z

@@ -33,18 +31,6 @@ def __init__(self, window: 'Window'):
        self._build()

    def _build(self, event=None):


but not sure what the comment means with regard to how _build should change.

So, I think this is saying that once we build the plugin menu with app_model, it should update itself when plugins are enabled/disabled, so we shouldn't need to update them here. We should then also be adding the plugin menu actions & actions created from plugin widgets in register_manifest_actions...?

DragaDoncila · 2022-09-27T05:40:10Z

@@ -33,18 +31,6 @@ def __init__(self, window: 'Window'):
        self._build()

    def _build(self, event=None):


I see a few plugin widget tests are failing but I kept the _add_registered_widget fun - any ideas @DragaDoncila ?

Yeah this is just a testing quirk but, now that we're not adding the Install/Uninstall Plugins and Plugin Errors buttons via this menu, there's no separator anymore so these few lines of code clear your action list (and are reproduced in all the failing tests lol). I feel like we don't even need to filter the actions, since we're just checking whether a given widget name is in there?

Czaki · 2022-09-27T08:00:38Z

I get a TypeError when trying to open the Install/Uninstall Plugins... dialog:

it looks like a problem with the injection process.

…lugin

github-actions · 2022-09-28T02:56:41Z

Click here to download the docs artifacts
docs
(zip file)

lucyleeow · 2022-10-01T06:45:27Z

Full traceback just for my reference:

The above exception was the direct cause of the following exception:

TypeError                                 Traceback (most recent call last)
File ~/miniconda3/envs/napari/lib/python3.10/site-packages/app_model/backends/qt/_qaction.py:52, in QCommandAction._on_triggered(self=QMenuItemAction(MenuItem(when=None, group='1_plu...tle=None, toggled=None), alt=None), app='napari'), checked=False)
     48 def _on_triggered(self, checked: bool) -> None:
     49     # execute_command returns a Future, for the sake of eventually being
     50     # asynchronous without breaking the API.  For now, we call result()
     51     # to raise any exceptions.
---> 52     self._app.commands.execute_command(self._command_id).result()
        self._command_id = 'napari:plugins:plugin_install_dialog'
        self = QMenuItemAction(MenuItem(when=None, group='1_plugins', order=1.0, command=CommandRule(id='napari:plugins:plugin_install_dialog', title='Install/Uninstall Plugins...', category=None, tooltip=None, status_tip=None, icon=None, enablement=None, short_title=None, toggled=None), alt=None), app='napari')
        self._app = Application('napari')

File ~/miniconda3/envs/napari/lib/python3.10/site-packages/app_model/registries/_commands_reg.py:202, in CommandsRegistry.execute_command(self=<CommandsRegistry at 0x7f9c582aac80 (65 commands)>, id='napari:plugins:plugin_install_dialog', execute_asychronously=False, *args=(), **kwargs={})
    198 except Exception as e:
    199     if self._raise_synchronous_exceptions:
    200         # note, the caller of this function can also achieve this by
    201         # calling `future.result()` on the returned future object.
--> 202         raise e
    203     future.set_exception(e)
    205 return future

File ~/miniconda3/envs/napari/lib/python3.10/site-packages/app_model/registries/_commands_reg.py:197, in CommandsRegistry.execute_command(self=<CommandsRegistry at 0x7f9c582aac80 (65 commands)>, id='napari:plugins:plugin_install_dialog', execute_asychronously=False, *args=(), **kwargs={})
    195 future: Future = Future()
    196 try:
--> 197     future.set_result(cmd(*args, **kwargs))
        future = <Future at 0x7f9c24cd8940 state=pending>
        cmd = <function _show_plugin_install_dialog at 0x7f9c24d4a0e0>
        args = ()
        kwargs = {}
    198 except Exception as e:
    199     if self._raise_synchronous_exceptions:
    200         # note, the caller of this function can also achieve this by
    201         # calling `future.result()` on the returned future object.

File ~/miniconda3/envs/napari/lib/python3.10/site-packages/in_n_out/_store.py:890, in Store.inject_processors.<locals>._deco.<locals>._exec(*args=(), **kwargs={})
    888 @wraps(func)
    889 def _exec(*args: P.args, **kwargs: P.kwargs) -> R:
--> 890     result = func(*args, **kwargs)
        func = <function _show_plugin_install_dialog at 0x7f9c24d4a050>
        args = ()
        kwargs = {}
    891     if result is not None:
    892         self.process(
    893             result,
    894             type_hint=type_hint,
    895             first_processor_only=first_processor_only,
    896             raise_exception=raise_exception,
    897         )

File ~/miniconda3/envs/napari/lib/python3.10/site-packages/in_n_out/_store.py:781, in Store.inject.<locals>._inner.<locals>._exec(*args=(), **kwargs={})
    775 except TypeError as e:
    776     # likely a required argument is still missing.
    777     # show what was injected and raise
    778     _argnames = (
    779         f"arguments: {set(_kwargs)!r}" if _kwargs else "NO arguments"
    780     )
--> 781     raise TypeError(
    782         f"After injecting dependencies for {_argnames}, {e}"
    783     ) from e
    785 return result

TypeError: After injecting dependencies for NO arguments, _show_plugin_install_dialog() missing 1 required positional argument: 'window'

Czaki

few remarks

lucyleeow · 2024-03-07T01:40:48Z

@Czaki I think I've addressed everything, just one more item waiting for your response: #4991 (comment)

lucyleeow · 2024-03-09T10:53:48Z

+    # Check that widget removed from `_dock_widgets` dict when closed
+    widget.destroyOnClose()
+    assert 'test' not in viewer.window._dock_widgets
+    assert ww.parent is None


So in the original test this was adapted from, there was more widget clean up steps:

napari/napari/_qt/_tests/test_plugin_widgets.py

Lines 149 to 155 in 27851ee

# Check that widget is destroyed when closed.

dw.destroyOnClose()

assert action not in viewer.window.plugins_menu.actions()

assert widg.parent() is None

widg.deleteLater()

widg.close()

qtbot.wait(50)

are these needed?

Czaki

Based on Your promise of followup PR I think that this is ready for merge.

# References and relevant issues Follows on from #4991 # Description * Use `partial` for the npe1 sample menu build * Fix the looping, we should have one list of submenus and actions, which gets added at the end of all looping (I've tested in env with 2 npe1 plugins). This also allows us to add all actions and submenus to the `unregister` list, so actions are correct when plugin status is changed.

# References and relevant issues Follow on from #4991 see: #4991 (comment) # Description Better naming in `test_widget_hide_destroy` Adds more widget clean up steps. The tests passes fine currently but Draga suggested it was best to add these back.

# References and relevant issues Follow on from #4991 # Description Removes lambdas when updating from context during `add_menu` `_qt_main_window.py`. Each menu function just gets layerlist and does not use a helper function anymore. Prevents any use of strings now. Not 100% this is better. There is a lot of repetition but there was previously as well. Open to change.

lucyleeow added 4 commits August 25, 2022 21:31

add command ids

0b8d3a1

menu id

4cb37e0

add qactions

d9b0c76

wip

4dfb28b

lucyleeow marked this pull request as draft August 30, 2022 06:06

github-actions Bot assigned lucyleeow Aug 30, 2022

github-actions Bot added the qt Relates to qt label Aug 30, 2022

[pre-commit.ci] auto fixes from pre-commit.com hooks

5db580e

for more information, see https://pre-commit.ci

lucyleeow mentioned this pull request Aug 30, 2022

TODO list for integration of app-model #4860

Open

21 tasks

lucyleeow added 2 commits September 1, 2022 11:55

actions attempt

baa51e1

merge

987245a

lucyleeow commented Sep 1, 2022

View reviewed changes

Comment thread napari/_qt/menus/plugins_menu.py Outdated

lucyleeow changed the title ~~WIP Use app model for plugins menu~~ Use app model for plugins menu Sep 16, 2022

lucyleeow requested a review from DragaDoncila September 16, 2022 05:22

lucyleeow marked this pull request as ready for review September 16, 2022 05:22

lucyleeow added 3 commits September 16, 2022 15:53

remove unused fun in plugins_menu

27cf27e

merge main, update qt main window

9dcb321

add build back in

1e164d2

lucyleeow commented Sep 16, 2022

View reviewed changes

Merge branch 'main' into menu_plugin

4c06998

DragaDoncila reviewed Sep 27, 2022

View reviewed changes

lucyleeow added 2 commits September 28, 2022 12:45

Merge branch 'main' into menu_plugin

833ae4c

Merge branch 'menu_plugin' of github.com:lucyleeow/napari into menu_p…

e5ebf22

…lugin

lucyleeow added 6 commits March 6, 2024 12:58

merge main

d0d9a31

rm setup cfg

e3e1d62

add raise to provider

3c4aa98

fix typo

77f0432

use new provider in qnpe2

d55ad69

use 2 func provide or raise

6c331b5

Czaki reviewed Mar 6, 2024

View reviewed changes

lucyleeow added 4 commits March 7, 2024 11:07

add tests

811aea4

review

b4dbae0

lint

6467f28

lint

fd45dba

lucyleeow mentioned this pull request Mar 7, 2024

QtViewerDockWidget.isVisible does not seem to work in tests #6722

Closed

add widget tests

fed2a29

lucyleeow commented Mar 9, 2024

View reviewed changes

fix test

c0c4c55

Czaki approved these changes Mar 11, 2024

View reviewed changes

Czaki added the ready to merge Last chance for comments! Will be merged in ~24h label Mar 11, 2024

Czaki merged commit fa0c3b1 into napari:main Mar 12, 2024

Czaki removed the ready to merge Last chance for comments! Will be merged in ~24h label Mar 12, 2024

lucyleeow deleted the menu_plugin branch March 13, 2024 00:27

This was referenced Mar 13, 2024

Fix npe1 samples menu building #6739

Merged

Fix Remove lambdas when updating menus from context in Window #6740

Merged

Improve widget clean up in test_widget_hide_destroy #6741

Merged

DragaDoncila mentioned this pull request Mar 21, 2024

allow v2.3 in metadataversion, fallback to any string napari/npe2#344

Merged

lucyleeow mentioned this pull request Jul 2, 2024

Add test for layer context menu actions using execute_command #7030

Merged

psobolewskiPhD mentioned this pull request Nov 10, 2024

Plugin widget display state is broken #4435

Closed

		@@ -33,18 +31,6 @@ def __init__(self, window: 'Window'):
		self._build()

		def _build(self, event=None):

	# TODO: after app-model, these QMenus will be evented and self-updating
	# and we can remove this... but `_register_manifest_actions` will need to
	# add the actions file and plugins menus (since we don't require plugins to
	# list them explicitly)
	for v in Viewer._instances:
	v.window.plugins_menu._build()
	v.window.file_menu._rebuild_samples_menu()

	# Check that widget is destroyed when closed.
	dw.destroyOnClose()
	assert action not in viewer.window.plugins_menu.actions()
	assert widg.parent() is None
	widg.deleteLater()
	widg.close()
	qtbot.wait(50)

Uh oh!

Conversation

lucyleeow commented Aug 30, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Uh oh!

Uh oh!

lucyleeow commented Sep 16, 2022

Uh oh!

lucyleeow Sep 16, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions Bot commented Sep 16, 2022

Uh oh!

codecov Bot commented Sep 27, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

github-actions Bot commented Sep 27, 2022

Uh oh!

DragaDoncila left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Czaki commented Sep 27, 2022

Uh oh!

github-actions Bot commented Sep 28, 2022

Uh oh!

lucyleeow commented Oct 1, 2022

Uh oh!

Czaki left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

lucyleeow commented Mar 7, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Czaki left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

lucyleeow commented Aug 30, 2022 •

edited

Loading

lucyleeow Sep 16, 2022 •

edited

Loading

codecov Bot commented Sep 27, 2022 •

edited

Loading