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.

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

Use display name on plugins that provide it #32

Merged
merged 5 commits into from
May 29, 2024

Conversation

goanpeca
Copy link
Contributor

@goanpeca goanpeca commented Apr 10, 2024

References and relevant issues

Description

This PR now displays the display name (if provided by author) on the available plugins list. The installed list still will not show them as another fix needs to be carried out on the npe2 project to include the extra field in the metadata.

Search works for both package name and display name

Screenshot

Before

Screenshot 2024-04-15 at 9 06 13 AM

After this PR

Screenshot 2024-04-15 at 8 57 25 AM

@goanpeca goanpeca self-assigned this Apr 10, 2024
@goanpeca goanpeca changed the title Add exception handling Use display name on plugins that provide it Apr 10, 2024
@jaimergp
Copy link
Contributor

How does this relate to #15?

@goanpeca
Copy link
Contributor Author

@jaimergp #15 was a stale PR, this one superseeds it!

@goanpeca goanpeca requested review from jaimergp and Czaki and removed request for jaimergp April 15, 2024 14:06
@goanpeca goanpeca marked this pull request as ready for review April 15, 2024 14:07
@goanpeca goanpeca mentioned this pull request Apr 15, 2024
12 tasks
Copy link
Contributor

@Czaki Czaki left a comment

Choose a reason for hiding this comment

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

looks ok for me

napari_plugin_manager/qt_plugin_dialog.py Outdated Show resolved Hide resolved
@jaimergp
Copy link
Contributor

Since we don't disambiguate on display names (the uniqueness is only guaranteed for the actual package name), this could be used to inject malicious packages in the ecosystem.

I suggest the actual package name is also shown next to the display name, or at least provide an icon with a tooltip listing the actual name.

@Czaki
Copy link
Contributor

Czaki commented Apr 19, 2024

Since we don't disambiguate on display names (the uniqueness is only guaranteed for the actual package name), this could be used to inject malicious packages in the ecosystem.

good point. I think we should always show name of package, as people may not check tooltip. We should also highlight collisions somehow.

@goanpeca
Copy link
Contributor Author

goanpeca commented Apr 28, 2024

Thanks for the comments, I will add the name next to the display name !

Since the Package name is a QPushButton it cannot have rich text added to it to display the package name smaller or in italics or something different.

Screenshot 2024-04-27 at 9 05 38 PM

Would something like this work @jaimergp @Czaki ?

@goanpeca
Copy link
Contributor Author

We should also highlight collisions somehow.

As in 2 packages are using the same display name?

Sounds like something to be addressed separately? 🤔

@Czaki
Copy link
Contributor

Czaki commented Apr 28, 2024

@goanpeca did you point example package with, a different package name and display name?

@goanpeca
Copy link
Contributor Author

goanpeca commented Apr 30, 2024

@goanpeca did you point example package with, a different package name and display name?

@Czaki yes, in the example above: Display Name is Cut Detector, and package name is cut-detector. Apologies I was not more explicit :)

@Czaki
Copy link
Contributor

Czaki commented Apr 30, 2024

I missed it (on image) will test it tomorrow.

@jaimergp
Copy link
Contributor

Any updates here?

@Czaki
Copy link
Contributor

Czaki commented May 14, 2024

In line 404 we get plugin name from button text, that will not work with rich name.

Why can we not change button into label? As far as I check we only use it to open project website, that could be done with link on QLabel.

@goanpeca
Copy link
Contributor Author

Why can we not change button into label? As far as I check we only use it to open project website, that could be done with link on QLabel.

You are correct, working on a prototype

@goanpeca goanpeca marked this pull request as draft May 23, 2024 16:55
@goanpeca
Copy link
Contributor Author

goanpeca commented May 24, 2024

This uses now a clickable label to preserve the previous behavior. Also styling of links inside rich texts is not possible, so this also preserves the UI look in general

Example on image

Display name: napari ARCOS
plugin name: arcos-gui

Screenshot 2024-05-23 at 11 17 37 PM

What do you think @Czaki, @jaimergp, @psobolewskiPhD ?

Also, I needed to change some QSS that lives on napari side. I am bringing this over on a separate PR

@goanpeca goanpeca force-pushed the enh/doisplay-name branch 6 times, most recently from 0115ae0 to 08902aa Compare May 24, 2024 06:35
@@ -30,7 +30,7 @@ jobs:
strategy:
fail-fast: false
matrix:
platform: [ubuntu-latest, windows-latest, macos-latest]
platform: [ubuntu-latest, windows-latest, macos-13]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was failing with missing wheels due to latest being arm I think?

from napari_plugin_manager.qt_package_installer import InstallerActions

if qtpy.API_NAME == 'PySide2' and sys.version_info[:2] == (3, 11):
if (qtpy.API_NAME == 'PySide2' and platform.system() != "Linux") or (
Copy link
Contributor Author

@goanpeca goanpeca May 24, 2024

Choose a reason for hiding this comment

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

The same error was being seen across the board

Soma variation of object cannot be interpreted as an integer

Following napari I am only testing pyside2 combination for linux with python 3.9 and 3.10

@goanpeca goanpeca requested review from Czaki and jaimergp May 24, 2024 14:50
@goanpeca goanpeca marked this pull request as ready for review May 24, 2024 14:50
@goanpeca goanpeca requested a review from psobolewskiPhD May 27, 2024 23:07
@goanpeca goanpeca force-pushed the enh/doisplay-name branch from 08902aa to 31944fa Compare May 28, 2024 14:48
@goanpeca goanpeca merged commit 8f701cf into napari:main May 29, 2024
10 checks passed
@goanpeca goanpeca deleted the enh/doisplay-name branch May 29, 2024 22:45
@goanpeca goanpeca added the enhancement New feature or request label Jul 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants