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

Add transform mode button for layers #6794

Open
wants to merge 42 commits into
base: main
Choose a base branch
from

Conversation

dalthviz
Copy link
Member

@dalthviz dalthviz commented Mar 29, 2024

References and relevant issues

Part of #3975

Description

Add a button to activate the layers transform mode. Also, add a way to reset the transform to the initial value set at layer creation (reset trigger via Alt + Left mouse click over the transform mode button):

A preview:

transform_mode_button

Notes/TODOs:

@dalthviz dalthviz self-assigned this Mar 29, 2024
@github-actions github-actions bot added the qt Relates to qt label Mar 29, 2024
Copy link

codecov bot commented Mar 29, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 92.44%. Comparing base (98c2e0f) to head (a9c116c).

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #6794      +/-   ##
==========================================
- Coverage   92.48%   92.44%   -0.04%     
==========================================
  Files         612      612              
  Lines       55165    55229      +64     
==========================================
+ Hits        51018    51057      +39     
- Misses       4147     4172      +25     

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

@github-actions github-actions bot added the tests Something related to our tests label Mar 29, 2024
Co-authored-by: Isabela Presedo-Floyd <50221806+isabela-pf@users.noreply.github.com>
@github-actions github-actions bot added the design Design discussion label Apr 19, 2024
@dalthviz dalthviz changed the title [WIP] Add transform mode button for layers Add transform mode button for layers Apr 25, 2024
self.MODE.TRANSFORM,
True,
self.TRANSFORM_ACTION_NAME,
extra_tooltip_text=trans._('(use Alt-Left mouse click to reset)'),
Copy link
Member

Choose a reason for hiding this comment

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

Does pre-pending a new line char here work to make the total tooltip 2 lines?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yep, doing something like:

Suggested change
extra_tooltip_text=trans._('(use Alt-Left mouse click to reset)'),
extra_tooltip_text=trans._('\n(use Alt-Left mouse click to reset)'),

makes things look like this:

tooltip

Copy link
Member

Choose a reason for hiding this comment

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

yes! with the new line, maybe we don't need the parens and the use?
so just
Alt-Left mouse click to reset
I think that's still clear, but a bit more concise.

Copy link
Member Author

Choose a reason for hiding this comment

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

Just in case, a preview:

imagen

Copy link
Contributor

Choose a reason for hiding this comment

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

Nice! Now wondering if we should default to adding a newline for everything before the extra_tooltip_text...

Copy link
Member Author

Choose a reason for hiding this comment

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

That makes sense! From a quick check seems like the only other button that has some extra text is pan/zoom:

imagen

Maybe something like Temporarily re-enable by holding Space could be used as text in case the extra text is added always in a new line?

imagen

Also, should an issue be made to tackle that later or maybe is something worthy to be done here?

Copy link
Member

Choose a reason for hiding this comment

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

I mean I feel like the new line would be safe as a default...
but I don't think I would change it in this PR--if at all. It's easy to add the new line, harder to remove it if someone does want a one-liner.

Copy link
Member Author

@dalthviz dalthviz May 10, 2024

Choose a reason for hiding this comment

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

Created #6907 to further check the new line behavior related to binding actions to buttons. For the moment, I think things here can be left as they are. Another option could be to change the pan/zoom extra text to include a new line so something like:

self.panzoom_button = self._radio_button(
            layer,
            'pan_zoom',
            self.MODE.PAN_ZOOM,
            False,
            self.PAN_ZOOM_ACTION_NAME,
            extra_tooltip_text=trans._('\n(or hold Space)'),
            checked=True,
        )

imagen

Or using the text suggestion above:

self.panzoom_button = self._radio_button(
            layer,
            'pan_zoom',
            self.MODE.PAN_ZOOM,
            False,
            self.PAN_ZOOM_ACTION_NAME,
            extra_tooltip_text=trans._('\nTemporarily re-enable by holding Space'),
            checked=True,
        )

329304872-0bf88973-1fef-4560-a55d-c1b4e4a7ef7f

Let me know what do you think!

Copy link
Member

@psobolewskiPhD psobolewskiPhD left a comment

Choose a reason for hiding this comment

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

I like this a lot. Looks like a lot of changes at first, but it's mostly docstrings and refactoring that makes things simpler in the long run.

It is strange at first to see the tool icons for image layers, but it opens the possibility of other tools there (Crop? Histogram?)

Played with it locally -- found bug in Labels transform, made issue -- and it works nicely. We'll need to make a docs PR to document the change.

@brisvag
Copy link
Contributor

brisvag commented May 7, 2024

It is strange at first to see the tool icons for image layers, but it opens the possibility of other tools there (Crop? Histogram?)

<3

@dalthviz
Copy link
Member Author

dalthviz commented May 7, 2024

We'll need to make a docs PR to document the change.

Is there any specific place in the docs where things need to be added or changed? Not completly sure what needs to be done but happy to help with that 👍

dalthviz and others added 2 commits May 7, 2024 11:10
Co-authored-by: Peter Sobolewski <76622105+psobolewskiPhD@users.noreply.github.com>
Copy link
Member

@psobolewskiPhD psobolewskiPhD left a comment

Choose a reason for hiding this comment

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

Gah! I realized something: transform mode is 2D only, so we need the button be disabled if viewer is in 3D.

Edit: this is already the case for the Shapes layer, because the other buttons were behaving that way. Looks like self._EDIT_BUTTONS could be the way to go.

@psobolewskiPhD
Copy link
Member

I think we'll need to write up a description of Transform mode somewhere -- viewer guide? I'd hate to duplicate it for every layer, need to think about this -- and then inform that the icon activates that mode and that alt-click resets transforms.

@dalthviz
Copy link
Member Author

dalthviz commented May 8, 2024

I think we'll need to write up a description of Transform mode somewhere -- viewer guide? I'd hate to duplicate it for every layer, need to think about this

Created napari/docs#420 as quick draft but checking seems like probably some info should be also added over the layer types detailed descriptions at https://github.com/napari/docs/tree/main/docs/howtos/layers. Maybe to prevent duplication a general buttons (and even maybe controls) description should be added at https://github.com/napari/docs/blob/main/docs/howtos/layers/index.md ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
design Design discussion qt Relates to qt tests Something related to our tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants