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

[dalthviz]

Part of #110

Explores the option to add a warning message so users are better informed of the dangers of using the PyPI source option on bundle installations (mixing pip + conda). This particular implementation uses a dialog to show the warning. A preview:

Note: Check locally the warning message triggering and dialog layout as seen from a bundle installation, you need to replace/change

napari-plugin-manager/napari_plugin_manager/qt_plugin_dialog.py

Lines 131 to 134 in 3260704

	def _warn_pypi_install(self):
	return running_as_constructor_app() or is_conda_package(
	'napari'
	) # or True

and uncommenting the # or True part

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 93.53%. Comparing base (cccd4d1) to head (63adb5b).
Report is 1 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #111      +/-   ##
==========================================
+ Coverage   93.46%   93.53%   +0.07%     
==========================================
  Files          11       11              
  Lines        1943     1981      +38     
==========================================
+ Hits         1816     1853      +37     
- Misses        127      128       +1     

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

[goanpeca]

Hi @dalthviz is this good to go?

All looks good :)

[dalthviz]

Hi there, technically yes but still not sure if whether this or #112 is the preferred approach to improve things in regards of issue #110 🤔 Will post a message over the Zulip discussion pointing to the PRs to see if we can gather more feedback 👍

[psobolewskiPhD]

Technically pip will take into account existing site-packages right? but not necessarily namespace issues.
Plus if there are plugins that have napari[all]--which there are--that will likely break the bundle.

[jaimergp]

Let's focus on this one to close #110.

[dalthviz]

Should #113 be merged first so then I update/redo this on top of that work @jaimergp @goanpeca ?

[jaimergp]

All yours! :)

[jaimergp]

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().

[dalthviz]

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 ?

[psobolewskiPhD]

I added a suggestion to consider making the warning show when napari is from conda-forge.
I've broken envs using the plugin installer before like this, so it may be worth mentioning.
Perhaps should consider this for a followup PR perhaps, because we could expect non-bundle users to be more advanced, so they could find it annoying to dismiss tie dialog. I'd love a dont warn again checkbox--not sure where we can save that state though 😬 so that would certainly be a followup.

[dalthviz]

I'd love a dont warn again checkbox--not sure where we can save that state though 😬 so that would certainly be a followup.

Yep, that makes sense to me. We could use a similar approach for settings as #95 or we could also check using QSettings (although maybe relying on Qt for that is not desired). Maybe there could be other ways 🤔

[jaimergp]

so that would certainly be a followup.

I'd advocate for that! Shall we merge here @dalthviz or do you prefer iterating on this PR?

[dalthviz]

I'd advocate for that! Shall we merge here @dalthviz or do you prefer iterating on this PR?

I think I could add here the checkbox to the dialog so at least on a per launch/session basis you can dismiss it. Then in a follow up PR we could check ways to add some persistence/settings. Does that make sense?

Besides that, what do you think about #111 (comment) @jaimergp ?

[psobolewskiPhD]

@dalthviz

I think I could add here the checkbox to the dialog so at least on a per launch/session basis you can dismiss it.

i think this is a good idea as a stepping stone!
I suspect most people install some stuff--multiple plugins--and then don't for quite some time!
Only issue would be if someone restarts napari after each install, based on the warning?
🤷
Even this I'd be cool with it being it's own PR.

[psobolewskiPhD]

Looks good to me!

[jaimergp]

Awesome, merging then @dalthviz.