Update lists without refreshing and other fixes #45
Conversation
goanpeca
force-pushed
the
enh/remove-instead-of-refresh
branch
from
22e1f36 to
2491092
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #45 +/- ##
==========================================
+ Coverage 87.11% 91.91% +4.79%
==========================================
Files 9 10 +1
Lines 1366 1669 +303
==========================================
+ Hits 1190 1534 +344
+ Misses 176 135 -41 ☔ View full report in Codecov by Sentry. |
goanpeca
force-pushed
the
enh/remove-instead-of-refresh
branch
from
be697d2 to
301194d
Compare
goanpeca
changed the title
|
@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? Also, off topic: oh, pip 🙄 : 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 :) ? |
There was a problem hiding this comment.
Initial review. I would prefer to see much better test coverage, as it will be not often tested maually by core devs.
I also found setUpdatesEnabled method of QWidget https://doc.qt.io/qt-6/qwidget.html#updatesEnabled-prop that may allow increasing batch size so speedup loading plugins.
|
Hi @Czaki thanks for the review. Will fixe your suggestions and try to increase coverage! |
goanpeca
force-pushed
the
enh/remove-instead-of-refresh
branch
3 times, most recently
from
725bd36 to
4a9aa84
Compare
|
@Czaki added more testing as requested. I think we are in much better shape now :) |
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
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. |
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 👍 |
|
I also get the issue Daniel posted. Clicking refresh once briefly showed all installed plugins, then blank again. |
|
Gave this another check and I would say I can reproduce the issue consistently: Steps/Behavior
Setup info
|
|
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 ? |
|
This message was appearing when running the tests |
goanpeca
force-pushed
the
enh/remove-instead-of-refresh
branch
from
e2a6374 to
da9c2b2
Compare
|
Created #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? |
|
I'd like to avoid the numpy <2 pin. I think the issue can be resolved with the PySide6 pin. |
|
Here's the deny list, which include a lot of PySide6: |
|
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? |
goanpeca
force-pushed
the
enh/remove-instead-of-refresh
branch
from
04c0b0c to
38276d0
Compare
|
Since we are skipping tests for py3.9 on the github workflow anyway I only added
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 👍
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 completedMaybe there is a need to initialize the |
|
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? 🤔 |
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
Refreshbutton and tooltip that clears cache, requerys npe2api and repopulates the listsPerforms actions in place, adding or removing items between lists instead of refreshing them