Closed Bug 356821 Opened 15 years ago Closed 8 years ago
UI wording change for search engine addition dialog
2.48 KB, patch
|Details | Diff | Splinter Review|
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
nice to have, sure.
Priority: -- → P4
Target Milestone: --- → Future
"Make this the current search engine" works.
http://mxr.mozilla.org/mozilla-central/source/toolkit/locales/en-US/chrome/search/search.properties#7 addEngineUseNowText=Start &using it right away
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 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 ¤t 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?
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.
(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.
- 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 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+
Fixed commit message per :mak's request
Attachment #725530 - Attachment is obsolete: true
Is this check-in-able?
Same patch, changed email address to match my other submissions.
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.