Broke in Bug 215101 - Web Inspector: "Show transparency grid" string needs to be disambiguated for localization
Created attachment 406401 [details] Patch
I ran Tools/Scripts/update-webkit-localizable-strings to generate this patch.
<rdar://problem/66855263>
I don't understand how this fix could be correct. The original patch was supposed to create two localizable strings. Now there's only one.
How did you generate Localizations/en.lproj/localizedStrings.js in your patch?
When two parameters are passed to WI.UIString, they are "string" and "comment" (not a key!): ``` WI.UIString = function(string, key, comment) { "use strict"; if (WI.dontLocalizeUserInterface) return string; // UIString(string, comment) if (arguments.length === 2) { ``` It seems to me you want to specify two different keys. One way to do that would be to specify the 3rd parameter. Brian, do you want to take this bug?
(In reply to Nikita Vasilyev from comment #6) > When two parameters are passed to WI.UIString, they are "string" and > "comment" (not a key!): > > ``` > WI.UIString = function(string, key, comment) > { > "use strict"; > > if (WI.dontLocalizeUserInterface) > return string; > > // UIString(string, comment) > if (arguments.length === 2) { > ``` > > It seems to me you want to specify two different keys. One way to do that > would be to specify the 3rd parameter. > > Brian, do you want to take this bug? Ugh, I hate positional parameters. I'll fix this today.
Created attachment 406427 [details] Patch
Comment on attachment 406427 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=406427&action=review > Source/WebInspectorUI/Localizations/en.lproj/localizedStrings.js:1153 > -localizedStrings["Show transparency grid (tooltip)"] = "Show transparency grid"; > +/* Show transparency grid */ > localizedStrings["Show transparency grid (settings label)"] = "Show transparency grid"; > +/* Show transparency grid */ > +localizedStrings["Show transparency grid (tooltip)"] = "Show transparency grid"; We don't copy the same text as a comment. While this fixes the bug, that's not a good practice. The comments provide localizers with some context. Check out other comments in this file to get a better idea of an appropriate comment.
Comment on attachment 406427 [details] Patch What you have is fine (nice first bug!), butto echo @Brian Burg (In reply to Brian Burg from comment #7) > Ugh, I hate positional parameters. I'll fix this today. I feel like a better long-term solution to this would be to just eliminate the `(string, comment)` variant and instead always require a `key` if a `comment` is needed (a `key` can sometimes be enough of a comment anyways, such as bug). That way, bugs like this can be avoided :) If you want to do that, we'd need to change: - `Source/Tools/Scripts/extract-localizable-js-strings` to not use the 2nd argument as the `comment` when only 2 arguments are provided - `WI.UIString` to not swap the `comment` and `key` when only 2 arguments are provided - existing callers of `WI.UIString` (this can be done quickly just by looking at `Source/WebInspectorUI/Localizations/en.lproj/localizedStrings.js`) This logic was added in r245827, so I'd recommend looking at that to see how it used to be before :)
Created attachment 406429 [details] Patch
(In reply to Devin Rousso from comment #10) > Comment on attachment 406427 [details] > Patch > > What you have is fine (nice first bug!), butto echo @Brian Burg > > (In reply to Brian Burg from comment #7) > > Ugh, I hate positional parameters. I'll fix this today. > > I feel like a better long-term solution to this would be to just eliminate > the `(string, comment)` variant and instead always require a `key` if a > `comment` is needed (a `key` can sometimes be enough of a comment anyways, > such as bug). That way, bugs like this can be avoided :) > > If you want to do that, we'd need to change: > - `Source/Tools/Scripts/extract-localizable-js-strings` to not use the 2nd > argument as the `comment` when only 2 arguments are provided > - `WI.UIString` to not swap the `comment` and `key` when only 2 arguments > are provided > - existing callers of `WI.UIString` (this can be done quickly just by > looking at > `Source/WebInspectorUI/Localizations/en.lproj/localizedStrings.js`) > > This logic was added in r245827, so I'd recommend looking at that to see how > it used to be before :) Agree, but let's make a separate bug. Here: https://bugs.webkit.org/show_bug.cgi?id=215400
(In reply to Nikita Vasilyev from comment #9) > Comment on attachment 406427 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=406427&action=review > > > Source/WebInspectorUI/Localizations/en.lproj/localizedStrings.js:1153 > > -localizedStrings["Show transparency grid (tooltip)"] = "Show transparency grid"; > > +/* Show transparency grid */ > > localizedStrings["Show transparency grid (settings label)"] = "Show transparency grid"; > > +/* Show transparency grid */ > > +localizedStrings["Show transparency grid (tooltip)"] = "Show transparency grid"; > > We don't copy the same text as a comment. While this fixes the bug, that's > not a good practice. > > The comments provide localizers with some context. Check out other comments > in this file to get a better idea of an appropriate comment. Thanks! I've updated the comments in the latest patch to provide better context as to where the strings appear.
Comment on attachment 406429 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=406429&action=review > Source/WebInspectorUI/Localizations/en.lproj/localizedStrings.js:1153 > -localizedStrings["Show transparency grid (tooltip)"] = "Show transparency grid"; > +/* Settings tab checkbox label for whether the transparency grid is shown by default */ > localizedStrings["Show transparency grid (settings label)"] = "Show transparency grid"; > +/* Tooltip for showing the checkered transparency grid under images and canvases */ > +localizedStrings["Show transparency grid (tooltip)"] = "Show transparency grid"; It seems to me that you edited this file directly. This is an auto-generated file. You shouldn't edit it directly. Run `Tools/Scripts/update-webkit-localizable-strings` script to update it instead. > Source/WebInspectorUI/UserInterface/Views/CanvasContentView.js:50 > + this._showGridButtonNavigationItem = new WI.ActivateButtonNavigationItem("show-grid", WI.UIString("Show transparency grid", "Show transparency grid (tooltip)", "Show transparency grid"), WI.UIString("Hide transparency grid"), "Images/NavigationItemCheckers.svg", 13, 13); The comment should be the 3rd argument here. Instead of "Show transparency grid", it should be "Tooltip for showing ...". Same goes for every string you changed.
Comment on attachment 406429 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=406429&action=review >> Source/WebInspectorUI/UserInterface/Views/CanvasContentView.js:50 >> + this._showGridButtonNavigationItem = new WI.ActivateButtonNavigationItem("show-grid", WI.UIString("Show transparency grid", "Show transparency grid (tooltip)", "Show transparency grid"), WI.UIString("Hide transparency grid"), "Images/NavigationItemCheckers.svg", 13, 13); > > The comment should be the 3rd argument here. Instead of "Show transparency grid", it should be "Tooltip for showing ...". > > Same goes for every string you changed. along these lines, it may be worth creating a `WI.repeatedUIString.showTransparencyGridTooltip` and a `WI.repeatedUIString.showTransparencyGridSetting` to save the trouble of repeating all these strings
(In reply to Devin Rousso from comment #15) > Comment on attachment 406429 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=406429&action=review > > >> Source/WebInspectorUI/UserInterface/Views/CanvasContentView.js:50 > >> + this._showGridButtonNavigationItem = new WI.ActivateButtonNavigationItem("show-grid", WI.UIString("Show transparency grid", "Show transparency grid (tooltip)", "Show transparency grid"), WI.UIString("Hide transparency grid"), "Images/NavigationItemCheckers.svg", 13, 13); > > > > The comment should be the 3rd argument here. Instead of "Show transparency grid", it should be "Tooltip for showing ...". > > > > Same goes for every string you changed. > > along these lines, it may be worth creating a > `WI.repeatedUIString.showTransparencyGridTooltip` and a > `WI.repeatedUIString.showTransparencyGridSetting` to save the trouble of > repeating all these strings I was going to summit almost the same comment 😅
(In reply to Devin Rousso from comment #15) > Comment on attachment 406429 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=406429&action=review > > >> Source/WebInspectorUI/UserInterface/Views/CanvasContentView.js:50 > >> + this._showGridButtonNavigationItem = new WI.ActivateButtonNavigationItem("show-grid", WI.UIString("Show transparency grid", "Show transparency grid (tooltip)", "Show transparency grid"), WI.UIString("Hide transparency grid"), "Images/NavigationItemCheckers.svg", 13, 13); > > > > The comment should be the 3rd argument here. Instead of "Show transparency grid", it should be "Tooltip for showing ...". > > > > Same goes for every string you changed. > > along these lines, it may be worth creating a > `WI.repeatedUIString.showTransparencyGridTooltip` and a > `WI.repeatedUIString.showTransparencyGridSetting` to save the trouble of > repeating all these strings Is there any value in creating a `WI.repeatedUIString.showTransparencyGridSetting` given it is only used once for the Settings tab? The tooltip certainly makes sense to define once.
(In reply to Patrick Angle from comment #17) > Is there any value in creating a > `WI.repeatedUIString.showTransparencyGridSetting` given it is only used once for the Settings tab? The tooltip certainly makes sense to define once. oh yeah if it's only used once then no I wouldn't bother :P
Created attachment 406436 [details] Patch
Comment on attachment 406436 [details] Patch r=me, nice work! As a general piece of feedback it's always a good idea to include additional info/context in the ChangeLog so that when looking back at this change in the future there's a concise and clear explanation of what and why. Even something as simple as "Create repeated `WI.UIString ` helper" would be helpful :)
Committed r265675: <https://trac.webkit.org/changeset/265675> All reviewed patches have been landed. Closing bug and clearing flags on attachment 406436 [details].