Closed goanpeca closed 2 months ago
Attention: Patch coverage is 89.17836%
with 54 lines
in your changes missing coverage. Please review.
Project coverage is 91.91%. Comparing base (
5027417
) to head (14d0c8a
).
:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.
@psobolewskiPhD I tried to use the qdebounce from superqt but it does not exactly work as it is needed.
Works well. I think the tooltip is too long and technical?
No user has any idea what npe2api is or whatever.
How about just Refresh plugin listings
? or something like that? Honestly, refresh alone is probably clear enough.
Also, off topic: oh, pip 🙄 :
ERROR: Cannot install zarpaint==0.3.1 because these package versions have conflicting dependencies.
The conflict is caused by:
zarpaint 0.3.1 depends on napari>=0.4.11.dev80
The user requested (constraint) napari==0.5.0a2.dev720+gb00d7381b
Edit: I see now that Jaime requested this. 😬 I think it's just too much of an implementation detail... 🤷
@jaimergp what do you think. Maybe we could simplify the message a bit :) ?
Hi @Czaki thanks for the review. Will fixe your suggestions and try to increase coverage!
@Czaki added more testing as requested. I think we are in much better shape now :)
@jaimergp what do you think. Maybe we could simplify the message a bit :) ?
Yep, no strong feelings. Go ahead.
Hi there, gave this a check and seems like after installing a plugin the only way for it to show up over the installed list is to hit Refresh
a couple of times:
Maybe something in my setup is causing that (checking this on Windows) 🤔
Hi @dalthviz thanks for checking. Is this something that happens consistently every time or only some of the time? I have seen it sometimes on my end and I am trying to see what can be done.
Is this something that happens consistently every time or only some of the time?
I checked just a couple of times (launched napari, installed a plugin, saw the behavior, uninstalled the plugin and then relaunched napari to check again to record the GIF), so not sure if I can call that being able to reproduce consistenly 😅, but I will check again and define more detailed steps to reproduce :+1:
I also get the issue Daniel posted. Fresh napari dev install (python 3.12), installed this branch. Launch napari, open manager. Filter and install napari-threedee. Success, but doesn't show:
Clicking refresh once briefly showed all installed plugins, then blank again. Clicking refresh again shows the correct result of the filter:
Gave this another check and I would say I can reproduce the issue consistently:
Plugins > Install/Uninstall Plugins...
affinder
)
Available Plugins
section shows that is currently installingAvailable Plugins
section but doesn't show over the Installed Plugins
section
Available Plugins
section shows over the item count (415/416)
Refresh
once, it shows a popup related with some info about the need to restart napari after install np2 plugins to update UI related things (not totally sure if related to the work here but, thinking about it, shouldn't the popup be positioned inside the dialog? 🤔 ):
Refresh
again, now the Installed Plugins
section is updated and shows the new installed plugin itemIndeed, I can now reporduce, I will add a timer to refresh the installed plugins list after a while, not sure why it does not get pikcedup immediately. The popup not showing inside the dialog, and it also being modal, sounds weird. Will check the position when showing it and perhaps make it not modal.
Thanks for the reviews @dalthviz @psobolewskiPhD
@dalthviz @psobolewskiPhD I think I found the culprit and should now be working as expected. Could you test again?
@dalthviz I added a fix to https://github.com/napari/napari-plugin-manager/issues/61 since this PR was already fixing part of what you reported. Could you check :) ?
Cheers!
Gave another check and seems like the installed plugin is now visible after the install finishes without having to click Refresh
:+1: However, seems like now the popup after installation doesn't appear even after clicking Refresh
?:
Also seems like this fixes #61 :+1: See https://github.com/napari/napari-plugin-manager/issues/61#issuecomment-2220784637
👍 However, seems like now the popup after installation doesn't appear even after clicking Refresh?:
What popup 🤔 ?
Probably the one discussed at https://github.com/napari/napari-plugin-manager/pull/68?
What popup 🤔 ?
Probably the one discussed at https://github.com/napari/napari-plugin-manager/pull/68?
Exactly! Checking this I see that when uninstalling a plugin the popup keeps appearing but when installing it doesn't appear anymore:
@dalthviz I think there is a misunderstanding.
There is no way for us to know if a plugin is npe2 before installing and the code has only displayed the message when uninstalling because at that moment we actually know the type of plugin (npe2 in this case). There is a comment # TODO: NPE version unknown before installing
and there are actually 2 places where the popup is displayed, so the code is duplicated and wonky
# TODO: NPE version unknown before installing
if item.npe_version != 1 and action_name == InstallerActions.UNINSTALL:
# show warning pop up dialog
message = trans._(
'When installing/uninstalling npe2 plugins, you must '
'restart napari for UI changes to take effect.'
)
self._warn_dialog = WarnPopup(text=message)
delta_x = 75
global_point = widget.action_button.mapToGlobal(
widget.action_button.rect().topLeft()
)
global_point = QPoint(global_point.x() - delta_x, global_point.y())
self._warn_dialog.move(global_point)
The other popup you were seeing in your PR "after install" was not really appearing after install, but it was appearing after refresh which was the previous behavior after a package was instaled, the lists refreshed completely. Also the check in the code was looking at any discovered npe2 plugins, and if they were indeed discovered, then the popup appeared in a global generic position next to the error indicator. That was the previous behavior.
The code in your PR https://github.com/napari/napari-plugin-manager/pull/68/files#diff-c4a932d5522a5a713957223899c8991682affd18d838b197d3c508ea32333a1eR921
I will change the popup to not be modal and unify under a single helper method.
@dalthviz I made changes could check again :) ?
I am not sute why the test are now failing for py3.11 on mac.
SKIPPED [2] napari_plugin_manager/_tests/test_qt_plugin_dialog.py:480: pyside specific bug
================== 42 passed, 9 skipped, 4 warnings in 21.68s ==================
I do not see a fail or error on the test run :-\
I think we might need this constraint too:
pyside6_experimental = [
"PySide6 < 6.5 ; python_version < '3.12'"
]
Gave another check to this and seems like now the uninstall pop up gets placed under the application window? I'm seeing this:
Seems like not closing the pop up prevents continuing with the uninstall action. Maybe that's way the pop up was being shown with modal properties? 🤔
I do not see a fail or error on the test run :-\
Just in case, although the tests are passing, when finishing seems like the process is segfaulting (exit 11 - https://github.com/napari/napari-plugin-manager/actions/runs/9904959061/job/27363858135?pr=45#step:7:3225). As mentioned above, constraining the PySide6 version could help :+1:
Thanks for the review @dalthviz pushing a fix
So it seems that the latest napari dev is using numpy 2 for py 3.11 and above and that is causing problems here. I added numpy<2 in the package dependencies just to check...and tests are not segfaulting anymore.
What do you think we should do @jaimergp ?
Hm, that's interesting. AFAIK we are not using numpy
ourselves, are we? Maybe @Czaki or @jni know more about this tricky pyside x numpy interaction 😬
This message was appearing when running the tests
pytest -v --color=yes --cov=napari_plugin_manager --cov-report=xml [tox/tox_env/api.py:426]
A module that was compiled using NumPy 1.x cannot be run in
NumPy 2.0.0 as it may crash. To support both 1.x and 2.x
versions of NumPy, modules must be compiled with NumPy 2.0.
Some module may need to rebuild instead e.g. with 'pybind11>=2.12'.
If you are a user of the module, the easiest solution will be to
downgrade to 'numpy<2' or try to upgrade the affected module.
We expect that some modules will need time to support NumPy 2.
Created https://github.com/napari/napari-plugin-manager/issues/72 to track the problem, in the meantime this can be merged as soon as all tests pass!
Thanks everyone for your comments
Maybe we should use constraints files from napari/napari for the test env? Here's how it's used in napari/docs https://github.com/napari/docs/blob/9dabb9e6598d286851cd97abf12a9cb858f8b65b/.github/workflows/build_and_deploy.yml#L54-L59
I'd like to avoid the numpy <2 pin. I think the issue can be resolved with the PySide6 pin. napari/PySide6 is both experimental and rather broken, so I'd not have that be driving stuff like numpy version.
Here's the deny list, which include a lot of PySide6: https://github.com/napari/napari/blob/5e685a20c9563a42576ead39a4f0286337a52ce6/resources/constraints/version_denylist.txt#L3
Hi @psobolewskiPhD thanks for the info, let me push something with what you suggest to check.
However there is a problem with numpy>2 at the moment as displayed by the error message
Yeah, I saw that. But napari tests do pass with the constraints. So unless it's something specific the manager uses, I think the constraints should get this to pass too? Looked promising here: https://github.com/napari/napari-plugin-manager/pull/75
Since we are skipping tests for py3.9 on the github workflow anyway I only added
PySide6: PySide6 != 6.4.3, !=6.5.0, !=6.5.1, !=6.5.1.1, !=6.5.2, != 6.5.3, != 6.6.0, != 6.6.1, != 6.6.2 ; python_version >= '3.10' and python_version < '3.12'
Seems to be working fine now :) thanks for the help @psobolewskiPhD
Merging after tests pass, fingers crossed 🤞🏼
@dalthviz could you check that the modal pop on install and uninstall is working as expected. I think that was the only missing part (besides the pyside6 shenanigans)
Gave this another check and seems like now the popup for install and uninstall are being placed along side the item :+1:
However, using the action to show the dialog shows the following traceback:
---------------------------------------------------------------------------
AttributeError Traceback (most recent call last)
File ~\anaconda3\envs\napari-dev\lib\site-packages\app_model\backends\qt\_qaction.py:55, in QCommandAction._on_triggered(self=QMenuItemAction(MenuItem(when=Expr.parse('True')...tle=None, toggled=None), alt=None), app='napari'), checked=False)
51 def _on_triggered(self, checked: bool) -> None:
52 # execute_command returns a Future, for the sake of eventually being
53 # asynchronous without breaking the API. For now, we call result()
54 # to raise any exceptions.
---> 55 self._app.commands.execute_command(self._command_id).result()
self._command_id = 'napari.window.plugins.plugin_install_dialog'
self = QMenuItemAction(MenuItem(when=Expr.parse('True'), group='1_plugins', order=1.0, command=CommandRule(id='napari.window.plugins.plugin_install_dialog', title='Install/Uninstall Plugins...', category=None, tooltip=None, status_tip=None, icon=None, icon_visible_in_menu=True, enablement=None, short_title=None, toggled=None), alt=None), app='napari')
self._app = Application('napari')
File ~\anaconda3\envs\napari-dev\lib\site-packages\app_model\registries\_commands_reg.py:245, in CommandsRegistry.execute_command(self=<CommandsRegistry at 0x1a952ccb910 (148 commands)>, id='napari.window.plugins.plugin_install_dialog', execute_asynchronously=False, *args=(), **kwargs={})
241 except Exception as e:
242 if self._raise_synchronous_exceptions:
243 # note, the caller of this function can also achieve this by
244 # calling `future.result()` on the returned future object.
--> 245 raise e
246 future.set_exception(e)
248 return future
File ~\anaconda3\envs\napari-dev\lib\site-packages\app_model\registries\_commands_reg.py:240, in CommandsRegistry.execute_command(self=<CommandsRegistry at 0x1a952ccb910 (148 commands)>, id='napari.window.plugins.plugin_install_dialog', execute_asynchronously=False, *args=(), **kwargs={})
238 future: Future = Future()
239 try:
--> 240 future.set_result(cmd(*args, **kwargs))
future = <Future at 0x1a9626881f0 state=pending>
cmd = <function _show_plugin_install_dialog at 0x000001A95DB9A170>
args = ()
kwargs = {}
241 except Exception as e:
242 if self._raise_synchronous_exceptions:
243 # note, the caller of this function can also achieve this by
244 # calling `future.result()` on the returned future object.
File ~\anaconda3\envs\napari-dev\lib\site-packages\in_n_out\_store.py:934, in Store.inject_processors.<locals>._deco.<locals>._exec(*args=(), **kwargs={})
932 @wraps(func)
933 def _exec(*args: P.args, **kwargs: P.kwargs) -> R:
--> 934 result = func(*args, **kwargs)
func = <function _show_plugin_install_dialog at 0x000001A95DB9BD00>
args = ()
kwargs = {}
935 if result is not None:
936 self.process(
937 result,
938 type_hint=type_hint,
(...)
941 _funcname=getattr(func, "__qualname__", str(func)),
942 )
File ~\anaconda3\envs\napari-dev\lib\site-packages\in_n_out\_store.py:804, in Store.inject.<locals>._inner.<locals>._exec(*args=(), **kwargs={})
797 logger.debug(
798 " Calling %s with %r (injected %r)",
799 _fname,
800 bound.arguments,
801 _injected_names,
802 )
803 try:
--> 804 result = func(**bound.arguments)
bound = <BoundArguments (window=<napari._qt.qt_main_window.Window object at 0x000001A952D1E800>)>
func = <function _show_plugin_install_dialog at 0x000001A95AD94280>
bound.arguments = {'window': <napari._qt.qt_main_window.Window object at 0x000001A952D1E800>}
805 except TypeError as e:
806 if "missing" not in e.args[0]:
File E:\Acer\Documentos\Quansight\Napari\napari\napari\_qt\_qapp_model\qactions\_plugins.py:35, in _show_plugin_install_dialog(window=<napari._qt.qt_main_window.Window object>)
30 # TODO: Once menu contributions supported, `napari_plugin_manager` should be
31 # amended to be a napari plugin and simply add this menu item itself.
32 # This callback is only used when this package is available, thus we do not check
33 from napari_plugin_manager.qt_plugin_dialog import QtPluginDialog
---> 35 QtPluginDialog(window._qt_window).exec_()
QtPluginDialog = <class 'napari_plugin_manager.qt_plugin_dialog.QtPluginDialog'>
window._qt_window = <napari._qt.qt_main_window._QtMainWindow object at 0x000001A954209C60>
window = <napari._qt.qt_main_window.Window object at 0x000001A952D1E800>
File E:\Acer\Documentos\Quansight\Napari\napari otros\gonzalo\napari-plugin-manager\napari_plugin_manager\qt_plugin_dialog.py:1326, in QtPluginDialog.exec_(self=<napari_plugin_manager.qt_plugin_dialog.QtPluginDialog object>)
1323 plugin_dialog.setModal(True)
1324 plugin_dialog.show()
-> 1326 if self._first_open:
self = <napari_plugin_manager.qt_plugin_dialog.QtPluginDialog object at 0x000001A962694280>
1327 stylesheet = get_current_stylesheet([STYLES_PATH])
1328 self.setStyleSheet(stylesheet)
AttributeError: 'QtPluginDialog' object has no attribute '_first_open'
INFO: Plugin Manager: process completed
Maybe there is a need to initialize the _first_open
attribute early?
Another thing I noticed now is that since is possible to close the plugin dialog with in progress tasks, you can end up showing the pop up after installation when the dialog is closed, so you can see something like:
Checking, this behavior is also present with latest main, but maybe could be worthy to take care of that here too? Something like oviating the pop up logic in that case and instead just show with the napari notification the info about the need to restart napari for UI changes to take effect along side the process completed message? 🤔
Checking, this behavior is also present with latest main, but maybe could be worthy to take care of that here too? Something like oviating the pop up logic in that case and instead just show with the napari notification the info about the need to restart napari for UI changes to take effect along side the process completed message? 🤔
Great suggestion @dalthviz I had not thought about that but yes! Will push a fix
Also fixing the bug you found!
I think the errors are now fixed @dalthviz , next round :)
Gave this another check and seems like things are working as expected 🎉
Merging! Thanks for the reviews
Fixes #29 Fixes #61
Adds a
Refresh
button and tooltip that clears cache, requerys npe2api and repopulates the listsPerforms actions in place, adding or removing items between lists instead of refreshing them