8000 Implement command palette widget by hanjinliu · Pull Request #5483 · napari/napari · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Implement command palette widget #5483

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

Jump to bottom
Merged
merged 33 commits into from
Jan 26, 2025
Merged

Implement command palette widget #5483

merged 33 commits into from
Jan 26, 2025

Conversation

hanjinliu
Copy link
Contributor
@hanjinliu hanjinliu commented Jan 15, 2023 •
edited by github-actions bot
Loading

Description

This PR implements a command palette widget.

viewer-output

Still work-in-progress, but here's the list of required functinalities and how much of them this PR has done (See #5430).

Search logic: fuzzy matching ranked by match scoring

→ fuzzy matching is not implemented yet. Just match by __contains__ for now.

Command context: breadcrumbs for each search result (e.g., segment_blobs_and_things with_membranes [plugin name] > gaussian blur or preferences > plugin settings > plugin API)

→ Contexts are auto-generated from the command ID (e.g. "napari:layer:XYZ" → "napari > layer > XYZ")

Command coverage: napari menus & submenus, layer context menus, all plugin command/menu contributions as specified in the manifest

→ Currently, only those commands registered in the app-model way are available (so some commands such as "Open Files ..." are not available).

Action effects: there would be an indication in the UI that shows whether it will run the command, or direct to a dialog and additional input required from user

→ Not yet, but current implementation is already prepared for it.

Unavailable commands: gray out command options that aren’t available given the context. For example “split RGB” would be grayed out if the user hasn’t selected an image layer

→ The "when" parameters of commands are evaluated and unavaliable ones are grayed out.

Type of change

  • Bug-fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

References

#5430

How has this been tested?

  • example: the test suite for my feature covers cases x, y, and z
  • example: all tests pass with my change
  • example: I check if my changes works with both PySide and PyQt backends
    as there are small differences between the two Qt bindings.

Final checklist:

  • My PR is the minimum possible work for the desired functionality
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • I have added tests that prove my fix is effective or that my feature works
  • If I included new strings, I have used trans. to make them localizable.
    For more information see our translations guide.

@github-actions github-actions bot added the qt Relates to qt label Jan 15, 2023
@Czaki
Copy link
Collaborator
Czaki commented Jan 15, 2023

Active commands should always be first. You should not need to scroll through deactivate one to find the active one.

DragaDoncila
DragaDoncila reviewed Jan 18, 2023
View reviewed changes
Copy link
Contributor
@DragaDoncila DragaDoncila left a comment

Choose a reason for hiding this comment

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

This is awesome Hanjin, thanks for getting it off the ground!

I left a couple of general comments to aid my own understanding. Otherwise I have a few early concerns about the code:

  • I notice we've added the command palette as a new ViewerModel argument and I suspect we probably don't want to do that, as I see the command palette as something purely internal - I don't imagine a user creating a viewer with a different command palette. But maybe that's just me 😆 In general I see the command palette as a widget layer on top of app-model's commands rather than needing a new model
  • I suspect we don't want the command palette to live in components if it has any GUI/Qt related code, which it looks like it does. I'm not sure where the best location would be but again, I would expect most of the code to be purely providing a GUI interface for app-model's commands
  • We may want the widget itself to be contributed to napari/superqt, or even depend on yours if you plan on maintaining it separately from napari?

I'm not the best at answering these kinds of code structure/where to put things questions. Maybe @Czaki has suggestions in that vein?

napari/components/command_palette/_api.py Outdated


@inspect.signature
def register_without_func(
Copy link
Contributor

Choose a reason for hiding this comment

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

When would we have a funcless command? What would this be useful for?

napari/components/command_palette/_api.py Outdated
title: str | None,
desc: str | None = None,
tooltip: str | None = None,
when: Callable[..., bool] = None,
Copy link
Contributor

Choose a reason for hiding this comment

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

I will need to double check this but for some reason I remember app-model commands having both when and enablement clauses and I feel like when is whether is shows up in the menu at all and enablement is whether it's greyed out or not. In which case we'd probably be wanting enablement here? Again I need to check on this though.

napari/components/command_palette/_api.py Outdated
...

@overload
def register(
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is it that both CommandPalette and CommandGroup have the same overloaded register methods? Are you ever registered with one but not the other? Can the CommandGroup not just be an additional attribute of the Command?

@DragaDoncila
Copy link
Contributor

@hanjinliu we spoke a little bit about this in community meeting today and other core devs share my concerns about the "widgetiness" of the changes to components and the addition of the command palette to ViewerModel. We also all would love if the command palette interface was a thinner layer on top of app-model. I'm going to look into how we could achieve that and will get back to you with some more concrete suggestions. In the meantime if you have any ideas of course go for it.

@hanjinliu
Copy link
Contributor Author

Thank you for the review & discussion! Yes, I was also thinking of refactoring napari independent part into other repository.
In my opinion, the "backend" directory of app-model would be the better place to put. Commands are usually registered before a widget is actually created. This pre-registration architecture can be done along with app-model pretty well but requires a lot of additional, command-palette-specific functions if we put it in superqt.

@github-actions github-actions bot added the tests Something related to our tests label Sep 10, 2024
@jni jni added the feature New feature or request label Oct 23, 2024
@jni jni modified the milestones: 0.5.6, 0.6.0 Jan 15, 2025
@hanjinliu
Copy link
Contributor Author

@jni thank you for summing up the to-dos.

  • remove reader/writer contributions from the command palette
  • some form of display of contribution type (widgets/samples/etc)

I think these can be done in the future by specifying the category attribute of app_model.Action at the timing of loading plugins, so that command palette can filter which commands to be added or show the contribution type.

  • visual design improvements

I already fixed this. I'll push soon.

Hm. current error looks like we should finally go to never PySide6...

As far as I tested locally, it seems to me that this update() line can just be removed. I don't remember why I added this line ...

@github-actions github-actions bot added the design Design and discussion about it label Jan 17, 2025
@jni
Copy link
Member
jni commented Jan 17, 2025

As far as I tested locally, it seems to me that this update() line can just be removed. I don't remember why I added this line ...

Do you want to commit that to see if it passes CI while still working?

@hanjinliu
Copy link
Contributor Author

Do you want to commit that to see if it passes CI while still working?

Ok, let's see!

@psobolewskiPhD
Copy link
Member

This is pretty fun! For anyone trying to figure out how to use it, it's Command/Ctrl-Shift-P -- does not appear to be configurable at the moment, but is in the View menu.
In vanilla napari it's pretty limited, but a super sweet way to open samples, that's for sure!
It's interesting what doesn't work. I couldn't figure out:

  • toggle 3D/2D (button)
  • viewer.reset_view (button)
    I guess those are not routed correctly for this to work?

@jni
Copy link
Member
jni commented Jan 18, 2025

Omg all green 😭

@jni
Copy link
Member
jni commented Jan 18, 2025 •
edited
Loading

I couldn't figure out:

yeah probably those are not routed through app model. We should improve that! Having the command palette will be massive positive pressure to get the migration done. I agree that opening samples with this absolutely rocks.

@jni jni marked this pull request as ready for review January 24, 2025 09:12
@jni jni removed the needs:out-of-draft Needs transition from draft to ready for review label Jan 24, 2025
@jni jni added the ready to merge Last chance for comments! Will be merged in ~24h label Jan 24, 2025
@jni jni merged commit 45d92cb into napari:main Jan 26, 2025
37 checks passed
@github-actions github-actions bot removed the ready to merge Last chance for comments! Will be merged in ~24h label Jan 26, 2025
@jni
Copy link
Member
jni commented Jan 26, 2025

Whoop whoop! 🥳

@TimMonko
Copy link
Contributor
TimMonko commented Feb 6, 2025

I keep meaning to come back to this PR and just say how awesome it is. Any time I'm not on main right now I miss this command palette. Agreed that it's so nice already even just for loading Samples, but I really look forward to the usability this unlocks in the future!

This was referenced Feb 15, 2025
Closed
Closed
Open
@imagesc-bot
Copy link

This pull request has been mentioned on Image.sc Forum. There might be relevant details there:

https://forum.image.sc/t/upcoming-napari-0-6-0-release/108954/1

@jni
Copy link
Member
jni commented Apr 23, 2025

Just posting a video here so I can link to it 😂

command-palette.mp4

@imagesc-bot
Copy link

This pull request has been mentioned on Image.sc Forum. There might be relevant details there:

https://forum.image.sc/t/napari-0-6-0-released/112147/1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@DragaDoncila DragaDoncila DragaDoncila left review comments

@lucyleeow lucyleeow lucyleeow left review comments

@jni jni jni approved these changes

@Czaki Czaki Awaiting requested review from Czaki Czaki is a code owner

@psobolewskiPhD psobolewskiPhD Awaiting requested review from psobolewskiPhD psobolewskiPhD is a code owner

Assignees

@hanjinliu hanjinliu

Labels
design Design and discussion about it feature New feature or request highlight PR that should be mentioned in next release notes qt Relates to qt tests Something related to our tests ui change UI/UX
Projects
None yet
Milestone
0.6.0
Development

Successfully merging this pull request may close these issues.
10 participants
@hanjinliu @Czaki @DragaDoncila @jni @melissawm @lucyleeow @psobolewskiPhD @TimMonko @imagesc-bot @willingc
0