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

Show notification when using shortcut to change current selected label to largest used label plus one #6546

Merged
merged 9 commits into from
Feb 3, 2024

Conversation

dalthviz
Copy link
Member

@dalthviz dalthviz commented Dec 19, 2023

References and relevant issues

Closes #6517

Description

Add showing a notification when the shortcut to set current selected label to largest used label plus one can't change the label value since it is already the largest used label plus one.

A preview:

label

…l to max value plus one when no change is possible

Co-authored-by: Peter Sobolewski <76622105+psobolewskiPhD@users.noreply.github.com>
@dalthviz dalthviz self-assigned this Dec 19, 2023
if layer.selected_label == new_selected_label:
show_info(
trans._(
"Largest label already selected",
Copy link
Collaborator

Choose a reason for hiding this comment

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

It sounds misinformation to me. Largest may mean Largest by volume/area.
And here we select first empty labels num.

Copy link
Member Author

Choose a reason for hiding this comment

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

Maybe the message could be then something like Maximum label value plus one already set as current selected label or something like that which explains the situation with a more specific/correct wording? Not totally sure what words will describe better that layer.selected_label is already equal to np.max(layer.data) + 1 🤔

Copy link
Collaborator

Choose a reason for hiding this comment

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

This proposition is much better.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah it's tricky.
Maximum label value plus one already set as current selected label isn't exactly intuitive either.
maybe highest unused label?

Copy link
Member Author

Choose a reason for hiding this comment

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

So something like Highest unused label already set as current selected label?

Copy link
Member

Choose a reason for hiding this comment

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

In the end we need a message -- notification -- that is targeted at a user painting labels and is clear as to why seemingly nothing happened when they pushed the keybind.

Copy link
Member Author

@dalthviz dalthviz Dec 20, 2023

Choose a reason for hiding this comment

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

Maybe something like Current selected label is not being used. You will need to use it to be able to set the current select label to the next label available ? Kind of wordy but maybe can sound like more aimed to an end user which is unaware of implementation details but still wants to increase the current selected label? Could it even be worthy to mention in the notification that to increase the label regardless of it being used you can use the = shortcut?

Edit - The message with the Increase the currently selected label by one shortcut mention could be something like:

Current selected label is not being used. You will need to first use it to be able to set the current select label to the next value available.

To use the next label available regardles of it being used, please check the Increase the currently selected label by one keybinding (=)

Copy link
Member

Choose a reason for hiding this comment

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

I like that!

Copy link
Member Author

Choose a reason for hiding this comment

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

What do you think about structuring the message in that way @Czaki ?

Copy link
Collaborator

Choose a reason for hiding this comment

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

looks ok

Copy link

codecov bot commented Dec 19, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (c6abeb5) 92.29% compared to head (34066e7) 92.25%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #6546      +/-   ##
==========================================
- Coverage   92.29%   92.25%   -0.05%     
==========================================
  Files         605      605              
  Lines       54088    54091       +3     
==========================================
- Hits        49923    49901      -22     
- Misses       4165     4190      +25     

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

dalthviz and others added 3 commits December 19, 2023 12:59
Co-authored-by: Peter Sobolewski <76622105+psobolewskiPhD@users.noreply.github.com>
Co-authored-by: Grzegorz Bokota <bokota+github@gmail.com>
@Czaki
Copy link
Collaborator

Czaki commented Dec 21, 2023

This PR will may generate conflict with #6548, where, because of typing, I add if guard to allow use only this action is a NumPy array (As it is a whole data scan).

This is only a note, I do not thnk that it will require any action.

@psobolewskiPhD psobolewskiPhD added the ready to merge Last chance for comments! Will be merged in ~24h label Jan 9, 2024
@psobolewskiPhD
Copy link
Member

Just FYI, I think the plan is to merge stuff once 0.4.19 is released, to minimize cherry pick issues.

@psobolewskiPhD
Copy link
Member

@Czaki we in the clear to merge this? Re: your comment above

@psobolewskiPhD psobolewskiPhD merged commit ad78ee2 into napari:main Feb 3, 2024
32 checks passed
@psobolewskiPhD psobolewskiPhD removed the ready to merge Last chance for comments! Will be merged in ~24h label Feb 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Keybinding to set a new label doesn't work consistently
3 participants