-
Notifications
You must be signed in to change notification settings - Fork 6.7k
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
Quick Accent: Add currencies to VK_3 and VK_4 (most commonly used keys for currencies) #36665
base: main
Are you sure you want to change the base?
Conversation
… symbols) + other minor currency updates.
This reverts commit 0c99507.
@microsoft-github-policy-service agree |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not a fan of alot of redundancy
LetterKey.VK_B => new[] { "฿", "в", "₿" }, | ||
LetterKey.VK_C => new[] { "¢", "₡", "č" }, | ||
LetterKey.VK_D => new[] { "₫" }, | ||
LetterKey.VK_E => new[] { "€", "£" }, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My opinion: the origin of the symbol - that is the letter L - outweighs a disputable visual resemblance. If it were we, I would not add it here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But does anyone know that's the origin symbol? I live in England and I would not have guessed that. This suggestion feels like sacrificing usability for semantics. Also, with this change, it's in both, which was the suggested fix in the later posts in the issue, so both views are addressed.
LetterKey.VK_M => new[] { "₼" }, | ||
LetterKey.VK_N => new[] { "л" }, | ||
LetterKey.VK_O => new[] { "¤" }, | ||
LetterKey.VK_P => new[] { "£", "₽" }, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same. Just like there's no dollar sign under the "D".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The pound sign is and was under P so this comment contradicts the existing code base.
Not to be pedantic but this is quite literally not redundant. I think you mean repetitive but negating repetition requires a more reasoned retort given repetition serves an active purpose in a stable system. Alternative paths to useful behavior improve user experience. On a side note, having utilization based priority of symbols will naturally order them in the most convenient way for each individual user, and I believe there's a request to opt out of specific locale characters, so repetition in the long term has very little negative impact on user experience, so the assumed (unstated) cons to this approach seem very minimal from my point of view. |
Hi @ansalls, I stumbled upon this PR and I was wondering why you chose VK_3 and VK_4, so I decided to do some counting. I don't really understand where you got the idea of these being the most commonly used for currencies. In my opinion this would make Quick Accent, already a relatively complex tool, unnecessarily more confusing and hard to use. Maybe my judgement is wrong, so message me with any counterarguments you might have – I am open to discussion. Best regards. (Edit: turns out there are 217 layouts, I used an old list. I am not going to recount, as it would have minimal effect on the numbers) |
Summary of the Pull Request
Added currencies to VK_3 and VK_4 (the most commonly used keys for currencies) and a couple of other minor currency updates - added £ to VK_L (base character) and VK_E (similar-presenting character), ¤ (generic currency symbol) to VK_O, and ₿ (bitcoin symbol) to VK_B. Few things were mapped to 3 and 4, so the lists aren't long even with the whole set of currency characters added.
Built and verified updates worked as expected.
PR Checklist
Detailed Description of the Pull Request / Additional comments
Note: I'd investigated attempting to determine whether a virtual key represented a currency symbol to dynamically show the additional currency symbols only in that event. However, there wasn't a straightforward way to accomplish that and it would require significant changes to the application's structure to implement.
Validation Steps Performed
Built and manually verified that the additional characters appeared and were output following the update.