Select shortcut shift #5021
Select shortcut shift #5021
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #5021 +/- ##
==========================================
- Coverage 88.84% 88.78% -0.06%
==========================================
Files 579 579
Lines 49109 49116 +7
==========================================
- Hits 43629 43609 -20
- Misses 5480 5507 +27 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
I like the main idea of this PR, but do not like multiple repetitions of extra_tooltip_text as it is an additional thing to be maintained.
I prefer to have some generic solution. For example, inspect the action to check if it support Hold shortcut to temporarily activate. Or at least export these strings to some function (not global variable as trans need to be supported) to not repeat the same string multiple times that may introduce some problem in further refactoring.
There was a problem hiding this comment.
Good idea to unify the behaviours. I think we should prioritize mnemonics (order in GUI matches order of shortcuts) rather than aesthetics (order by category).
To be honest, I think the best solution would be to have letters rather than numbers (d for delete and so on), but I wasn't there when numbers were chosen in the first place. @jni?
I agree with @Czaki on the maintainability, we should find a more general way of adding the tooltips.
Either way, I think this should wait 0.5 to be merged, because it breaks quite prominent shortcuts.
|
Thanks again @mstabrin - you're on fire!! 🔥 🚒
For what it's worth they used to be letters but it was decided the ergonomics weren't great for keyboard users (@haesleinhuepf suggested, IIRC) so we switched to the top row of numbers. With @Czaki's recent PR #5018 we can better support multiple, configurable shortcuts so maybe having both is a better long term solution edit: the letters as shortcuts are also only meaningful in our language, which isn't ideal but maybe not a dealbreaker? @LCObus do you have insight here? |
Yes, you can read in #2800 why I proposed that. TL:DR: Ergonomy. When changing it again, please make sure users can annotate things for hours without breaking their fingers on the keyboard. |
You're totally right, we should keep the numbers. So, to get back on topic, I suggest we ensure shortcut numbers and order of buttons in the GUI match, and leave the possible extra shortcuts with #5018 to a later date.
I don't think that would be a big deal (most of the docs and tutorials will likely be in english), but maybe we can localize those as well? :P |
|
I really like the idea of generalization of the tooltip and tried to automatically detect if a button is toggable by holding it, but until now I was unable to do so :( |
|
I tried copying the |
all current methods with behavior "short click to change, hold to temporary activate" are done using |
|
@Czaki Thank you for pointing me in the right direction :) I added it now as an attribute to the Action class. |
|
+1 to ergonometry as the best reason to start with numbers. |
|
Finally had a chance to play with this—really dig this PR. Once #5018 goes in it will be pure 🔥. |
Move `swap_labels` button
|
Thanks for the work here @mstabrin! Could you merge |
|
Just want to note that PR should close #3133 |
|
Side note: @Czaki does the release script use the "ui change" label? I think it's important to note things like moving buttons in release notes. |
|
I merged the main branch and adjusted the code accordingly :) |
|
I have some tests failing locally, but the same tests also fail on the main branch, so I am not sure what to do about it :) |
|
Can you clarify which tests are failing for you locally? |
|
@psobolewskiPhD It appears it was due to some cluttering of the python environment. |
Ok. The CI macOS fail seems like the Pint 0.20 fail: |
|
Pint 0.20.1 released. |
|
@mstabrin I was testing this today and noticed that |
|
@mstabrin @jni related to the new polygons tools discussion: this PR would change existing icon-keybinding relation, but harmonize between layers. Obviously existing users might have workflows affected, but the harmonization lowers barriers to entry and makes it easier going forward. |
|
Hey @psobolewskiPhD, Thanks for the reminder, I was out of the project for some time. I will see what I can do :) |
|
@mstabrin We've been discussing some aspects related to tools recently, so I was reminded of this PR. We definitely need to give some thought to tools/icons/organization, so it might get re-reviewed from a fresh perspective. |
|
@mstabrin What is your current status? Did you predict that you will be able to come back to PRs, or we could take your PRs to finish them? |
Description
Based on the previous discussion, the functionality asked for in #5013 is already present :)
However, I propose some layout change to make it less confusing:
Points, Shapes, and Labels might be used in the same project multiple times.
However, their shortcuts for Select and delete are not identical:
Select(3) in Points is Delete(3) in Shapes
Which immediately lead to a loss of data points via testing.
Therefore, I propose that the shortcuts for Delete(1) and Select(2) are identical in both layers.
I also reflected this in the order of the RadioButtons, but I am also fine by just adjusting the shortcut to keep the visual grouping of functionality rather then reflecting the order of the shortcuts.
Additionally, it seems that the functionality of the visual representation of the shortcuts in the tooltips was already there, but was missing a call to the update function.
I also added information about the toggle option to those buttons.
Type of change
References
#5013