Closed Bug 882598 Opened 7 years ago Closed 7 years ago

Defect - Rename updateAuto.label to reflect string change

Categories

(Firefox for Metro Graveyard :: Install/Update, defect, P1)

defect

Tracking

(Not tracked)

VERIFIED FIXED
Firefox 24

People

(Reporter: flod, Assigned: bbondy)

References

Details

(Whiteboard: feature=defect c=Settings_pane_options_and_about u=metro_firefox_user p=1)

Attachments

(1 file, 3 obsolete files)

https://hg.mozilla.org/mozilla-central/rev/920669704476#l7.12

<!ENTITY updateAuto.label "Automatically install updates (recommended: improved security)">
<!ENTITY updateAuto.label "Automatically install updates from desktop &brandShortName;">

Considering how the string has changed, please change the name of this entity.
Is this string change approved from UX?
Flags: needinfo?(ywang)
Another problem with this string is that is shown not only on Windows 8, but on other OS's as well where it doesn't make any sense at all.
We could fix that by dynamically changing the label, but I like the more simple solution of just changing the string as per Comment 0. Just waiting for Yuan to confirm this is OK.
Sorry ignore Comment 3, I misinterpreted what Comment 0 meant. 
I think comment 0 is just for the entity name itself and not the string right?
On pre-win8 machines I think we need to dynamically change the string to what it was before instead of including "from Desktop Firefox"
(In reply to Brian R. Bondy [:bbondy] from comment #4)
> Sorry ignore Comment 3, I misinterpreted what Comment 0 meant. 

https://developer.mozilla.org/en-US/docs/Making_String_Changes

Once landed on mozilla-central, if you want to change a string you have to change its entity name.
Understood, thanks for the link!
Questions for Yuan and rstrong before I do this work to avoid going back and forth.  I'd like to do this before the next migration if there is work to be done:

- Do we need to update pre -win8 to change back to: "Automatically install updates (recommended: improved security)" from what it currently is: "Automatically install updates from desktop &brandShortName;"
- (rstrong only) Do we need to update the entity label? The link quote above says you don't need to unless you are changing the meaning, I think the meaning stays the same, even know there is a slight modification to it to not say recommended.
- Do we want the (recommended: improved security) note re-added in win8?  If so do we add it to the metro and desktop option?
Flags: needinfo?(robert.bugzilla)
> - (rstrong only) Do we need to update the entity label? The link quote above
> says you don't need to unless you are changing the meaning, I think the
> meaning stays the same, even know there is a slight modification to it to
> not say recommended.

I'm not rstrong, but the problem here is not the "general meaning", you introduced a variable there and changing the entity name is the only way to force localizers to notice that change.
(In reply to Brian R. Bondy [:bbondy] from comment #7)
> Questions for Yuan and rstrong before I do this work to avoid going back and
> forth.  I'd like to do this before the next migration if there is work to be
> done:
> 
> - Do we need to update pre -win8 to change back to: "Automatically install
> updates (recommended: improved security)" from what it currently is:
> "Automatically install updates from desktop &brandShortName;"
I think that makes sense but UX needs to make that call.

> - (rstrong only) Do we need to update the entity label? The link quote above
> says you don't need to unless you are changing the meaning, I think the
> meaning stays the same, even know there is a slight modification to it to
> not say recommended.
Since there are localizers that localize nightly please change the entity name to avoid confusion.

> - Do we want the (recommended: improved security) note re-added in win8?  If
> so do we add it to the metro and desktop option?
If it is in any of the messages I think that it should be in all of them but that is a UX call.
1. Do we need to update pre -win8 to change back to: "Automatically install updates (recommended: improved security)" from what it currently is:"Automatically install updates from desktop &brandShortName;"? 

For pre Windows 8 updates, that makes sense to me. And on Mac, it has the same message. 

2. Do we want the (recommended: improved security) note re-added in win8?  If so do we add it to the metro and desktop option?

I think we shouldn't add the note back in Windows 8. We have given "Not checking updates" a security risk flag. I think there is enough information to inform users about security risks. 

Adding to "Update desktop" and "Update metro and desktop" doesn't really help the users make a decision. And there is already a note about add-on compatibility. Adding one more is going to cause information overload...

Adding to either one of these options may create confusion. Users should make their decision based on the add-on compatibility not security risks. We should focus on showing the most important note, in order to help users make their choice.
Flags: needinfo?(ywang)
Attached patch Patch v1. (obsolete) — Splinter Review
I'd like to try to get this landed on m-c before the uplift if possible.
Assignee: nobody → netzen
Status: NEW → ASSIGNED
Attachment #764175 - Flags: review?(robert.bugzilla)
Blocks: metrov1it9, 833182, 866229
No longer blocks: metrov1triage
No longer depends on: 866229
Whiteboard: feature=defect c=Settings_pane_options_and_about u=metro_firefox_user p=1
Summary: Rename updateAuto.label to reflect string change → Defect - Rename updateAuto.label to reflect string change
Priority: -- → P1
Comment on attachment 764175 [details] [diff] [review]
Patch v1.

Discussed a couple of problems with bbondy on irc and he will be resubmitting
Attachment #764175 - Flags: review?(robert.bugzilla)
Attached patch Patch v2. (obsolete) — Splinter Review
- Had mistakenly added the dynamic string to the dtd instead of the properties file. 
- When I previously tested, I was mistakenly looking at the first string instead of the 2nd in metro builds (which was correct for in metro).
Attachment #764376 - Flags: review?(robert.bugzilla)
Attachment #764175 - Attachment is obsolete: true
Comment on attachment 764376 [details] [diff] [review]
Patch v2.

>diff --git a/browser/locales/en-US/chrome/browser/preferences/advanced.dtd b/browser/locales/en-US/chrome/browser/preferences/advanced.dtd
>--- a/browser/locales/en-US/chrome/browser/preferences/advanced.dtd
>+++ b/browser/locales/en-US/chrome/browser/preferences/advanced.dtd
>@@ -79,24 +79,25 @@
> <!ENTITY clearOfflineAppCacheNow.label   "Clear Now">
> <!ENTITY clearOfflineAppCacheNow.accesskey "N">
> <!ENTITY overrideSmartCacheSize.label    "Override automatic cache management">
> <!ENTITY overrideSmartCacheSize.accesskey "O">
> 
> <!ENTITY updateTab.label                 "Update">
> 
> <!ENTITY updateApp.label                 "&brandShortName; updates:">
>-<!ENTITY updateAuto.label                "Automatically install updates from desktop &brandShortName;">
>-<!ENTITY updateAuto.accesskey            "A">
> <!ENTITY updateAutoMetro.label           "Automatically update from desktop and Windows 8 style &brandShortName;">
> <!ENTITY updateAutoMetro.accesskey       "s">
> <!ENTITY updateCheck.label               "Check for updates, but let me choose whether to install them">
> <!ENTITY updateCheck.accesskey           "C">
> <!ENTITY updateManual.label              "Never check for updates (not recommended: security risk)">
> <!ENTITY updateManual.accesskey          "N">
>+<!-- Note either updateAutoNoMetro is used or (updateAutoMetro and updateAutoDesktop), so re-using accesss key in updateAutoNoMetro is OK -->
Mention that updateAutoDesktop is in preferences.properties
What about the accesskey for updateAutoDesktop?

>+<!ENTITY updateAutoNoMetro.label         "Automatically install updates (recommended: improved security)">
>+<!ENTITY updateAutoNoMetro.accesskey     "A">
Please position this where updateAuto was removed.
Please rename to updateAuto1 or updateAutoDefault

Removing r? until the accesskey question is answered
Attachment #764376 - Flags: review?(robert.bugzilla)
btw: I usually go with a separate label, keep all of the strings in the dtd, and just hide the label(s) as appropriate... it just seems cleaner code and string wise.
(In reply to Robert Strong [:rstrong] (do not email) from comment #15)
> btw: I usually go with a separate label, keep all of the strings in the dtd,
> and just hide the label(s) as appropriate... it just seems cleaner code and
> string wise.

Normally I do this for labels, and I did this for the previous bug, but in this case there is code that checks the radio button value that would have to be more complicated, so I think in this case it's better to use the properties file instead.
Attached patch Patch v3. (obsolete) — Splinter Review
Attachment #764376 - Attachment is obsolete: true
Attachment #764515 - Flags: review?(robert.bugzilla)
Comment on attachment 764515 [details] [diff] [review]
Patch v3.

(In reply to Robert Strong [:rstrong] (do not email) from comment #14)
<snip>
> >+<!ENTITY updateAutoNoMetro.label         "Automatically install updates (recommended: improved security)">
> >+<!ENTITY updateAutoNoMetro.accesskey     "A">
> Please position this where updateAuto was removed.
> Please rename to updateAuto1 or updateAutoDefault
You didn't rename updateAutoNoMetro

r=me with that
Attachment #764515 - Flags: review?(robert.bugzilla) → review+
Attached patch Patch v4Splinter Review
Implemented missed nit.  Carrying forward r+.
Attachment #764515 - Attachment is obsolete: true
Attachment #764532 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/83aa31ec53d9
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
updateAuto.label string was modified in bug 866229 - https://hg.mozilla.org/mozilla-central/rev/920669704476:
-<!ENTITY updateAuto.label                "Automatically install updates (recommended: improved security)">
+<!ENTITY updateAuto.label                "Automatically install updates from desktop &brandShortName;">
 <!ENTITY updateAuto.accesskey            "A">

Verified the string is updated in the latest Nightly:
Mozilla/5.0 (Windows NT 6.2; rv:25.0) Gecko/20130710 Firefox/25.0, build id: 20130710030205
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.