Last Comment Bug 758803 - Enhance wording of compacting option
: Enhance wording of compacting option
Status: RESOLVED FIXED
: polish
Product: Thunderbird
Classification: Client Software
Component: Preferences (show other bugs)
: Trunk
: All All
: -- minor with 1 vote (vote)
: Thunderbird 16.0
Assigned To: :aceman
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-05-25 14:42 PDT by Ulf Zibis
Modified: 2012-07-07 07:29 PDT (History)
5 users (show)
ryanvm: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
patch (7.51 KB, patch)
2012-06-09 10:55 PDT, :aceman
iann_bugzilla: review-
bwinton: ui‑review+
iann_bugzilla: feedback+
Details | Diff | Splinter Review
patch v2 (9.39 KB, patch)
2012-06-11 13:20 PDT, :aceman
iann_bugzilla: review+
Details | Diff | Splinter Review
patch v3 (9.49 KB, patch)
2012-06-11 14:38 PDT, :aceman
mconley: review-
Details | Diff | Splinter Review
patch v4 (10.28 KB, patch)
2012-06-21 13:11 PDT, :aceman
mconley: review+
Details | Diff | Splinter Review

Description Ulf Zibis 2012-05-25 14:42:01 PDT
User Agent: Mozilla/5.0 (Windows NT 5.1; rv:12.0) Gecko/20100101 Firefox/12.0
Build ID: 20120420145725

Steps to reproduce:

I worried about the wording in the options settings pane.
At least the german translation does not clearly describe the fact, that the threshold is meant as sum over all folders, because in german the plural form for 'folder' is the same than the singular form. 


Actual results:

The german translation in options of:
"Compact folder, if that saves more space than x MB"
IMO is ambiguous. It's not clear, if
... that is meant as sum over all folders rather than space per folder.
... only that big folders are compacted at all.



Expected results:

If you would add the words 'all' and 'in sum', also the german (and maybe others too) translation would be unambiguously clear:
"Compact all folders, if that saves more space in sum than x MB"
Comment 1 :aceman 2012-05-30 06:51:59 PDT
If the English text is in plural, the translator should know how to properly express that in his language. But maybe he didn't understand what it means.
I would agree with making the text more precise also in English, because it can be understood in different ways, as you say.
Bwinton?
Comment 2 :aceman 2012-06-09 07:37:08 PDT
Seamonkey has the same string. Ian, should I update it there too?
Comment 3 :aceman 2012-06-09 10:55:35 PDT
Created attachment 631676 [details] [diff] [review]
patch

Bwinton: this is the wording proposal. And it also makes the textbox disabled when compacting is disabled. Other fields in the prefs do it too.

Ian: this is the wording proposal. Would you like it in Seamonkey?
Comment 4 Ian Neal 2012-06-09 15:13:14 PDT
Comment on attachment 631676 [details] [diff] [review]
patch

>+++ b/mail/components/preferences/advanced.xul
>@@ -74,18 +74,21 @@
>                   name="mailnews.mark_message_read.delay" type="bool"
>                   onchange="gAdvancedPane.updateMarkAsReadTextbox(this.value);"/>
>       <preference id="mailnews.mark_message_read.delay.interval"
>                   name="mailnews.mark_message_read.delay.interval" type="int"/>
>       <preference id="mail.openMessageBehavior" name="mail.openMessageBehavior" type="int"/>
>       <preference id="mail.close_message_window.on_delete" 
>                   name="mail.close_message_window.on_delete" type="bool"/>
>       <!-- Network tab -->
>+      <preference id="mail.prompt_purge_threshhold"
>+                  name="mail.prompt_purge_threshhold"
>+                  type="bool"/>
The pattern in this file seems to have the type on the same line as the name.
>+      <preference id="mail.purge_threshhold_mb"
>+                  name="mail.purge_threshhold_mb" type="int"/>

>           <groupbox>
>-            <caption label="&Diskspace;"/>
>+            <caption label="&diskspace;"/>
Do you really need to change the entity?

>+                <checkbox id="offlineCompactFolder"
>+                          label="&offlineCompactFolders.label;"
>+                          accesskey="&offlineCompactFolders.accesskey;"
>+                          oncommand="gAdvancedPane.updateCompactOptions(this.checked);"
You could add an onchange to the preference instead (SM does, but not sure if there is an advantage).
>                           preference="mail.prompt_purge_threshhold"/>
Does the checkbox need an aria-labelledby? (SM does, though one entry is incorrect for SM on this and the textbox)

Yes, SM would benefit from this clarity too.
Comment 5 :aceman 2012-06-09 15:29:03 PDT
(In reply to Ian Neal from comment #4)
> >           <groupbox>
> >-            <caption label="&Diskspace;"/>
> >+            <caption label="&diskspace;"/>
> Do you really need to change the entity?
No, I just thought to clean up the anomalous uppercase. I can drop this.

> >+                <checkbox id="offlineCompactFolder"
> >+                          label="&offlineCompactFolders.label;"
> >+                          accesskey="&offlineCompactFolders.accesskey;"
> >+                          oncommand="gAdvancedPane.updateCompactOptions(this.checked);"
> You could add an onchange to the preference instead (SM does, but not sure
> if there is an advantage).
This file uses both approaches. I tried the <preference onchange=> but it didn't know this.checked. I would have to play with this.value. Depends on what values it has. I'll try.

> >                           preference="mail.prompt_purge_threshhold"/>
> Does the checkbox need an aria-labelledby? (SM does, though one entry is
> incorrect for SM on this and the textbox)
No idea but I can try to copy/fix the SM definition.

> Yes, SM would benefit from this clarity too.
OK, I'll try to apply the string to SM too.
Comment 6 :aceman 2012-06-09 15:30:10 PDT
But first wait for bwinton's word on the wording.
Comment 7 Blake Winton (:bwinton) (:☕️) 2012-06-11 12:59:00 PDT
Comment on attachment 631676 [details] [diff] [review]
patch

Yeah, sure, I'll say ui-r=me for this wording.
Comment 8 Ian Neal 2012-06-11 13:15:00 PDT
Comment on attachment 631676 [details] [diff] [review]
patch

Wrong patch? no SeaMonkey parts?
Comment 9 :aceman 2012-06-11 13:20:35 PDT
Created attachment 632003 [details] [diff] [review]
patch v2

Sorry. That was just the original version of the patch that waited on bwinton. I should not r? you on that. Here is the new version with SM parts added and your nits.
Comment 10 Ian Neal 2012-06-11 14:02:21 PDT
Comment on attachment 632003 [details] [diff] [review]
patch v2

>+++ b/suite/mailnews/prefs/pref-offline.xul
>         <textbox id="offlineCompactFolderMin"
>                  type="number"
>                  size="5"
This needs changing to size="4" (not sure why it is 5 to start with) for the new text to fit within the page.
>                  min="1"
>                  max="2048"
>                  increment="10"
>                  value="20"
>                  preference="mail.purge_threshhold_mb"
>-                 aria-labelledby="offlineCompactFolder offlineCompactFolderMin kbLabel"/>

r=me with that fixed.
Comment 11 :aceman 2012-06-11 14:38:02 PDT
Created attachment 632027 [details] [diff] [review]
patch v3

Thanks, reduced the size in TB too. It still shows space for 5 digits, but that seems to be a general problem with a textbox.
Comment 12 Mike Conley (:mconley) - (Needinfo me!) 2012-06-18 07:53:23 PDT
Comment on attachment 632027 [details] [diff] [review]
patch v3

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

This mostly looks good - just one point.

::: mail/components/preferences/advanced.xul
@@ +315,5 @@
>              <hbox align="center">
> +                <checkbox id="offlineCompactFolder"
> +                          label="&offlineCompactFolders.label;"
> +                          accesskey="&offlineCompactFolders.accesskey;"
> +                          oncommand="gAdvancedPane.updateCompactOptions(this.checked);"

How hard would it be to use the command pattern here, instead of oncommand? The less we introduce usage of oncommand, the better.
Comment 13 Mike Conley (:mconley) - (Needinfo me!) 2012-06-21 12:03:06 PDT
Comment on attachment 632027 [details] [diff] [review]
patch v3

Whoops, forgot to set the r- flag.
Comment 14 :aceman 2012-06-21 12:55:17 PDT
There is no other usage of command in the file (maybe whole preferences dialog). So it would be hard for me to start the usage and have nothing to copy :) But let's try it.
Comment 15 :aceman 2012-06-21 13:11:16 PDT
Created attachment 635431 [details] [diff] [review]
patch v4

Is this enough?
Comment 16 Mike Conley (:mconley) - (Needinfo me!) 2012-07-05 12:52:14 PDT
Comment on attachment 635431 [details] [diff] [review]
patch v4

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

I'm good with this. Thanks, aceman.
Comment 17 Ryan VanderMeulen [:RyanVM] 2012-07-07 07:28:52 PDT
https://hg.mozilla.org/comm-central/rev/54bf0eb08ceb

Note You need to log in before you can comment on or make changes to this bug.