Closed Bug 1415733 Opened 7 years ago Closed 6 years ago

Migrate the "Search" section of Preferences to the new Localization API

Categories

(Firefox :: Settings UI, defect, P3)

Product:
Firefox
Component:
Settings UI
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Firefox 61
Tracking Flags:
Tracking Status
firefox61 --- fixed

People

(Reporter: zbraniecki, Assigned: zbraniecki)

References

Details

Attachments

(2 files, 1 obsolete file)

Bug 1415733 - Migrate the "Search" section of Preferences to the new Localization API.
59 bytes, text/x-review-board-request
flod
: review+
Gijs
: review+
Details
Screenshot of the area being refactored
51.82 KB, image/png
Details 
This bug will cover the migration work required to move the "Search" section of Preferences to Fluent.

URL: http://searchfox.org/mozilla-central/source/browser/components/preferences/in-content/sync.xul
Assignee: nobody → gandalf
Blocks: 1415730
Status: NEW → ASSIGNED
Priority: -- → P3
Comment on attachment 8926739 [details]
Bug 1415733 - Migrate the "Search" section of Preferences to the new Localization API.

https://reviewboard.mozilla.org/r/197978/#review203234

::: browser/locales/en-US/browser/preferences/search.ftl:1
(Diff revision 2)
> +pane-search-title = Search

This string is defined in preferences.dtd. Defining it here means that we need to duplicate it later? I think it should remain in a shared file (not sure if main.ftl, since this seems to be built on top of other code)

http://searchfox.org/mozilla-central/rev/c99d035f00dd894feff38e4ad28a73fb679c63a6/browser/components/preferences/in-content/preferences.xul#142

::: browser/locales/en-US/browser/preferences/search.ftl:37
(Diff revision 2)
> +engine-keyword-column
> +    .label = Keyword
> +
> +restore-default-se
> +    .label = Restore Default Search Engines
> +    .accesskey = d

D

::: browser/locales/en-US/browser/preferences/search.ftl:41
(Diff revision 2)
> +    .label = Restore Default Search Engines
> +    .accesskey = d
> +
> +remove-engine
> +    .label = Remove
> +    .accesskey = r

R
Comment on attachment 8926739 [details]
Bug 1415733 - Migrate the "Search" section of Preferences to the new Localization API.

https://reviewboard.mozilla.org/r/197978/#review203266

::: browser/locales/en-US/browser/preferences/search.ftl:54
(Diff revision 2)
> +duplicate-engine-msg =
> +    You have chosen a keyword that is currently in use by “{ $name }”.
> +    Please select another.
> +
> +duplicate-bookmark-msg =
> +    You have chosen a keyword that is currently in use by a bookmark.

Did you go on a new line here for formatting reasons, or do you have a new line because the second part of the string should be displayed on a new line?

I would assume the former, because that's what the current code does.

Isn't Fluent going to display this on two lines, i.e. "You have chosen a keyword that is currently in use by a bookmark.\nPlease select another."?
Comment on attachment 8926739 [details]
Bug 1415733 - Migrate the "Search" section of Preferences to the new Localization API.

https://reviewboard.mozilla.org/r/197978/#review203400

::: browser/components/preferences/in-content/search.js:305
(Diff revision 2)
> -        let emsg = strings.getFormattedString("duplicateEngineMsg", [dupName]);
> +        ] = await document.l10n.formatValues([
> +          ["duplicate-title"],
> +          eduplicate ?
> +            ["duplicate-engine-msg", { name: dupName }] :
> +            ["duplicate-bookmark-msg"],
> +        ]);

I'd make this three statements:

let msgids = ['duplicate-title'];
if (eduplicate) {
  msgids.push([..{}]);
}
else {
}
let [dtitle, msg] = await document.l10n.formatValues(msgids);

::: browser/locales/en-US/browser/preferences/search.ftl:22
(Diff revision 2)
> +    .label = Show search suggestions in address bar results
> +    .accesskey = l
> +
> +url-bar-suggestions-permanent-pb =
> +    Search suggestions will not be shown in location bar results
> +    because you have configured &brandShortName; to never remember history.

DTD entity reference should be branding?
Comment on attachment 8926739 [details]
Bug 1415733 - Migrate the "Search" section of Preferences to the new Localization API.

https://reviewboard.mozilla.org/r/197978/#review204174

The ftl file should closely map what a migration can do, in particular keep in-string whitespace.

The two cases below actually end up in Services.prompt.alert(), which is whitespace-sensitive.

::: browser/locales/en-US/browser/preferences/search.ftl:51
(Diff revision 3)
> +
> +// Variable
> +//   $name (String) - name of the engine using selected keyword
> +duplicate-engine-msg =
> +    You have chosen a keyword that is currently in use by “{ $name }”.
> +    Please select another.

This is not what we'll have with migrations, we should stick to a 1-1 mapping here so that our en-US testing reflects what migrations will do for localizations.

::: browser/locales/en-US/browser/preferences/search.ftl:55
(Diff revision 3)
> +    You have chosen a keyword that is currently in use by “{ $name }”.
> +    Please select another.
> +
> +duplicate-bookmark-msg =
> +    You have chosen a keyword that is currently in use by a bookmark.
> +    Please select another.

... here, too.
Comment on attachment 8926739 [details]
Bug 1415733 - Migrate the "Search" section of Preferences to the new Localization API.

Thanks for all the feedback so far!

I updated the patch to it and am left with one remaining issue - the jar.mn entries for branding.
The only way I got it to work for en-US at least is to hardcode the en-US, but I suspect that it's not the way it should be in official/aurora/nightly.

Pike - can you help me here?

(I'll still retest that migration produced FTL and the one in en-US match)
Attachment #8926739 - Flags: feedback?(l10n)
Comment on attachment 8926739 [details]
Bug 1415733 - Migrate the "Search" section of Preferences to the new Localization API.

Pike - actually, I think I only need a non-en-US brand for ./official/. Still, mot sure how to get it.

Stas - can you look at the migration file? Does it look reasonable?
Attachment #8926739 - Flags: feedback?(stas)
Attachment #8926739 - Flags: feedback?(l10n)
Comment on attachment 8926739 [details]
Bug 1415733 - Migrate the "Search" section of Preferences to the new Localization API.

https://reviewboard.mozilla.org/r/197978/#review207904

::: python/l10n/fluent_migrations/bug_1415733.py:10
(Diff revision 5)
> +
> +
> +def migrate(ctx):
> +    """Bug 1415733 - Migrate the "Search" section of Preferences to the new Localization API, part {index}"""
> +
> +    ctx.maybe_add_localization(

I realized that perhaps we don't need `maybe_add_localization` at all and filed bug 1420225 about it.

::: python/l10n/fluent_migrations/bug_1415733.py:20
(Diff revision 5)
> +
> +    ctx.add_transforms(
> +        'browser/branding/brand.ftl',
> +        'browser/branding/locales/en-US/brand.ftl',
> +        [
> +            FTL.Message(

This will need to be a private message in Fluent Syntax 0.5. I wonder if it would be easier to wait for 0.5 to land in fluent.js and python-fluent before running this migration.

::: python/l10n/fluent_migrations/bug_1415733.py:228
(Diff revision 5)
> +                value=COPY(
> +                    REPLACE(
> +                        'browser/chrome/browser/engineManager.properties',
> +                        'duplicateEngineMsg',
> +                        {
> +                            '%S': EXTERNAL_ARGUMENT(

In theory, we need bug 1317336 to land first before we can reliable support %S placeables in properties files.

In practice, in this particular case all existing translations use the literal %S which is well-supported by the migration code:

https://transvision.mozfr.org/string/?entity=browser/chrome/browser/engineManager.properties:duplicateEngineMsg&repo=gecko_strings
Thanks :stas!

I got the migration complete and was able to get the branding work, so I think this is ready for a more complete review even if we'll want to wait for some migration code updates before landing it.
Comment on attachment 8926739 [details]
Bug 1415733 - Migrate the "Search" section of Preferences to the new Localization API.

https://reviewboard.mozilla.org/r/197978/#review211422

I think this should be two patches, one for the pane controlling search engines, and one for the search-in-preferences feature.

As I mentioned in bug 1411012, too, please don't use abbreviations in IDs, that's just making code harder to read.

Also, I notice that we're using '-' both for English and for hiearchy. Like `show-url-bar-suggestions`. Is that a bug or a feature?

Lastly, now that we'll have grammatical data in branding, should we expose all pre-release-channel branding files to localizers? Or are we OK with pre-release channels having funky grammar? Also fine to be discussed in a dedicated bug.

::: browser/branding/aurora/locales/en-US/brand.ftl:1
(Diff revision 7)
> +brand-short-name = Firefox

Needs a license header

::: browser/branding/nightly/locales/en-US/brand.ftl:1
(Diff revision 7)
> +brand-short-name = Nightly

Also, license header.

::: browser/locales/en-US/browser/preferences/search.ftl:10
(Diff revision 7)
> +    .label = Use the address bar for search and navigation
> +search-bar-shown
> +    .label = Add search bar in toolbar
> +
> +default-search-engine = Default Search Engine
> +default-search-engine-cta = Choose the default search engine to use in the address bar and search bar.

no `-cta`

::: browser/locales/en-US/browser/preferences/search.ftl:20
(Diff revision 7)
> +
> +show-url-bar-suggestions
> +    .label = Show search suggestions in address bar results
> +    .accesskey = l
> +
> +url-bar-suggestions-permanent-pb = Search suggestions will not be shown in location bar results because you have configured { brand-short-name } to never remember history.

no `-pb`

::: browser/locales/en-US/browser/preferences/search.ftl:24
(Diff revision 7)
> +
> +url-bar-suggestions-permanent-pb = Search suggestions will not be shown in location bar results because you have configured { brand-short-name } to never remember history.
> +
> +one-click-search-engines = One-Click Search Engines
> +
> +one-click-search-engines-cta = Choose the alternative search engines that appear below the address bar and search bar when you start to enter a keyword.

`-cta`

::: browser/locales/en-US/browser/preferences/search.ftl:31
(Diff revision 7)
> +engine-name-column
> +    .label = Search Engine
> +engine-keyword-column
> +    .label = Keyword
> +
> +restore-default-se

`-se` should be spelled out.

::: browser/locales/en-US/browser/preferences/search.ftl:39
(Diff revision 7)
> +
> +remove-engine
> +    .label = Remove
> +    .accesskey = R
> +
> +find-more-se = Find more search engines

`-se`

::: python/l10n/fluent_migrations/bug_1415733.py:1
(Diff revision 7)
> +# coding=utf8

License header.

::: python/l10n/fluent_migrations/bug_1415733.py:17
(Diff revision 7)
> +    ctx.maybe_add_localization(
> +        'browser/chrome/browser/preferences/search.dtd')
> +    ctx.maybe_add_localization(
> +        'browser/chrome/browser/preferences/preferences.dtd')
> +    ctx.maybe_add_localization(
> +        'browser/chrome/browser/engineManager.properties')

We don't need these anymore?
Attachment #8926739 - Flags: review?(l10n) → review-
Depends on: 1424682
No longer depends on: 1424682
Attachment #8926739 - Attachment is obsolete: true
Comment on attachment 8958314 [details]
Bug 1415733 - Migrate the "Search" section of Preferences to the new Localization API.

https://reviewboard.mozilla.org/r/227248/#review233080

::: browser/components/preferences/in-content/search.js:361
(Diff revision 1)
>  
>        // Notify the user if they have chosen an existing engine/bookmark keyword
>        if (eduplicate || bduplicate) {
> -        let strings = document.getElementById("engineManagerBundle");
> -        let dtitle = strings.getString("duplicateTitle");
> -        let bmsg = strings.getString("duplicateBookmarkMsg");
> +        let msgids = [["duplicate-title"]];
> +        if (eduplicate) {
> +          msgids.push(["duplicate-engine-msg", { name: dupName }]);

I think you have the wrong l10n-ids here, unless I'm misunderstanding the code?

::: browser/locales/en-US/browser/preferences/preferences.ftl:408
(Diff revision 1)
> +
> +search-suggestions-cant-show = Search suggestions will not be shown in location bar results because you have configured { -brand-short-name } to never remember history.
> +
> +search-one-click-header = One-Click Search Engines
> +
> +search-choose-alternative-engines = Choose the alternative search engines that appear below the address bar and search bar when you start to enter a keyword.

search-one-click-desc for consistency (it's the description of this section)

::: browser/locales/en-US/browser/preferences/preferences.ftl:425
(Diff revision 1)
> +    .label = Remove
> +    .accesskey = R
> +
> +search-find-more-link = Find more search engines
> +
> +search-keyword-warning-title = Duplicate Keyword

Add a comment here to avoid possible confusion:

# This warning is displayed when the chosen keyword is already in use
# ('Duplicate' is an adjective)

::: browser/locales/en-US/browser/preferences/preferences.ftl:426
(Diff revision 1)
> +    .accesskey = R
> +
> +search-find-more-link = Find more search engines
> +
> +search-keyword-warning-title = Duplicate Keyword
> +search-keyword-warning-engine = You have chosen a keyword that is currently in use by “{ $name }”. Please select another.

Worth adding a note that $name is the name of a search engine.

::: python/l10n/fluent_migrations/bug_1415733_preferences_search.py:122
(Diff revision 1)
> +                value=REPLACE(
> +                    'browser/chrome/browser/preferences/search.dtd',
> +                    'urlBarSuggestionsPermanentPB.label',
> +                    {
> +                        '&brandShortName;': MESSAGE_REFERENCE(
> +                            'brand-short-name'

Missing leading -
Not sure why from the diff, but this patch fails to apply on central for me. Can you check if it needs a rebase?
It's based on the search results patch. Sorry for that!
Comment on attachment 8958314 [details]
Bug 1415733 - Migrate the "Search" section of Preferences to the new Localization API.

https://reviewboard.mozilla.org/r/227248/#review233382

Looks good to me.
Attachment #8958314 - Flags: review?(francesco.lodolo) → review+
Attachment #8958314 - Flags: review?(jaws) → review?(gijskruitbosch+bugs)
Comment on attachment 8958314 [details]
Bug 1415733 - Migrate the "Search" section of Preferences to the new Localization API.

https://reviewboard.mozilla.org/r/227248/#review233668

LGTM. FWIW, I noticed android and various other things running on the trypush. Those don't use the prefs. For future reference, something like this:

try: -b od -p win64,macosx64,linux64,linux64-asan -u mochitest-bc,firefox-ui-functional,marionette,marionette-e10s -t none --artifact

will get you all the test coverage the prefs have, I think, with artifact builds to make it go more quickly. :-)

(the tier-2 firefox ui en-US tests have a single high-frequency-intermittent-orange to do with search keyword attribute for artifact builds, but otherwise it should Just Work.)

::: browser/components/preferences/in-content/findInPage.js:296
(Diff revision 3)
>          document.l10n.setAttributes(msgElem, "search-results-sorry-message", {
>            query: this.query
>          });
>          let helpUrl = Services.urlFormatter.formatURLPref("app.support.baseURL") + "preferences";
>          let helpContainer = document.getElementById("need-help");
> -        let link = helpContainer.querySelector("a").setAttribute("href", helpUrl);
> +        helpContainer.querySelector("a").setAttribute("href", helpUrl);

Stray hunk from the other bug?
Attachment #8958314 - Flags: review?(gijskruitbosch+bugs) → review+
Pushed by zbraniecki@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/647154f30332
Migrate the "Search" section of Preferences to the new Localization API. r=flod,Gijs
https://hg.mozilla.org/mozilla-central/rev/647154f30332
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox61: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 61
Pushed by ncsoregi@mozilla.com:
https://hg.mozilla.org/mozilla-central/rev/c87a57081a24
Migrate the "Search" section of Preferences to the new Localization API. r=flod,Gijs a=reland
Status: REOPENED → RESOLVED
Closed: 6 years ago → 6 years ago
status-firefox61: affected → fixed
Flags: needinfo?(gandalf)
Resolution: --- → FIXED
Target Milestone: --- → Firefox 61
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: