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"

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?

Seamonkey has the same string. Ian, should I update it there too?

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 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.

(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.

But first wait for bwinton's word on the wording.

Comment on attachment 631676 [details] [diff] [review] patch Yeah, sure, I'll say ui-r=me for this wording.

Comment on attachment 631676 [details] [diff] [review] patch Wrong patch? no SeaMonkey parts?

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 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.

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 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 on attachment 632027 [details] [diff] [review] patch v3 Whoops, forgot to set the r- flag.

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.

Is this enough?

Comment on attachment 635431 [details] [diff] [review] patch v4 Review of attachment 635431 [details] [diff] [review]: ----------------------------------------------------------------- I'm good with this. Thanks, aceman.