From 9c7aecf326765ccd6b84ca51f3a3eb90269d42c7 Mon Sep 17 00:00:00 2001 From: dalthviz <16781833+dalthviz@users.noreply.github.com> Date: Tue, 5 Nov 2024 11:43:05 -0500 Subject: [PATCH 1/9] Add a warning dialog when triggering an install action using PyPI source on a bundle installation --- napari_plugin_manager/qt_plugin_dialog.py | 23 ++++++++++++++++++++++- 1 file changed, 22 insertions(+), 1 deletion(-) diff --git a/napari_plugin_manager/qt_plugin_dialog.py b/napari_plugin_manager/qt_plugin_dialog.py index 4706f1ca..0de42d12 100644 --- a/napari_plugin_manager/qt_plugin_dialog.py +++ b/napari_plugin_manager/qt_plugin_dialog.py @@ -43,6 +43,7 @@ QListWidget, QListWidgetItem, QMenu, + QMessageBox, QPushButton, QSizePolicy, QSplitter, @@ -528,6 +529,26 @@ def _action_requested(self): if self.action_button.objectName() == 'install_button' else InstallerActions.UNINSTALL ) + if ( + tool == InstallerTools.PIP + and action == InstallerActions.INSTALL + and ON_BUNDLE + ): + button_clicked = QMessageBox.warning( + self, + trans._('PyPI installation on bundle'), + trans._( + 'Installing from PyPI does not take into account existing installed packages, ' + 'so it can break existing installations. ' + 'If this happens the only solution is to reinstall the bundle.\n\n' + 'Are you sure you want to install from PyPI?' + ), + buttons=QMessageBox.StandardButton.Ok + | QMessageBox.StandardButton.Cancel, + defaultButton=QMessageBox.StandardButton.Cancel, + ) + if button_clicked != QMessageBox.StandardButton.Ok: + return self.actionRequested.emit(self.item, self.name, action, version, tool) def _update_requested(self): @@ -1190,7 +1211,7 @@ def _setup_ui(self): self.working_indicator.setMovie(mov) mov.start() - visibility_direct_entry = not running_as_constructor_app() + visibility_direct_entry = not ON_BUNDLE self.direct_entry_edit = QLineEdit(self) self.direct_entry_edit.installEventFilter(self) self.direct_entry_edit.returnPressed.connect(self._install_packages) From 4b787c0a9a86ce6ba4476858522dc47d45985509 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Althviz=20Mor=C3=A9?= <16781833+dalthviz@users.noreply.github.com> Date: Thu, 28 Nov 2024 09:27:29 -0500 Subject: [PATCH 2/9] Apply suggestions from code review Co-authored-by: jaimergp --- napari_plugin_manager/base_qt_plugin_dialog.py | 2 +- napari_plugin_manager/qt_plugin_dialog.py | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/napari_plugin_manager/base_qt_plugin_dialog.py b/napari_plugin_manager/base_qt_plugin_dialog.py index 293192a4..19a47e15 100644 --- a/napari_plugin_manager/base_qt_plugin_dialog.py +++ b/napari_plugin_manager/base_qt_plugin_dialog.py @@ -601,7 +601,7 @@ def _on_enabled_checkbox(self, state: Qt.CheckState) -> None: """ raise NotImplementedError - def _on_bundle(self) -> bool: + def _warn_pypi_install(self) -> bool: """ If the current installation comes from a bundle/standalone approach or not. diff --git a/napari_plugin_manager/qt_plugin_dialog.py b/napari_plugin_manager/qt_plugin_dialog.py index 1834aeba..f20912e9 100644 --- a/napari_plugin_manager/qt_plugin_dialog.py +++ b/napari_plugin_manager/qt_plugin_dialog.py @@ -127,7 +127,7 @@ def _on_enabled_checkbox(self, state: int): ) return - def _on_bundle(self): + def _warn_pypi_install(self): return running_as_constructor_app() From fc9afc80f0edf45de003c779ee1266e8e84e5993 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Althviz=20Mor=C3=A9?= <16781833+dalthviz@users.noreply.github.com> Date: Thu, 28 Nov 2024 11:54:54 -0500 Subject: [PATCH 3/9] Apply suggestions from code review Co-authored-by: Peter Sobolewski <76622105+psobolewskiPhD@users.noreply.github.com> --- napari_plugin_manager/qt_plugin_dialog.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/napari_plugin_manager/qt_plugin_dialog.py b/napari_plugin_manager/qt_plugin_dialog.py index f20912e9..e562fd94 100644 --- a/napari_plugin_manager/qt_plugin_dialog.py +++ b/napari_plugin_manager/qt_plugin_dialog.py @@ -128,7 +128,7 @@ def _on_enabled_checkbox(self, state: int): return def _warn_pypi_install(self): - return running_as_constructor_app() + return running_as_constructor_app() or is_conda_package('napari') class QPluginList(BaseQPluginList): From 7e995b10b5c2be68e9a27e5209b17c62f39e5d70 Mon Sep 17 00:00:00 2001 From: "pre-commit-ci[bot]" <66853113+pre-commit-ci[bot]@users.noreply.github.com> Date: Thu, 28 Nov 2024 16:55:03 +0000 Subject: [PATCH 4/9] [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci --- napari_plugin_manager/qt_plugin_dialog.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/napari_plugin_manager/qt_plugin_dialog.py b/napari_plugin_manager/qt_plugin_dialog.py index e562fd94..1d4e3623 100644 --- a/napari_plugin_manager/qt_plugin_dialog.py +++ b/napari_plugin_manager/qt_plugin_dialog.py @@ -128,7 +128,7 @@ def _on_enabled_checkbox(self, state: int): return def _warn_pypi_install(self): - return running_as_constructor_app() or is_conda_package('napari') + return running_as_constructor_app() or is_conda_package('napari') class QPluginList(BaseQPluginList): From cb7e0dc45f41651d9dc87744a89ed86cf41c73c0 Mon Sep 17 00:00:00 2001 From: dalthviz <16781833+dalthviz@users.noreply.github.com> Date: Thu, 28 Nov 2024 12:20:23 -0500 Subject: [PATCH 5/9] Update docstring and a couple of missing changes to follow the suggestions --- napari_plugin_manager/base_qt_plugin_dialog.py | 7 ++++--- napari_plugin_manager/qt_plugin_dialog.py | 1 + 2 files changed, 5 insertions(+), 3 deletions(-) diff --git a/napari_plugin_manager/base_qt_plugin_dialog.py b/napari_plugin_manager/base_qt_plugin_dialog.py index 19a47e15..20e744bf 100644 --- a/napari_plugin_manager/base_qt_plugin_dialog.py +++ b/napari_plugin_manager/base_qt_plugin_dialog.py @@ -603,11 +603,12 @@ def _on_enabled_checkbox(self, state: Qt.CheckState) -> None: def _warn_pypi_install(self) -> bool: """ - If the current installation comes from a bundle/standalone approach or not. + If the current installation should warn that a package from PyPI is going + to be installed. Returns ------- - This should return a `bool`, `True` if under a bundle like installation, `False` + This should return a `bool`, `True` if a warning should be shown, `False` otherwise. """ raise NotImplementedError @@ -630,7 +631,7 @@ def _action_requested(self): if ( tool == InstallerTools.PIP and action == InstallerActions.INSTALL - and self._on_bundle() + and self._warn_pypi_install() ): button_clicked = QMessageBox.warning( self, diff --git a/napari_plugin_manager/qt_plugin_dialog.py b/napari_plugin_manager/qt_plugin_dialog.py index 1d4e3623..42a2d6b4 100644 --- a/napari_plugin_manager/qt_plugin_dialog.py +++ b/napari_plugin_manager/qt_plugin_dialog.py @@ -33,6 +33,7 @@ from napari_plugin_manager.qt_package_installer import ( InstallerActions, ) +from napari_plugin_manager.utils import is_conda_package # Scaling factor for each list widget item when expanding. STYLES_PATH = Path(__file__).parent / 'styles.qss' From 3260704a78c05fa13c4ed9c772382b74c4c9953c Mon Sep 17 00:00:00 2001 From: dalthviz <16781833+dalthviz@users.noreply.github.com> Date: Tue, 3 Dec 2024 11:49:55 -0500 Subject: [PATCH 6/9] Initial checkbox dismiss implementation --- .../base_qt_plugin_dialog.py | 29 ++++++++++++++----- napari_plugin_manager/qt_plugin_dialog.py | 4 ++- 2 files changed, 25 insertions(+), 8 deletions(-) diff --git a/napari_plugin_manager/base_qt_plugin_dialog.py b/napari_plugin_manager/base_qt_plugin_dialog.py index 20e744bf..e8266f88 100644 --- a/napari_plugin_manager/base_qt_plugin_dialog.py +++ b/napari_plugin_manager/base_qt_plugin_dialog.py @@ -60,6 +60,7 @@ CONDA = 'Conda' PYPI = 'PyPI' +DISMISS_WARN_PYPI_INSTALL_DLG = False class PackageMetadataProtocol(Protocol): @@ -621,6 +622,7 @@ def _cancel_requested(self): ) def _action_requested(self): + global DISMISS_WARN_PYPI_INSTALL_DLG version = self.version_choice_dropdown.currentText() tool = self.get_installer_tool() action = ( @@ -632,20 +634,33 @@ def _action_requested(self): tool == InstallerTools.PIP and action == InstallerActions.INSTALL and self._warn_pypi_install() + and not DISMISS_WARN_PYPI_INSTALL_DLG ): - button_clicked = QMessageBox.warning( - self, - self._trans('PyPI installation on bundle'), + warn_msgbox = QMessageBox(self) + warn_msgbox.setWindowTitle( + self._trans('PyPI installation on bundle') + ) + warn_msgbox.setText( self._trans( 'Installing from PyPI does not take into account existing installed packages, ' 'so it can break existing installations. ' 'If this happens the only solution is to reinstall the bundle.\n\n' 'Are you sure you want to install from PyPI?' - ), - buttons=QMessageBox.StandardButton.Ok - | QMessageBox.StandardButton.Cancel, - defaultButton=QMessageBox.StandardButton.Cancel, + ) + ) + warn_checkbox = QCheckBox( + self._trans( + "Don't show this message again in the current session" + ) + ) + warn_msgbox.setCheckBox(warn_checkbox) + warn_msgbox.setIcon(QMessageBox.Icon.Warning) + warn_msgbox.setStandardButtons( + QMessageBox.StandardButton.Ok + | QMessageBox.StandardButton.Cancel ) + button_clicked = warn_msgbox.exec_() + DISMISS_WARN_PYPI_INSTALL_DLG = warn_checkbox.isChecked() if button_clicked != QMessageBox.StandardButton.Ok: return self.actionRequested.emit(self.item, self.name, action, version, tool) diff --git a/napari_plugin_manager/qt_plugin_dialog.py b/napari_plugin_manager/qt_plugin_dialog.py index 42a2d6b4..a251a1e5 100644 --- a/napari_plugin_manager/qt_plugin_dialog.py +++ b/napari_plugin_manager/qt_plugin_dialog.py @@ -129,7 +129,9 @@ def _on_enabled_checkbox(self, state: int): return def _warn_pypi_install(self): - return running_as_constructor_app() or is_conda_package('napari') + return running_as_constructor_app() or is_conda_package( + 'napari' + ) # or True class QPluginList(BaseQPluginList): From b503f7efa5aaa11ac1afb8007a3543e5f9c07440 Mon Sep 17 00:00:00 2001 From: dalthviz <16781833+dalthviz@users.noreply.github.com> Date: Tue, 3 Dec 2024 12:20:42 -0500 Subject: [PATCH 7/9] Action validation method approach --- .../base_qt_plugin_dialog.py | 47 ++++--------------- napari_plugin_manager/qt_plugin_dialog.py | 39 +++++++++++++++ 2 files changed, 47 insertions(+), 39 deletions(-) diff --git a/napari_plugin_manager/base_qt_plugin_dialog.py b/napari_plugin_manager/base_qt_plugin_dialog.py index e8266f88..7b9b09c8 100644 --- a/napari_plugin_manager/base_qt_plugin_dialog.py +++ b/napari_plugin_manager/base_qt_plugin_dialog.py @@ -38,7 +38,6 @@ QListWidget, QListWidgetItem, QMenu, - QMessageBox, QPushButton, QSizePolicy, QSplitter, @@ -60,7 +59,6 @@ CONDA = 'Conda' PYPI = 'PyPI' -DISMISS_WARN_PYPI_INSTALL_DLG = False class PackageMetadataProtocol(Protocol): @@ -602,14 +600,16 @@ def _on_enabled_checkbox(self, state: Qt.CheckState) -> None: """ raise NotImplementedError - def _warn_pypi_install(self) -> bool: + def _action_validation(self, tool, action) -> bool: """ - If the current installation should warn that a package from PyPI is going + Validate if the current action should be done or not. + + As an example you could warn that a package from PyPI is going to be installed. Returns ------- - This should return a `bool`, `True` if a warning should be shown, `False` + This should return a `bool`, `True` if the action should proceed, `False` otherwise. """ raise NotImplementedError @@ -622,7 +622,6 @@ def _cancel_requested(self): ) def _action_requested(self): - global DISMISS_WARN_PYPI_INSTALL_DLG version = self.version_choice_dropdown.currentText() tool = self.get_installer_tool() action = ( @@ -630,40 +629,10 @@ def _action_requested(self): if self.action_button.objectName() == 'install_button' else InstallerActions.UNINSTALL ) - if ( - tool == InstallerTools.PIP - and action == InstallerActions.INSTALL - and self._warn_pypi_install() - and not DISMISS_WARN_PYPI_INSTALL_DLG - ): - warn_msgbox = QMessageBox(self) - warn_msgbox.setWindowTitle( - self._trans('PyPI installation on bundle') - ) - warn_msgbox.setText( - self._trans( - 'Installing from PyPI does not take into account existing installed packages, ' - 'so it can break existing installations. ' - 'If this happens the only solution is to reinstall the bundle.\n\n' - 'Are you sure you want to install from PyPI?' - ) + if self._action_validation(tool, action): + self.actionRequested.emit( + self.item, self.name, action, version, tool ) - warn_checkbox = QCheckBox( - self._trans( - "Don't show this message again in the current session" - ) - ) - warn_msgbox.setCheckBox(warn_checkbox) - warn_msgbox.setIcon(QMessageBox.Icon.Warning) - warn_msgbox.setStandardButtons( - QMessageBox.StandardButton.Ok - | QMessageBox.StandardButton.Cancel - ) - button_clicked = warn_msgbox.exec_() - DISMISS_WARN_PYPI_INSTALL_DLG = warn_checkbox.isChecked() - if button_clicked != QMessageBox.StandardButton.Ok: - return - self.actionRequested.emit(self.item, self.name, action, version, tool) def _update_requested(self): version = self.version_choice_dropdown.currentText() diff --git a/napari_plugin_manager/qt_plugin_dialog.py b/napari_plugin_manager/qt_plugin_dialog.py index a251a1e5..3daf4ae4 100644 --- a/napari_plugin_manager/qt_plugin_dialog.py +++ b/napari_plugin_manager/qt_plugin_dialog.py @@ -19,6 +19,7 @@ from qtpy.QtGui import ( QMovie, ) +from qtpy.QtWidgets import QCheckBox, QMessageBox from napari_plugin_manager.base_qt_plugin_dialog import ( BasePluginListItem, @@ -32,11 +33,13 @@ ) from napari_plugin_manager.qt_package_installer import ( InstallerActions, + InstallerTools, ) from napari_plugin_manager.utils import is_conda_package # Scaling factor for each list widget item when expanding. STYLES_PATH = Path(__file__).parent / 'styles.qss' +DISMISS_WARN_PYPI_INSTALL_DLG = False def _show_message(widget): @@ -133,6 +136,42 @@ def _warn_pypi_install(self): 'napari' ) # or True + def _action_validation(self, tool, action): + global DISMISS_WARN_PYPI_INSTALL_DLG + if ( + tool == InstallerTools.PIP + and action == InstallerActions.INSTALL + and self._warn_pypi_install() + and not DISMISS_WARN_PYPI_INSTALL_DLG + ): + warn_msgbox = QMessageBox(self) + warn_msgbox.setWindowTitle( + self._trans('PyPI installation on bundle/conda') + ) + warn_msgbox.setText( + self._trans( + 'Installing from PyPI does not take into account existing installed packages, ' + 'so it can break existing installations. ' + 'If this happens the only solution is to reinstall the bundle/create a new conda environment.\n\n' + 'Are you sure you want to install from PyPI?' + ) + ) + warn_checkbox = QCheckBox( + self._trans( + "Don't show this message again in the current session" + ) + ) + warn_msgbox.setCheckBox(warn_checkbox) + warn_msgbox.setIcon(QMessageBox.Icon.Warning) + warn_msgbox.setStandardButtons( + QMessageBox.StandardButton.Ok + | QMessageBox.StandardButton.Cancel + ) + button_clicked = warn_msgbox.exec_() + DISMISS_WARN_PYPI_INSTALL_DLG = warn_checkbox.isChecked() + if button_clicked != QMessageBox.StandardButton.Ok: + return + class QPluginList(BaseQPluginList): From a9ce3556071cbfc4b521ebdda8082949e175ef26 Mon Sep 17 00:00:00 2001 From: dalthviz <16781833+dalthviz@users.noreply.github.com> Date: Tue, 3 Dec 2024 13:19:18 -0500 Subject: [PATCH 8/9] Missing return in the action validation approach --- napari_plugin_manager/qt_plugin_dialog.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/napari_plugin_manager/qt_plugin_dialog.py b/napari_plugin_manager/qt_plugin_dialog.py index 3daf4ae4..9b5dd795 100644 --- a/napari_plugin_manager/qt_plugin_dialog.py +++ b/napari_plugin_manager/qt_plugin_dialog.py @@ -170,7 +170,8 @@ def _action_validation(self, tool, action): button_clicked = warn_msgbox.exec_() DISMISS_WARN_PYPI_INSTALL_DLG = warn_checkbox.isChecked() if button_clicked != QMessageBox.StandardButton.Ok: - return + return False + return True class QPluginList(BaseQPluginList): From 63adb5bacdda22bc6fc8ff52fd3d000784b54e49 Mon Sep 17 00:00:00 2001 From: dalthviz <16781833+dalthviz@users.noreply.github.com> Date: Tue, 3 Dec 2024 15:07:43 -0500 Subject: [PATCH 9/9] Testing --- .../_tests/test_qt_plugin_dialog.py | 30 +++++++++++++++++++ 1 file changed, 30 insertions(+) diff --git a/napari_plugin_manager/_tests/test_qt_plugin_dialog.py b/napari_plugin_manager/_tests/test_qt_plugin_dialog.py index 08071616..3c1a8865 100644 --- a/napari_plugin_manager/_tests/test_qt_plugin_dialog.py +++ b/napari_plugin_manager/_tests/test_qt_plugin_dialog.py @@ -12,6 +12,7 @@ from napari.utils.translations import trans from qtpy.QtCore import QMimeData, QPointF, Qt, QUrl from qtpy.QtGui import QDropEvent +from qtpy.QtWidgets import QMessageBox if qtpy.API_NAME == 'PySide2' and sys.version_info[:2] > (3, 10): pytest.skip( @@ -484,6 +485,35 @@ def test_installs(qtbot, tmp_virtualenv, plugin_dialog, request): qtbot.wait(5000) +@pytest.mark.parametrize( + "message_return", + [QMessageBox.StandardButton.Cancel, QMessageBox.StandardButton.Ok], +) +def test_install_pypi_constructor( + qtbot, tmp_virtualenv, plugin_dialog, request, message_return +): + if "no-constructor" in request.node.name: + pytest.skip( + reason="This test is only relevant for constructor-based installs" + ) + + plugin_dialog.set_prefix(str(tmp_virtualenv)) + plugin_dialog.search('requests') + qtbot.wait(500) + item = plugin_dialog.available_list.item(0) + widget = plugin_dialog.available_list.itemWidget(item) + with patch.object(qt_plugin_dialog.QMessageBox, "exec_") as mock: + mock.return_value = message_return + if message_return == QMessageBox.StandardButton.Ok: + with qtbot.waitSignal( + plugin_dialog.installer.processFinished, timeout=60_000 + ): + widget.action_button.click() + else: + widget.action_button.click() + assert mock.called + + def test_cancel(qtbot, tmp_virtualenv, plugin_dialog, request): if "[constructor]" in request.node.name: pytest.skip(