356821 - UI wording change for search engine addition dialog

Status: RESOLVED FIXED

(Firefox :: Search, enhancement, P4)

Description

[Simon Ruggier]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-GB; rv:1.8.1) Gecko/20061003 Firefox/2.0
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-GB; rv:1.8.1) Gecko/20061003 Firefox/2.0

Minor suggestion:  I found the "Start using it right away" checkbox to be ever so slightly unclear - a user may think that leaving the box unchecked means they will have to reload the browser or something.  I put forward "Select this search engine now", or "Make this the current search engine now" as clearer alternatives.

Reproducible: Always

Comment 1

[:Gavin Sharp [email: [e-mail]]]

Yeah, I think one of those suggestions would probably be better.

Comment 2

[Mike Connor [:mconnor]]

nice to have, sure.

Comment 3

[Alex Limi (:limi) — Firefox UX Team]

"Make this the current search engine" works.

Comment 4

[Marco Bonardo [:mak]]

http://mxr.mozilla.org/mozilla-central/source/toolkit/locales/en-US/chrome/search/search.properties#7

addEngineUseNowText=Start &using it right away

Comment 5

[Caitlin Potter (:caitp)]

Attachment: Update addEngineUseNowText for locale en-US

Thought I'd submit a quick patch. I haven't figured out how to actually display a control which uses the addEngineUseNowText string, so I don't know how it looks exactly.

Comment 6

[Marco Bonardo [:mak]]

Comment on attachment 723490 [details] [diff] [review]
Update addEngineUseNowText for locale en-US

Review of attachment 723490 [details] [diff] [review]:
-----------------------------------------------------------------

Thank you for your contribution.

The string is used here
http://mxr.mozilla.org/mozilla-central/source/toolkit/components/search/nsSearchService.js#1274
and that method is invoked from here:
http://mxr.mozilla.org/mozilla-central/source/toolkit/components/search/nsSearchService.js#1385

The above comments says that this happens only for calls to nsIBrowserSearchService::addEngine with the 4th parameter true. a callpoint I found for that case is here:
http://mxr.mozilla.org/mozilla-central/source/browser/components/sidebar/nsSidebar.js#134

This is addSearchEngine that is a method that can be invoked by web pages to ask the user to add an engine, so it makes sense it shows a confirm dialog.
So, to test this you should either find a web page using window.sidebar.addSearchEngine, or make one.

::: toolkit/locales/en-US/chrome/search/search.properties
@@ +4,4 @@
>  
>  addEngineConfirmTitle=Add Search Engine
>  addEngineConfirmation=Add "%S" to the list of engines available in the search bar?\n\nFrom: %S
> +addEngineUseNowText=Make this the &current search engine

When changing a localization string, you must always change its identifier, in this case "addEngineUseNowText", I think "addEngineAsCurrentText" will be a good new value.

Finally, I think it's worth to keep using the same letter as shortcut, it was "u" from "using", you may just use "u" from "current", so c&urrent. This should avoid breaking users muscle memory (even if likely doesn't matter much for this randomly seen dialog)

Comment 7

[Caitlin Potter (:caitp)]

Changing the identifier means that nsSearchService.js would need to be updated too, though, is that not right? And if it were, would not each locale also need to be updated to include the new string?

Comment 8

[Caitlin Potter (:caitp)]

Attachment: New en-US localization string addEngineAsCurrentText

Added new en-US localization string 'addEngineAsCurrentText'

Added new localization string identifier addEngineAsCurrentText, with the value "Make this the c&urrent search engine".

Updated toolkit/components/search/nsSearchService.js to use this localization string, rather than the previous 'addEngineUseNowText'.

Tested alternative locales using 'Quick Locale Switcher' plugin, but the entire confirmation dialog would only display using en-US strings, even though other areas of the UI would use strings for different locales.

Comment 9

[Marco Bonardo [:mak]]

(In reply to snowball from comment #7)
> Changing the identifier means that nsSearchService.js would need to be
> updated too

yes

> And if it were, would not each
> locale also need to be updated to include the new string?

yes, but that's up to the localization teams, we just manage en-US

Comment 10

[Marco Bonardo [:mak]]

Comment on attachment 724418 [details] [diff] [review]
New en-US localization string addEngineAsCurrentText

Review of attachment 724418 [details] [diff] [review]:
-----------------------------------------------------------------

There is something wrong with your patch header, please verify you are following documentation at https://developer.mozilla.org/en-US/docs/Creating_a_patch and https://developer.mozilla.org/en/Mercurial_FAQ#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F

The patch itself looks correct, though I will take a look at the final version to ensure the format is correct before a formal review.

I don't think we need further ui-review considered comment 3, though feedback from Limi or Gavin on the string would clearly be welcome.

::: toolkit/components/search/nsSearchService.js
@@ +1271,5 @@
>                                            [this._name, this._uri.host], 2);
>      var checkboxMessage = null;
>      if (!getBoolPref(BROWSER_SEARCH_PREF + "noCurrentEngine", false))
> +      checkboxMessage =
> +      stringBundle.GetStringFromName("addEngineAsCurrentText");

please keep it onelined, it's true we usually request 80 chars limit in code style, but there is some flexibility in that request, especially in these cases where it's just a few (1) chars over

Comment 11

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 724418 [details] [diff] [review]
New en-US localization string addEngineAsCurrentText

You'll also need to remove the old string (addEngineUseNowText) from search.properties, since it's now unused.

Limi signed off on this already, no need to block on feedback again.

Comment 12

[Caitlin Potter (:caitp)]

Attachment: New en-US localization string addEngineAsCurrentText

- Added new localization string identifier addEngineAsCurrentText, with
  the value "Make this the c&urrent search engine".

- Removed old localization string identifier 'addEngineUseNowText'

- Updated toolkit/components/search/nsSearchService.js to use this
  localization string, rather than the previous 'addEngineUseNowText'.

--

Change on nsSearchService.js is not line-wrapped to 80 columns, per :mak's suggestion

Removed addEngineUseNowText identifier, per Gavin's request

Comment 13

[Marco Bonardo [:mak]]

Comment on attachment 725530 [details] [diff] [review]
New en-US localization string addEngineAsCurrentText

Review of attachment 725530 [details] [diff] [review]:
-----------------------------------------------------------------

The commit message should be in the form "Bug 356821 - Update add search engine as current checkbox label. r=mak"

you don't need that may lines of description for simple changes, a couple lines description are welcome for big patches usually, oneline comments are enough for this kind of change.

The patch format now looks correct.

r=me, provided you reduce the patch commit message as suggested

Comment 14

[Caitlin Potter (:caitp)]

Attachment: New en-US localization string addEngineAsCurrentText

Fixed commit message per :mak's request

Comment 15

[Caitlin Potter (:caitp)]

Is this check-in-able?

Comment 16

[Caitlin Potter (:caitp)]

Attachment: New en-US localization string addEngineAsCurrentText

Same patch, changed email address to match my other submissions.

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6588ec4909a4

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/6588ec4909a4