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.

Sign up for GitHub

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 for issue #4090: cmd.js line 20-26 replaced with OS-specific keyboard shortcut mappings with localization. #10570

Draft
wants to merge 3 commits into
base: develop
Choose a base branch
from
+9 −6
Draft

Fix for issue #4090: cmd.js line 20-26 replaced with OS-specific keyboard shortcut mappings with localization. #10570

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
34d48e6
Fix for issue #4090: cmd.js line 20-26 replaced with OS-specific keyb…
StillAbeginnerr Nov 30, 2024
959399e
Fix for issue #4090: cmd.js line 20-26 replaced with OS-specific keyb…
StillAbeginnerr Nov 30, 2024
672b376
Fix for issue #4090: cmd.js line 20-26 replaced with OS-specific keyb…
StillAbeginnerr Nov 30, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 8 additions & 6 deletions modules/ui/cmd.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,14 +14,14 @@ export var uiCmd = function (code) {
if (detected.os === 'win') {
if (code === '⌘⇧Z') return 'Ctrl+Y';
}

var mac = (detected.os === 'mac');
var result = '',
replacements = {
'⌘': 'Ctrl',
'⇧': 'Shift',
'⌥': 'Alt',
'⌫': 'Backspace',
'⌦': 'Delete'
'⌘': mac ? '⌘ ' : t('shortcuts.key.ctrl'),
'⇧': mac ? '⇧ ' : t('shortcuts.key.shift'),
'⌥': mac ? '⌥ ' : t('shortcuts.key.alt'),
'⌫': mac ? '⌫ ' : t('shortcuts.key.backspace'),
'⌦': mac ? '⌦ ' : t('shortcuts.key.del'),
Comment on lines +20 to +24
Copy link
Collaborator

@1ec5 1ec5 Dec 1, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is essentially the same as what c22fa60 did in uiCmd.display down below to fix the Keyboard Shortcuts screen. I suspect c22fa60 didn’t touch this function because it’s also being used in places where raw key identifiers are needed rather than human-readable labels. For example, the following code uses the return value to register the actual keyboard shortcut:

iD/modules/ui/full_screen.js

Lines 72 to 73 in 3025d4f

var keys = (detected.os === 'mac' ? [uiCmd('⌃⌘F'), 'f11'] : ['f11']);
context.keybinding().on(keys, fullScreen);

iD/modules/ui/zoom.js

Lines 102 to 110 in 3025d4f

utilKeybinding.plusKeys.forEach(function(key) {
context.keybinding().on([key], zoomIn);
context.keybinding().on([uiCmd('⌥' + key)], zoomInFurther);
});
utilKeybinding.minusKeys.forEach(function(key) {
context.keybinding().on([key], zoomOut);
context.keybinding().on([uiCmd('⌥' + key)], zoomOutFurther);
});

If you run this branch on macOS, or on Windows with the German localization, will the shortcuts function correctly when you press them? If not, then maybe we need to revert this function to what it was previously. If we look back at the code I pointed to earlier, notice how it’s only being used to create the tooltip – so it’s probably only for display purposes:

iD/modules/ui/sections/data_layers.js

Line 404 in b5d45e3

.keys([uiCmd('⌘⇧' + t('info_panels.history.key'))])

If that’s the case, then changing that code to call uiCmd.display instead might fix the issue for that particular tooltip. So instead of implementing anything new, you’d be reviewing each of the calls to uiCmd, deciding whether it should be replaced with a call to uiCmd.display.

Copy link
Author

@StillAbeginnerr StillAbeginnerr Dec 1, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank very much for the detailed explanation, it's very helpful and I now understood with more clarity, gotta read more about this codebase, and I've marked the calls where uiCmd been called and now checking how useful, the uiCmd.display would be for a particular tooltip...will submit a pull request again after high detailed review of each call.

Copy link
Author

@StillAbeginnerr StillAbeginnerr Dec 3, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@1ec5

While working, I found these 2 issues/bug

  1. Issue with Key Functionality.

It seems that the keyboard shortcut for ? isn’t functioning as expected. Based on testing, it appears the shortcut should be labeled as Shift + ? instead of just ?, as pressing ? alone triggers the black overlay below the inspect button in the top-left corner.


Image 1 :

image

In Image 2, The Keyboard Shortcut should also be labeled as Shift + ?.

Image 2 :

image
  1. Interaction Limitation with Quality Assurance Tab
    When the Quality Assurance tab is open in the Help section, the Keyboard Shortcuts option becomes unclickable. Interestingly, this behavior is not observed with other tabs, which allow seamless access to the keyboard shortcuts.

Image 1 : For example, the Keyboard Shortcuts options is clickable in case of points tab.


image

Image 2 : When the Quality Assurance tab is open in the Help section, the Keyboard Shortcuts option becomes unclickable.


WhatsApp Image 2024-12-03 at 11 03 13

Checked with latest changes on both local host and online deployment.

Copy link
Author

@StillAbeginnerr StillAbeginnerr Dec 3, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

image

Help/Keyboard Shortcuts.

I’ve resolved the labeling issue mentioned in the previous comment (Issue 1, Image 2) by replacing ? with Shift + ? in the relevant file for "Show Keyboard Shortcuts". Should I create a separate issue and raise a PR for this fix, or is it okay to include it as part of this current work?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems that the keyboard shortcut for ? isn’t functioning as expected. Based on testing, it appears the shortcut should be labeled as Shift + ? instead of just ?, as pressing ? alone triggers the black overlay below the inspect button in the top-left corner.

I think spelling it as just “?” is a standard convention, at least on macOS. If you’re using an English keyboard layout, pressing that key alone would be /, not ?. A user-selectable keyboard layout could conceivably have ? as the primary function of a key, or it could require a modifier key other than Shift. All that matters to a Web application like iD is whether the user has pressed the some key combination that would input “?” into a text field.

Copy link
Author

@StillAbeginnerr StillAbeginnerr Dec 5, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

reference undo_redo.js

image

WhatsApp Image 2024-12-04 at 18 10 59

Line 4 :
if we check by using console.log(uiCmd('⌘Z')); // Outputs either '⌘Z' on Mac or 'Ctrl+Z' on Windows/Linux

and now if we replace that with console.log(uiCmd.display('⌘Z')); // Outputs either '⌘Z' on Mac or '⌘Z' on Windows/Linux

Now, I observed that uiCmd outputs 'CTRL+Z' on German localization or any other regional language, like in case of german it should be STRG+Z when using uiCmd.display but this function shows Outputs either '⌘Z' on Mac or '⌘Z' on Windows/Linux.

It looks like there's some inconsistency with uiCmd.display when handling localization for keyboard shortcuts. While uiCmd correctly adapts to regional differences (e.g., showing "STRG+Z" in German locales) on applying t('shortcuts.key.ctrl') there, uiCmd.display seems to bypass this behavior and hardcodes "⌘Z" for both Mac and Windows/Linux.

I tried to fix this bug using the localization function t(), and it worked with uiCmd but not with uiCmd.display. I'm a bit unsure whether using uiCmd.display for this undo action is a bad idea, or if it's an exception due to hardcoding, or if there's something else I'm missing.

@1ec5 I'm kind of stuck on this, the problem I'm facing is like whether there's need for a smaller fix in the uiCmd function, not sure if its a exceptional case or I should think from a new implementation perspective which can be very challenging for a new contributor like me as I should be reviewing calls to these and deciding which function to be used ( uiCmd or uiCmd.display )

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How are you testing Windows or Linux? Are you testing on an actual Windows or Linux machine, spoofing the user agent string, or something else?

Copy link
Author

@StillAbeginnerr StillAbeginnerr Jan 10, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I use macOS 15.1.1 to test how things work on Windows and Linux by spoofing the user agent string. When I need to double-check or confirm any issues or assumptions, I rely on a real Windows 10 PC of my friend.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The Keyboard Shortcuts pane has already been using uiCmd.display; is it also showing macOS key names on Windows or Linux?

Copy link
Author

@StillAbeginnerr StillAbeginnerr Jan 10, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Checked throughly, there are no signs of macOS key names on Windows and Linux.

But since, the keyboard shortcuts pane been using uiCmd.display and there's no problems with it,

I see, it could be just a unique case for undo_redo.js because German localization is not happening on this part,

As we can see there's CTRL + Z on Linux System(I used Parallels Desktop to set up Debian OS) on Firefox Browser which should have been STRG + Z.

image

console.log('Detected OS:', mac ? 'mac' : 'not mac'); // Works Correctly
console.log('Detected OS:', utilDetect().os); // Works Correctly
console.log('Current Language:', detected.language);
console.log('Translated CTRL:', t('shortcuts.key.ctrl')); // Works Correctly
console.log('uiCmd: ⌘Z is', uiCmd('⌘Z')); // Works Correctly except Translation.
console.log('uiCmd.display: ⌘Z is', uiCmd.display('⌘Z')); // not working properly because of code.length issues otherwise would have displayed STRG + Z in German localization.

So I think that this line if (code.length !== 1) return code; returns the non-translated value because for ('⌘Z') length is 2 and replacement doesn't take place in uiCmd.display in cmd.js

But If I change it to like if (code.length !== 2) return code;
[the stack calls exceed](init.js:501 RangeError: Maximum call stack size exceeded) and crashes the webpage.

It uses the uiCmd Function right now in undo_redo.js, but if I want to localise it using uiCmd.display, For Mac/Windows/Linux, ⌘Z appears.

My LocalHost :

image

};

for (var i = 0; i < code.length; i++) {
Expand All @@ -36,12 +36,14 @@ export var uiCmd = function (code) {
};



// return a display-focused string for a given keyboard code
uiCmd.display = function(code) {
if (code.length !== 1) return code;

var detected = utilDetect();
var mac = (detected.os === 'mac');

var replacements = {
'⌘': mac ? '⌘ ' + t('shortcuts.key.cmd') : t('shortcuts.key.ctrl'),
'⇧': mac ? '⇧ ' + t('shortcuts.key.shift') : t('shortcuts.key.shift'),
Expand Down
1 change: 1 addition & 0 deletions modules/ui/sections/data_layers.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -405,6 +405,7 @@ export function uiSectionDataLayers(context) {
.placement('top')
);


historyPanelLabelEnter
.append('input')
.attr('type', 'checkbox')
Expand Down