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.

Sign up for GitHub

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

Add a warning message when triggering an install action using PyPI source on a bundle/conda installation #111

Merged
merged 11 commits into from
Dec 4, 2024
Merged

Add a warning message when triggering an install action using PyPI source on a bundle/conda installation #111

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
9c7aecf
Add a warning dialog when triggering an install action using PyPI sou…
dalthviz Nov 5, 2024
a9f6f81
Merge branch 'main' into issue_110_warning_opt
dalthviz Nov 27, 2024
807e313
Merge branch 'main' into issue_110_warning_opt
dalthviz Nov 27, 2024
4b787c0
Apply suggestions from code review
dalthviz Nov 28, 2024
fc9afc8
Apply suggestions from code review
dalthviz Nov 28, 2024
7e995b1
[pre-commit.ci] auto fixes from pre-commit.com hooks
pre-commit-ci[bot] Nov 28, 2024
cb7e0dc
Update docstring and a couple of missing changes to follow the sugges…
dalthviz Nov 28, 2024
3260704
Initial checkbox dismiss implementation
dalthviz Dec 3, 2024
b503f7e
Action validation method approach
dalthviz Dec 3, 2024
a9ce355
Missing return in the action validation approach
dalthviz Dec 3, 2024
63adb5b
Testing
dalthviz Dec 3, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
32 changes: 32 additions & 0 deletions napari_plugin_manager/base_qt_plugin_dialog.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@
QListWidget,
QListWidgetItem,
QMenu,
QMessageBox,
QPushButton,
QSizePolicy,
QSplitter,
Expand Down Expand Up @@ -600,6 +601,17 @@ def _on_enabled_checkbox(self, state: Qt.CheckState) -> None:
"""
raise NotImplementedError

def _warn_pypi_install(self) -> bool:
"""
If the current installation comes from a bundle/standalone approach or not.

Returns
-------
This should return a `bool`, `True` if under a bundle like installation, `False`
otherwise.
"""
raise NotImplementedError

def _cancel_requested(self):
version = self.version_choice_dropdown.currentText()
tool = self.get_installer_tool()
Expand All @@ -615,6 +627,26 @@ def _action_requested(self):
if self.action_button.objectName() == 'install_button'
else InstallerActions.UNINSTALL
)
if (
tool == InstallerTools.PIP
and action == InstallerActions.INSTALL
and self._on_bundle()
Copy link
Contributor

@jaimergp jaimergp Nov 28, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some folks might not want this behavior, on bundle or not. Let's turn this third condition into self._warn_pypi_install() and then users can choose what to do about it in their subclasses. In our case, _warn_pypi will call running_as_constructor_app().

Copy link
Member Author

@dalthviz dalthviz Nov 28, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thinking about how to generalize the warning dialog text, maybe another approach that could be done is to instead add a _action_validation method that should be implemented returning a boolean? So something like:

    def _action_validation(self, tool, action) -> bool:
        raise NotImplementedError

    def _action_requested(self):
        version = self.version_choice_dropdown.currentText()
        tool = self.get_installer_tool()
        action = (
            InstallerActions.INSTALL
            if self.action_button.objectName() == 'install_button'
            else InstallerActions.UNINSTALL
        )
        if self._action_validation(tool, action):
            self.actionRequested.emit(self.item, self.name, action, version, tool)

Then the napari implementation would be something like:

    def _action_validation(self, tool, action) -> bool:
        if (
            tool == InstallerTools.PIP
            and action == InstallerActions.INSTALL
            and (running_as_constructor_app() or is_conda_package('napari'))
        ):
            button_clicked = QMessageBox.warning(
                self,
                self._trans('PyPI installation on bundle'),
                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,
            )
            if button_clicked != QMessageBox.StandardButton.Ok:
                return False
        return True

What do you think @jaimergp ?

):
button_clicked = QMessageBox.warning(
self,
self._trans('PyPI installation on bundle'),
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,
)
if button_clicked != QMessageBox.StandardButton.Ok:
return
self.actionRequested.emit(self.item, self.name, action, version, tool)

def _update_requested(self):
Expand Down
3 changes: 3 additions & 0 deletions napari_plugin_manager/qt_plugin_dialog.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -127,6 +127,9 @@ def _on_enabled_checkbox(self, state: int):
)
return

def _warn_pypi_install(self):
return running_as_constructor_app()
dalthviz marked this conversation as resolved.
Show resolved Hide resolved


class QPluginList(BaseQPluginList):

Expand Down
Loading