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

fix: better align access keys with the guidelines #843

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

Conversation

smileyhead
Copy link
Contributor

@smileyhead smileyhead commented Mar 7, 2025

Summary

Resolves #678.

Based on Microsoft's guidelines, I have…

  • added access keys (mnemonics) to all menu items and some buttons which I found were missing them
  • moved the position of existing mnemonics where needed
  • created new strings where a mnemonic was needed for only one instance of that string
  • created new strings specifically in the New Tag window to replace the generic ‘+’ buttons with something more descriptive that allows for mnemonics
  • created strings and added labels to the main window drop-downs to allow for mnemonics
  • created links (buddies) between labels and corresponding UI elements to allow for mnemonics
  • modified the File -> Open Recent list to prefix numbers in front of the directory paths, which allow for mnemonics
  • deleted unneeded _alt strings, as some of them should always have mnemonics and others should never have them

Unresolved issues:

  • I would have liked to create a mnemonic for the ‘Color’ label in the New Tag window that activates the button below it. The buddy has been defined, but it does not work and I do not know why.
  • The search box cannot be added mnemonics to directly and I am unsure on how to proceed (see this comment for more info)

Tasks Completed

  • Platforms Tested:
    • Windows x86
    • Windows ARM
    • macOS x86
    • macOS ARM
    • Linux x86
    • Linux ARM
  • Tested For:
    • Basic functionality
    • PyInstaller executable

@CyanVoxel CyanVoxel added Type: Bug Something isn't working as intended Type: QoL A quality of life (QoL) enhancement or suggestion Type: UI/UX User interface and/or user experience Type: Translation Translates or improves translation capabilities Priority: Medium An issue that shouldn't be be saved for last Status: Review Needed A review of this is needed Status: Blocked This issue or pull request is awaiting the outcome of another issue or pull request labels Mar 7, 2025
@CyanVoxel CyanVoxel added this to the Alpha v9.5.2 milestone Mar 7, 2025
@smileyhead
Copy link
Contributor Author

While looking into adding mnemonics to the search bar in the main window, I have discovered that adding them to a QLineEdit element directly is, unfortunately, not supported.

We might consider adding a shortcut key to focus on the text box (Ctrl + F, for instance) and signal this in the placeholder text (‘Search Entries ({shortcut})’), but that is out of scope for this PR. Alternatively, we could add a buddied QLabel in front of the search box, something like ‘Search Entries:’, but I do not wish to do that without prior approval.

@smileyhead
Copy link
Contributor Author

I apologise; I realised I have made this PR too soon, as there are still more commits I wish to make. I will reopen this when I am done.

@smileyhead smileyhead closed this Mar 8, 2025
@smileyhead
Copy link
Contributor Author

I have made all the modifications I wanted and updated the PR to reflect that. It is now ready for review.

@smileyhead smileyhead reopened this Mar 8, 2025
@CyanVoxel CyanVoxel removed the Status: Blocked This issue or pull request is awaiting the outcome of another issue or pull request label Mar 10, 2025
@CyanVoxel
Copy link
Member

I apologize for the inconvenience - a large refactor was recently merged (#844) that has moved several files throughout the codebase. This was done in order to correct some deep issues with the project's layout and was performed now as it seemed to be the best opportunity (as of 3/6/25) to disrupt as few open PRs as possible.

Unfortunately this PR is one that's been disrupted by this change. A rebase targeting main will be required to resolve the merge conflicts generated here. A merge commit is possible, however I do not recommend it as a rebase will be much smoother and much less work. I apologize again for the disruption.

I also noticed that the Ruff workflow was failing beforehand, please let me know if you'd like any help with that.

@smileyhead
Copy link
Contributor Author

This is the first time I have dealt with this issue, so apologies if it doesn't work the first time. Here goes.

@smileyhead smileyhead reopened this Mar 10, 2025
Kept:
- `generic.apply_alt`: <ins>A</ins>pply (renamed to `generic.apply`)
- `generic.cancel`: Cancel
- `generic.done`: Done
- `generic.delete`: Delete
- `generic.edit_alt`: <ins>E</ins>dit (renamed to `generic.edit`)
- `generic.overwrite`: Overwrite
- `generic.rename_alt`: <ins>R</ins>ename (renamed to `generic.rename`)
- `generic.skip_alt`: <ins>S</ins>kip (renamed to `generic.skip`)

Removed:
- `generic.apply`: Apply – **Reason:** Unused, and this should always have the mnemonic <ins>A</ins>pply by convention.
- `generic.cancel_alt`: <ins>C</ins>ancel – **Reason:** The access key for Cancel is Esc which already works in TagStudio.
- `generic.done_alt`: <ins>D</ins>one – **Reason:** Likewise to Cancel, the access key for Apply is Esc.
- `generic.delete_alt`: <ins>D</ins>elete – **Reason:** Destructive actions should not have mnemonics, so as to avoid accidental activation.
- `generic.edit`: Edit – **Reason:** Again, this should have mnemonics.
- `generic.overwrite_alt`: <ins>O</ins>verwrite – **Reason:** Destructive action.
- `generic.rename`: Rename – **Reason:** Unused.
- `generic.skip`: Skip – **Reason:** Unused.
@smileyhead
Copy link
Contributor Author

I have decided to also address the question of the _alt strings while waiting for a review. The modifications I've made are as follows:

Kept:

  • generic.apply_alt: Apply (renamed to generic.apply)
  • generic.cancel: Cancel
  • generic.done: Done
  • generic.delete: Delete
  • generic.edit_alt: Edit (renamed to generic.edit)
  • generic.overwrite: Overwrite
  • generic.rename_alt: Rename (renamed to generic.rename)
  • generic.skip_alt: Skip (renamed to generic.skip)

Removed:

  • generic.apply: Apply – Reason: Unused, and this should always have the mnemonic Apply by convention.
  • generic.cancel_alt: Cancel – Reason: The access key for Cancel is Esc which already works in TagStudio.
  • generic.done_alt: Done – Reason: Likewise to Cancel, the access key for Apply is Esc.
  • generic.delete_alt: Delete – Reason: Destructive actions should not have mnemonics, so as to avoid accidental activation.
  • generic.edit: Edit – Reason: Again, this should have mnemonics.
  • generic.overwrite_alt: Overwrite – Reason: Destructive action.
  • generic.rename: Rename – Reason: Unused.
  • generic.skip: Skip – Reason: Unused.

@CyanVoxel CyanVoxel mentioned this pull request Mar 11, 2025
8 tasks
@smileyhead
Copy link
Contributor Author

Sorry, is this PR still on the way to getting merged? I totally understand if you're just busy with other things in the project, I'm just not sure how to interpret this silence.

As far as I can tell, the failing check is due to the string changes I've made producing key mismatches across the different language .JSONs. I assume this can be fixed automatically? If not, I could go over all the language files and make the necessary changes manually.

@CyanVoxel
Copy link
Member

Sorry, is this PR still on the way to getting merged? I totally understand if you're just busy with other things in the project, I'm just not sure how to interpret this silence.

Yes, sorry about the delay! When I initially pulled this to review I noticed the new mnemonic underlines across the interface of the program, which led me to research what the default behavior of other programs is and how this might be able to be avoided. This led me to create #856 since macOS doesn't support keyboard mnemonics at all and that would at least prevent the UI from being cluttered on that system, but unfortunately I couldn't find anything useful to suggest regarding Qt's handling of mnemonics. It just feels a bit odd to have underlines across most of the UI text when other programs on Windows don't have to deal with this because the underlines only appear (outside of a menubar) when the ALT key is pressed. If there was a straightforward way to do that here then I wouldn't have any complaints, but so far I haven't been able to find any way to do that with Qt. One option I was considering was the ability to make mnemonic underlines a toggle in the settings, which would then use a similar "filtering" method to #856. But since the new settings menu in #859 hasn't been merged yet I also didn't want to tell you to add a setting that would immediately conflict with that PR.

As far as I can tell, the failing check is due to the string changes I've made producing key mismatches across the different language .JSONs. I assume this can be fixed automatically? If not, I could go over all the language files and make the necessary changes manually.

I would appreciate if you could do that, I'm not sure if Weblate would handle doing it automatically very well and it would mean that the workflow checks would be failing on main in the meantime.

@smileyhead
Copy link
Contributor Author

make mnemonic underlines a toggle in the settings

Being able to always see mnemonics even when not holding Alt is a system setting in Windows. I wonder if it would be possible to make TagStudio only show them as long as Alt it held down while this is disabled? It would go around having to add a manual override on an app level.

image

I would appreciate if you could do that

Alright, I will do that.

@smileyhead
Copy link
Contributor Author

Now that I'm looking through the other language files, several of them are already missing lots of strings. I wonder why they weren't triggering a fail before…?

@Computerdores
Copy link
Collaborator

Now that I'm looking through the other language files, several of them are already missing lots of strings. I wonder why they weren't triggering a fail before…?

The check validates that the format strings are correct (they use all the keys the english one uses and no others than those) not that all keys have translations. After all, we wouldn't want the checks to fail because a language isn't fully translated yet

@smileyhead
Copy link
Contributor Author

That should be it, then.

The check validates that the format strings are correct (they use all the keys the english one uses and no others than those) not that all keys have translations.

Ah, I see. That makes sense.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Priority: Medium An issue that shouldn't be be saved for last Status: Review Needed A review of this is needed Type: Bug Something isn't working as intended Type: QoL A quality of life (QoL) enhancement or suggestion Type: Translation Translates or improves translation capabilities Type: UI/UX User interface and/or user experience
Projects
Status: 👀 In review
Development

Successfully merging this pull request may close these issues.

[Bug]: Access keys (mnemonics) breaking guidelines
3 participants