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

in Tooltips and config, upper/lower case of the keybindings are not respected #2856

Closed
mmahmoudian opened this issue Aug 4, 2022 · 11 comments
Labels
Bug It's a bug
Milestone

Comments

@mmahmoudian
Copy link
Member

Flameshot Version

Flameshot v12.1.0 (001726a)

Installation Type

User repository (AUR)

Operating System type and version

5.15.57-2-MANJARO

Description

At the moment the in the tooltips and config window, all keybindings are shown as upper-case letters where as they only work when the key is pressed without holding the Shift. A little more investigation also shows that the config file also contains upper-case letters. This means that although Flameshot is capable of recognizing upper and lower-case letters (e.g T doesn't work but t does), the user does not have the option to use upper case for one keybinding and lower-case for another.

I suggest:

  1. display the correct form of the keybinding <- the bug
  2. letting user to also utilize upper-case bindings <- the feature request

Steps to reproduce

No response

Screenshots or screen recordings

Showcasing tooltip:

image

Showcasing config window:
image

Showcasing the message box:
image

System Information

inxi --system --graphics --width=80
System:
  Host: aaa Kernel: 5.15.57-2-MANJARO arch: x86_64 bits: 64
    Desktop: KDE Plasma v: 5.24.6 Distro: Manjaro Linux
Graphics:
  Device-1: Intel CometLake-S GT2 [UHD Graphics 630] driver: i915 v: kernel
  Device-2: AMD Lexa XT [Radeon PRO WX 3200] driver: amdgpu v: kernel
  Display: x11 server: X.Org v: 21.1.4 driver: X: loaded: amdgpu,modesetting
    gpu: amdgpu resolution: 1: 1920x1080~60Hz 2: 1080x1920~60Hz
    3: 1080x1920~60Hz
  OpenGL: renderer: AMD Radeon Pro WX 3200 Series (polaris12 LLVM 14.0.6 DRM
    3.42 5.15.57-2-MANJARO) v: 4.6 Mesa 22.1.4
@mmahmoudian mmahmoudian added Unconfirmed Bug The bug is not confirmed by anyone else. Bug It's a bug and removed Unconfirmed Bug The bug is not confirmed by anyone else. labels Aug 4, 2022
@Tomasito665
Copy link
Contributor

IMHO this is not a bug. Stating the obvious, a key combination is a combination of keys on your keyboard. Rendering a Shift + T combination as upper-case T is one of many ways of representing that key combination. The issue with this approach is that while it works for character keys, it starts to fall apart for keys that do not represent a character. How would you represent the key combination Shift + Enter, for example?

@mmahmoudian
Copy link
Member Author

@Tomasito665 Thanks for your comment, but I disagree. This would make the non-tech savvy users confused. Take a look at any software out there, the key combination is presented as lower case.

@Tomasito665
Copy link
Contributor

@mmahmoudian I agree that T could suggest a Shift + T key combination. However, representing a Shift + T combination simply with an upper-case T is ambiguous in some cases. Take the Greek keyboard layout, for example, which yields an upper-case sigma for both keys W and S. Given a shortcut represented with Σ, you would have no way to tell whether the shortcut is Shift + W or Shift + S.

What do you think of Google's approach to Chrome's keyboard shortcut descriptions? It defines all shortcut descriptions with lower-case letters and prepends shift-modified ones with Shift +. This takes away the confusion you describe and is not ambiguous.

image

https://support.google.com/chrome/answer/157179

@borgmanJeremy
Copy link
Contributor

I think @Tomasito665 latest proposal is the way to go.

@mmahmoudian
Copy link
Member Author

However, representing a Shift + T combination simply with an upper-case T is ambiguous in some cases.

This is exactly my point number 1. We should show the lower case, and show the modifier keys if they are used. I'm not suggesting to use T but rather show Shift+t.

@borgmanJeremy
Copy link
Contributor

Ohhhh ok I misunderstood

@Tomasito665
Copy link
Contributor

However, representing a Shift + T combination simply with an upper-case T is ambiguous in some cases.

This is exactly my point number 1. We should show the lower case, and show the modifier keys if they are used. I'm not suggesting to use T but rather show Shift+t.

We are on the same page, then :-)

@Tomasito665
Copy link
Contributor

I went ahead and gave it my best shot. Humongous pull request:
#2934

@mmahmoudian
Copy link
Member Author

@Tomasito665 yeap, it took me thousands of milliseconds to read the changes 😅 thanks for the PR. Simple and efficient as far as I can tell. 👍

@Tomasito665
Copy link
Contributor

Are there any pending tasks for this issue? If not, let's close it! :-)

@mmahmoudian
Copy link
Member Author

Thanks again for the PR. I'm closing this now.

@mmahmoudian mmahmoudian added this to the v13 milestone Oct 21, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug It's a bug
Projects
None yet
Development

No branches or pull requests

3 participants