UI wording change for search engine addition dialog

RESOLVED FIXED in Future

Status

()

P4
enhancement
RESOLVED FIXED
12 years ago
6 years ago

People

(Reporter: Simon80, Assigned: caitlin.potter)

Tracking

Trunk
Future
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [good-first-bug][mentor=mak])

Attachments

(1 attachment, 4 obsolete attachments)

(Reporter)

Description

12 years ago
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
Yeah, I think one of those suggestions would probably be better.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Hardware: PC → All
Version: unspecified → Trunk
Keywords: uiwanted
nice to have, sure.
Priority: -- → P4
Target Milestone: --- → Future
"Make this the current search engine" works.
Keywords: uiwanted
http://mxr.mozilla.org/mozilla-central/source/toolkit/locales/en-US/chrome/search/search.properties#7

addEngineUseNowText=Start &using it right away
Whiteboard: [good-first-bug][mentor=mak]
(Assignee)

Comment 5

6 years ago
Created attachment 723490 [details] [diff] [review]
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.
Attachment #723490 - Flags: feedback?(mak77)
Attachment #723490 - Flags: feedback?(limi)
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)
Attachment #723490 - Flags: feedback?(mak77) → feedback-
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?
(Assignee)

Comment 8

6 years ago
Created attachment 724418 [details] [diff] [review]
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.
Attachment #723490 - Attachment is obsolete: true
Attachment #723490 - Flags: feedback?(limi)
Attachment #724418 - Flags: feedback?(mak77)
Attachment #724418 - Flags: feedback?(limi)
(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 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
Attachment #724418 - Flags: feedback?(mak77) → feedback+
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.
Attachment #724418 - Flags: feedback?(limi)
(Assignee)

Comment 12

6 years ago
Created attachment 725530 [details] [diff] [review]
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
Assignee: nobody → caitlin.potter
Attachment #724418 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #725530 - Flags: feedback?(mak77)
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
Attachment #725530 - Flags: feedback?(mak77) → review+
(Assignee)

Comment 14

6 years ago
Created attachment 728325 [details] [diff] [review]
New en-US localization string addEngineAsCurrentText

Fixed commit message per :mak's request
Attachment #725530 - Attachment is obsolete: true
(Assignee)

Comment 15

6 years ago
Is this check-in-able?
(Assignee)

Updated

6 years ago
Keywords: checkin-needed
(Assignee)

Comment 16

6 years ago
Created attachment 763043 [details] [diff] [review]
New en-US localization string addEngineAsCurrentText

Same patch, changed email address to match my other submissions.
Attachment #728325 - Attachment is obsolete: true
Attachment #763043 - Flags: review?(mak77)
https://hg.mozilla.org/mozilla-central/rev/6588ec4909a4
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.