REGRESSION (r265224): Web Inspector: LOCALIZED STRING NOT FOUND next to the Image checkbox in the Sources prefs panel

• Component: Web Inspector
• Status: RESOLVED FIXED

[Nikita Vasilyev]

Broke in Bug 215101 - Web Inspector: "Show transparency grid" string needs to be disambiguated for localization

[Nikita Vasilyev]

Created attachment 406401 [details]
Patch

[Nikita Vasilyev]

I ran Tools/Scripts/update-webkit-localizable-strings to generate this patch.

[Radar WebKit Bug Importer]

<rdar://problem/66855263>

[Blaze Burg]

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.

[Nikita Vasilyev]

How did you generate Localizations/en.lproj/localizedStrings.js in your patch?

[Nikita Vasilyev]

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?

[Blaze Burg]

(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.

[Patrick Angle]

Created attachment 406427 [details]
Patch

[Nikita Vasilyev]

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.

[Devin Rousso]

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 :)

[Patrick Angle]

Created attachment 406429 [details]
Patch

[Blaze Burg]

(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

[Patrick Angle]

(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.

[Nikita Vasilyev]

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.

[Devin Rousso]

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

[Nikita Vasilyev]

(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 😅

[Patrick Angle]

(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.

[Devin Rousso]

(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

[Patrick Angle]

Created attachment 406436 [details]
Patch

[Devin Rousso]

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 :)

[EWS]

Committed r265675: <https://trac.webkit.org/changeset/265675>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 406436 [details].