1612679 - Fix Show all bookmarks shortcut for Windows in locales

Status: RESOLVED FIXED

(Mozilla Localizations :: Other, defect)

Description

[Francesco Lodolo [:flod]]

# Verify what shortcut for that operation
# are recommended by the Human Interface Guidelines
# of each platform for your locale.
bookmark-show-all-shortcut =
    .key = { PLATFORM() ->
        [linux] O
       *[other] B
    }

That should have been I, not B, since it comes from
https://hg.mozilla.org/l10n/gecko-strings/file/default/browser/chrome/browser/browser.dtd#l326

Also using this bug to format the file like the content serialized by Pontoon and Fluent libraries, since it helps me running checks on this kind of keys.

Comment 1

[Francesco Lodolo [:flod]]

@zibi
I'm completely lost. Can you clarify what the reasoning was behind creating this key?

The comment in the FTL file has nothing to do with the comment for those two entities in the DTD
https://hg.mozilla.org/l10n/gecko-strings/file/default/browser/chrome/browser/browser.dtd#l322

Comment 2

[Zibi Braniecki [:zbraniecki][:gandalf]]

Sure. Before the patch it was:

#ifndef MOZ_WIDGET_GTK
    <key id="manBookmarkKb" key="&bookmarksCmd.commandkey;" command="Browser:ShowAllBookmarks" modifiers="accel,shift"/>
#else
    <key id="manBookmarkKb" key="&bookmarksGtkCmd.commandkey;" command="Browser:ShowAllBookmarks" modifiers="accel,shift"/>
#endif

<!ENTITY bookmarksCmd.commandkey "b">
<!-- LOCALIZATION NOTE (bookmarksGtkCmd.commandkey): This command
  -  key should not contain the letters A-F, since these are reserved
  -  shortcut keys on Linux. -->
<!ENTITY bookmarksGtkCmd.commandkey "o">

after the patch it is:

<key id="manBookmarkKb" data-l10n-id="bookmark-show-all-shortcut" command="Browser:ShowAllBookmarks" modifiers="accel,shift"/>

# Verify what shortcut for that operation
# are recommended by the Human Interface Guidelines
# of each platform for your locale.
bookmark-show-all-shortcut =
    .key = { PLATFORM() ->
        [linux] O
       *[other] B
    }

the comment was added per recommendation from the reviewer.

I think there was no bug in the migration patch, and it reflects the state from before. Am I missing something?

Comment 3

[Francesco Lodolo [:flod]]

I think there was no bug in the migration patch, and it reflects the state from before. Am I missing something?

This is the migration:

bookmark-show-all-shortcut =
    .key = { PLATFORM() ->
        [linux] { COPY(browser_path, "bookmarksGtkCmd.commandkey") }
       *[other] { COPY(browser_path, "bookmarksWinCmd.commandkey") }
    }

bookmarksWinCmd.commandkey is not bookmarksCmd.commandkey. The result is that now all locales have the wrong shortcut for Windows.

This was missed in review, since there was a lot of noise from other changes (capitalization), and only caught it from checks after I run the migrations on all locales.

Comment 4

[Francesco Lodolo [:flod]]

To be clear, what I need to know is if the migration was incorrect, and I need to figure out how to rerun it. From what I've seen so far, that seems to be the case, and we should migrate bookmarksCmd.commandkey for [other].

Comment 5

[Zibi Braniecki [:zbraniecki][:gandalf]]

Yep, you are correct

Comment 6

[Francesco Lodolo [:flod]]

Should be fixed now, e.g.
https://hg.mozilla.org/l10n-central/de/rev/e44e1368e936c0115763782b45393ad0fe85f9d9