Fix shortcut ambiguities

shaavan commented at 6:03 pm on July 21, 2022: contributor

Picks up #129 fixes #126

This PR addresses and fixes the shortcut collisions.
This PR takes reference from this commit.

Changes done in this PR:

Option whose shortcut is changed	From -> To	To avoid collision with
Size of database cache	`D` -> `I`	Display tab
Enable coin control	`C` -> `a`	Cancel option
Connect through SOCKS5 proxy	`C` -> `S`	Cancel option
Minimize to tray instead of taskbar	`M` -> `T`	Main tab

Also, this PR address this comment in #129, which was suggested to add with the original PR.

“Third party transaction URLs” string could be ended with :

qt: Fix shortcut ambiguities

- This commit fixes shortcut ambiguity by getting rid of shortcut
  collisions.

f77451c8c1

shaavan commented at 12:46 pm on July 23, 2022: contributor

Updated from f77451c8c11b9d41f5b6fa8659d947b0c99051e7 -> dd536e97b19743898c531909731b7c40977e2cc9 (pr633.01 -> pr633.02, diff)

Changes:

Added another commit that changes the shortcut for the Close option from l -> C without introducing shortcut collisions.

The reasoning for the change:

Close action is marked with shortcut c everywhere else in the GUI.
c is a more intuitive shortcut for Close action than l.
There is no shortcut conflict introduced with this change.

in src/qt/forms/optionsdialog.ui:241 in dd536e97b1 outdated

237@@ -238,7 +238,7 @@
238              <string>Whether to show coin control features or not.</string>
239             </property>
240             <property name="text">
241-             <string>Enable coin &amp;control features</string>
242+             <string>Enable co&amp;in control features</string>

jarolrod commented at 8:26 am on July 25, 2022:

This makes the shortcut hard to see, it might as well not be there if you can’t reasonably see it. Additionally, the letter i seems off for representing Enable coin control features. Maybe consider if there is a letter here that can make the shortcut more visible and feel more appropriate for the feature?

shaavan commented at 3:29 pm on August 1, 2022:

I think the visibility is slightly affected, so I agree with your suggestion of using another letter.

However, all the first intuitive first choices for this option, E, c, o, f, are used by other options.

I think a would be a good choice for this feature. Though it is not the most intuitive letter, it will help with the visibility as this would at the starting of the option name.

En&able coin control features

shaavan force-pushed on Aug 1, 2022

shaavan commented at 3:36 pm on August 1, 2022: contributor

Updated from dd536e97b19743898c531909731b7c40977e2cc9 to cfe407c81d66ea4b14737208eb568dfd36265a73 (pr633.02 -> pr633.03, diff)

Addressed @jarolrod comment

Changed Enable coin control features shortcut from i to a.

The reasoning for this change is explained here.

in src/qt/forms/optionsdialog.ui:241 in cfe407c81d outdated

237@@ -238,7 +238,7 @@
238              <string>Whether to show coin control features or not.</string>
239             </property>
240             <property name="text">
241-             <string>Enable coin &amp;control features</string>
242+             <string>En&amp;able coin control features</string>

jarolrod commented at 10:20 pm on August 6, 2022:

you’ve introduced a new ambiguity within this file, now this option collides with the shortcut to switch to the Window tab within the Options Window. If you test this new shortcut, the Enable Coin Control features option will not actuate.

shaavan commented at 5:23 pm on August 13, 2022:

Thanks, Jarol, for catching this!

I have reverted to the original change done for this option in this PR. Addressed in 5fde8fb.

jarolrod commented at 10:21 pm on August 6, 2022: member

It should be noted that this is picking up #129, and should look at the discussion had there especially the feedback.

The second commit despite stating that “there are no shortcut conflict introduced with this change”, in fact introduces a new collision between the Close and Copy button.

I have gone through all of the shortcuts and can confirm there are no more besides the ones mentioned in this review.

qt: Add colon to third party URLS data field 5fde8fbe08

shaavan force-pushed on Aug 13, 2022

shaavan commented at 5:18 pm on August 13, 2022: contributor

Updated from cfe407c81d66ea4b14737208eb568dfd36265a73 to 5fde8fbe0850218403c54d0cee04ecb2f36d79a0 (pr633.03 -> pr633.04, diff)

Addressed @jarolrod comment

Reverted to pr633.01. Corresponding changes include:
- Changed Enable coin control features shortcut from a to i.
- Changes shortcut of close option from C to l.
Added a commit to addressing this comment in PR #129 from which this PR is picked.

Thank you very much, Jarol, for your thorough review!

hebasto renamed this:
~~qt: Fix shortcut ambiguities~~
Fix shortcut ambiguities
on Aug 15, 2022

luke-jr approved

luke-jr commented at 10:58 pm on August 25, 2022: member

utACK 5fde8fbe0850218403c54d0cee04ecb2f36d79a0, but PR description needs to be updated

pablomartin4btc approved

pablomartin4btc commented at 12:33 pm on September 14, 2022: contributor

Tested ACK.

I’ve reproduced the issue and verified the collisions on the Options window (from main menu “Settings” item “Options”) between the shorcuts (alt+?) for the 4 elements across the different tabs and the shorcuts to navigate through the different tabs and/ or the “Cancel” button at right side at the bottom.

After compiling the fix 5fde8fbe0850218403c54d0cee04ecb2f36d79a0, I’ve verified that there’s no collisions and the shortcuts work as expected.

I’ve pasted the screenshot above just as a reference and to show some of the shortcuts.

I’ve also added a “Where” column at the table that was provided at the top description to make it clearer.

Option whose shortcut is changed	Where	From -> To	To avoid collision with
Size of database cache	Main tab	D -> I	Display tab
Enable coin control	Wallet tab	C -> a	Cancel option
Connect through SOCKS5 proxy	Network tab	C -> S	Cancel option
Minimize to tray instead of taskbar	Window tab	M -> T	Main tab

pablomartin4btc commented at 1:36 pm on September 14, 2022: contributor

Since we are here, shouldn’t we add a shortcut for the “Options” window under the “Settings” menu? Like Ctrl+O or Ctrl+Shift+O?

Another kind of unrelated comment but it’s related somehow with one of the options (“Minimize on close” on Window tab) and the Ctrl+W shortcut. After playing a bit with some shortcuts and accidentally closing the app with Ctrl+W (Ctrl+Q is the shortcut to close the app specified on the File menu), I found this was worked out on 15768, shouldn’t be this “Minimize on close” be ticked by default?

hebasto commented at 4:15 pm on October 26, 2022: member

Maybe start from catching QShortcut::activatedAmbiguously() signals? It would help to test this PR, and translated GUI and future changes as well.

hebasto commented at 2:07 pm on March 27, 2023: member

@shaavan Still working on this PR?

DrahtBot commented at 2:07 pm on March 27, 2023: contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	luke-jr
Concept ACK	pablomartin4btc

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

DrahtBot added the label CI failed on Aug 18, 2023

DrahtBot removed the label CI failed on Aug 23, 2023

DrahtBot added the label CI failed on Aug 31, 2023

DrahtBot removed the label CI failed on Sep 4, 2023

hebasto commented at 6:25 pm on September 22, 2023: member

Closing for now due to lack of progress. Let us know if this should be reopened.

Labeling “Up for grabs” as well.

hebasto closed this on Sep 22, 2023

hebasto added the label Up for grabs on Sep 22, 2023

bitcoin-core locked this on Sep 21, 2024

Fix shortcut ambiguities #633

Reviews