510707 - Add prefs ui for imap autosync date constraints

Status: RESOLVED FIXED

(Thunderbird :: Preferences, defect)

Description

[David :Bienvenu]

In bug 482476, I added backend support for a date constraint for imap auto sync - if the pref "mail.autosync.max_age_days" is set to a > 0 value, imap auto sync won't download messages older than the limit, and it will mark messages older than that as pending removal, and offline store compaction will remove them from the offline store. We need options ui for this, presumably in the network and disk space tab, for a global setting that limits the age of downloaded messages. 

If we were to make this per-server, the option would go in the account settings. I haven't coded it that way, though it would be fairly easy to switch. But I believe everyone agreed this should be a global option (though I'm slightly tempted to make it per-server so the UI can go with the existing autosync pref ui, which is per-server).

But I'll let you and Bryan figure that out...

Comment 1

[Bryan Clark (DevTools PM) [:clarkbw]]

from bug 482476 Comment #1

we started on a ui for this, I think we just need to get some real widget mockups generated to get a feel for it.  Also a UI like this will have some real i18n issues so some rearrangement might be in order.

Keep the last [ 3 ] ( months | v ) of email and use less than [ 3 ] ( gigabytes
| v ) of disk space.

Comment 2

[David :Bienvenu]

We're not doing the disk space constraints for 3.0, just the age limits.

Comment 3

[Bryan Clark (DevTools PM) [:clarkbw]]

ah right, so we'll just start with the first half of that UI.

Comment 4

[Bryan Clark (DevTools PM) [:clarkbw]]

Attachment: domi made pref screenshot

I just used the DOM inspector to get the right set of widgets inserted and it doesn't feel correct, at least wording wise.

One alternate is:

  Only keep the email from the last [ 3 ] ( Month(s) | v )

However that doesn't give us an easy way to turn it on or off.  We could use some kind of checkbox to turn on the limiter.

  [x] Only keep messages within the last:
      [ 3 ] ( Month(s) | v )

I'll keep trying a couple different concepts but overall it feels strange being in the same groupbox as the disk cache whose options effect a completely different system.

Comment 5

[Blake Winton (:bwinton) (:☕️)]

Attachment: UI idea for the option.

What about having the dropdowns be "Nothing/Day/Month/Year", and if it's nothing, then we hide or disable the textbox?

I've also thrown it into its own group.  Let me know what you think.

Thanks,
Blake.

Comment 6

[Blake Winton (:bwinton) (:☕️)]

Attachment: A second idea.

Comment 7

[Bryan Clark (DevTools PM) [:clarkbw]]

Comment on attachment 396599 [details]
A second idea.

nice!

Comment 8

[Blake Winton (:bwinton) (:☕️)]

Attachment: A patch for the previous screenshot.

Okay, give this a try, and let me know what I need to make work differently.  :)

Thanks,
Blake.

Comment 9

[Bryan Clark (DevTools PM) [:clarkbw]]

Comment on attachment 396640 [details] [diff] [review]
A patch for the previous screenshot.

Looks good overall.

I'm not sure about the hidden=true of the the number entry because it moves around the menu list.  However when I tried the alternative disabled=true it doesn't appear obvious enough; at least on the mac.

I tried collapse=true but that still collapses the display.

Maybe we can do a .style="visibility:hidden" so it just removes from the view but doesn't actually remove the space.

Comment 10

[Blake Winton (:bwinton) (:☕️)]

Attachment: The previous patch, with clarkbw's suggestions, and more.

(In reply to comment #9)
> Maybe we can do a .style="visibility:hidden" so it just removes from the view
> but doesn't actually remove the space.

Done.  I kind of like it, but let me know what you think.

And while I'm here, let me explain the empty useAutosyncBefore.label and useAutosyncMiddle.label.  I added them to give the localizers some place to put extra text that might make the sentence flow a little better for them.  It's probably not the greatest solution, but it seemed a little nicer, at least.  If there's a different and better way to help that line get translated, I'll be happy to change it.

Thanks,
Blake.

Comment 11

[Magnus Melin [:mkmelin]]

Comment on attachment 396731 [details] [diff] [review]
The previous patch, with clarkbw's suggestions, and more.


>+    if (SearchIntegration) {
>       if (SearchIntegration.osVersionTooLow)
>         hideSearchUI = true;
>       else if (SearchIntegration.osComponentsNotRunning)
>         disableSearchUI = true;
>     }
>     else
>-    {
>       hideSearchUI = true;
>-    }

"If one if else branch has braces, they all (on the same level) should." So don't remove this.

>+    if (autosyncPref.value == 0) {
>+      // Special-case 0 to have an interval of "All" and a hidden value of 0.
>+      autosyncInterval.value = 0;
>+      autosyncValue.value = 0;
>+      autosyncValue.setAttribute("class", "hidden");

I don't think it's a good idea to call this "hidden". To much confusion with just setting foo.hidden =. Would need a more distinct name.

However, it looks and feels quite weird when the element disappears. If disabled didn't look normally distinct enough on mac, maybe also style it to look enough distinct?

>+    autosyncValue.setAttribute("class", "");

removeAttribute

>+<!ENTITY yearInterval.label              "Years of">
>+<!ENTITY useAutosyncAfter.label          "email will be kept">

I think this needs more explanation. "email will be kept for offline use" or something. 

>+<!ENTITY useAutosyncAfter.accesskey      "k">

The k as assesskey here is a bit odd, not sure how cases like this should be handled...

The bigger problem here though is this can't go into the main preferences, as it's an imap only pref.

Comment 12

[Bryan Clark (DevTools PM) [:clarkbw]]

(In reply to comment #11)
> However, it looks and feels quite weird when the element disappears. If
> disabled didn't look normally distinct enough on mac, maybe also style it to
> look enough distinct?

Ok, good point that sounds like a good path to take.

> >+    autosyncValue.setAttribute("class", "");
> 
> removeAttribute
> 
> >+<!ENTITY yearInterval.label              "Years of">
> >+<!ENTITY useAutosyncAfter.label          "email will be kept">
> 
> I think this needs more explanation. "email will be kept for offline use" or
> something. 

I was thinking this morning that we could say "email will be stored on disk"

> >+<!ENTITY useAutosyncAfter.accesskey      "k">
> 
> The k as assesskey here is a bit odd, not sure how cases like this should be
> handled...
> 
> The bigger problem here though is this can't go into the main preferences, as
> it's an imap only pref.

Yes but the account manager will make us use this per-account and when it's actually a global imap setting.  We don't really have a space for it.  Perhaps we should just label it "IMAP Sync..."

Comment 13

[Magnus Melin [:mkmelin]]

Yeah, we'd need to make the pref per-account, but per comment 0 shouldn't be too much work. (Could be pretty useful per account in general also.)

Comment 14

[Bryan Clark (DevTools PM) [:clarkbw]]

Comment on attachment 396731 [details] [diff] [review]
The previous patch, with clarkbw's suggestions, and more.

ack, review comments in comment #12

full string for group box was: "IMAP Message Synchronization"

Comment 15

[Bryan Clark (DevTools PM) [:clarkbw]]

(In reply to comment #13)
> Yeah, we'd need to make the pref per-account, but per comment 0 shouldn't be
> too much work. (Could be pretty useful per account in general also.)

I agree this change would fit into the Thunderbird landscape better.  At least we got the pref widgets worked out. :)

Comment 16

[Magnus Melin [:mkmelin]]

And if we do it now, at least we won't have the pref migration headaches later.

Comment 17

[Blake Winton (:bwinton) (:☕️)]

Attachment: The option in the account preferences.

Okay, here's the option in the account preferences.  I haven't moved over the javascript yet, but I hope to have that done and a new patch posted tonight.

Comment 18

[David :Bienvenu]

If we do that, then don't we need to implement per-server retention and download policies? I'm happy to do that, if we decide that's what we want, but what's currently implemented is global.

Comment 19

[Blake Winton (:bwinton) (:☕️)]

Attachment: The previous patch, with the javascript moved. (Depends on patch 397095 for bug 482476.)

The thing I'm the least sure about is the code described in the following comment:
// Special-case values <= 0 to have an interval of "All" and a disabled
// value of the positive version of the preference, so we don't lose
// the last value the user entered.

Since the back-end pref doesn't use any values <= 0, I thought that could be a place to store that info, but it does seem a little hackish, and will break if we change the interpretation of negative values in that preference.

(I attempted to do a little cleanup on files that I was editing.  If I messed up any of it, or if you'ld rather I didn't do it in this patch, please let me know.)

Thanks,
Blake.

Comment 20

[Blake Winton (:bwinton) (:☕️)]

Attachment: The previous patch, with the javascript moved. (Depends on patch 397095 for bug 482476.)

Uh, let's try that with the updated css files for Windows and Linux.

Comment 21

[Magnus Melin [:mkmelin]]

Comment on attachment 397172 [details] [diff] [review]
 The previous patch, with the javascript moved. (Depends on patch 397095 for bug 482476.) 

Re non-whitespace cleanup, in general you shouldn't. Though personally i could let this time pass ;)


>+<!ENTITY useAutosyncAfter.label          "email will be stored on disk">
>+<!ENTITY useAutosyncAfter.accesskey      "w">

I suspect the accesskey shouldn't be set here, but like before, not quite sure.
Also re the "hidden" class, it should have a different name - especially now that it isn't hidden.

Also note you'll have to fix seamonkey dtds too, this is shared code.

I think the widget looks good now, but perhaps it should be the first item under Disk Space? Then it would read first what to keep then what not to keep.

>+  let autosyncPref = document.getElementById("imap.autoSyncMaxAgeDays");
>+  if (autosyncPref.value == "")
>+    autosyncPref.value = "-1";
>+  let autosyncPrefValue = parseInt(autosyncPref.value, 10);

Make this prettier, 
 let autosyncPrefValue = (autosyncPref.value == "") ? -1 : parseInt(autosyncPref.value, 10);

or something.

>+    <!-- IMAP Autosync Preference -->
>+    <hbox align="center" hidefor="movemail,pop3,nntp,none,rss">
>+        <label id="useAutosyncBefore">&useAutosyncBefore.label;</label>
>+        <textbox id="autosyncValue" type="number" size="3" min="1"
>+                 max="100" class="autosync"

>+  if (autosyncPref.value == "") {
>+    return;
>+  }

Unnecessary braces.

The max should be whatever max value we really can have here.

Initial value of the widget is now 1, i'd suggest it's something else. Not sure what, but perhaps a default of say 30 or 14 days would be quite normal?

Also note this is now mailnews/ code, so you'll have to have someone else review most of it.

Comment 22

[David :Bienvenu]

Now that the per-server pref is implemented (in a patch, not checked in yet, in bug 482476), this code should be get/setting the imap per-server pref, e.g., mail.server.serverXX.autosync_max_age_days - we have some examples of account settings code setting imap server prefs, e.g., the imap delete model stuff. See, for example,

    <radiogroup wsm_persist="true" id="imap.deleteModel"
                oncommand="selectImapDeleteModel(this.value)"
                prefstring="mail.server.%serverkey%.delete_model">

Comment 23

[Blake Winton (:bwinton) (:☕️)]

(In reply to comment #21)
> Also re the "hidden" class, it should have a different name - especially now
> that it isn't hidden.

Uh, I thought that I got rid of it entirely.

https://bugzilla.mozilla.org/attachment.cgi?id=397172&action=diff#a/mail/themes/gnomestripe/mail/preferences/preferences.css_sec1 says

.autosync[disabled="true"] > .textbox-input-box {
  color: GrayText;
}


(In reply to comment #22)
> Now that the per-server pref is implemented (in a patch, not checked in yet,
> in bug 482476), this code should be get/setting the imap per-server pref,
> e.g., mail.server.serverXX.autosync_max_age_days

Uh, I thought I did that already.

https://bugzilla.mozilla.org/attachment.cgi?id=397172&action=diff#a/mailnews/base/prefs/content/am-offline.xul_sec1 says

    <label id="imap.autoSyncMaxAgeDays" hidden="true"
           wsm_persist="true" preftype="int"
           prefstring="mail.server.%serverkey%.autosync_max_age_days"/>

So now I'm starting to think that I'm confused, or that I've messed something up somewhere.  Am I seeing a different patch than everyone else?  Did my too-quick-revision of the patch (to fix the css) cause bugmail to send email too close together?  Is it just one of those freak occurrences that both of you are commenting on things that I had already fixed?


(As for the rest of Magnus' comments, I'll totally fix those, and in the future, won't clean up the code, other than whitespace, unless asked.)

Thanks,
Blake.

Comment 24

[David :Bienvenu]

Comment on attachment 397172 [details] [diff] [review]
 The previous patch, with the javascript moved. (Depends on patch 397095 for bug 482476.) 

my bad, I missed that part of the patch...

Comment 25

[Blake Winton (:bwinton) (:☕️)]

(In reply to comment #24)
> my bad, I missed that part of the patch...

*Phew*.  Good to know that I'm not going crazy.  :)

(In reply to comment #21)
> Re non-whitespace cleanup, in general you shouldn't. Though personally i could
> let this time pass ;)

I've reverted the files I didn't make any real changes to, cause it was easy to do so, and should make the diff clearer.

> >+<!ENTITY useAutosyncAfter.label          "email will be stored on disk">
> >+<!ENTITY useAutosyncAfter.accesskey      "w">
> I suspect the accesskey shouldn't be set here, but like before, not quite
> sure.

I asked on #l10n (at 8:30pm EST on a Sunday night), and got no response ;) , so I'll remove it until I hear otherwise.

> Also note you'll have to fix seamonkey dtds too, this is shared code.

Fixed.

> I think the widget looks good now, but perhaps it should be the first item
> under Disk Space? Then it would read first what to keep then what not to keep.

Fixed.

> Make this prettier, 
>  let autosyncPrefValue = (autosyncPref.value == "") ? -1 :
> parseInt(autosyncPref.value, 10);
> or something.

Changed.

> >+    <!-- IMAP Autosync Preference -->
> >+    <hbox align="center" hidefor="movemail,pop3,nntp,none,rss">
> >+        <label id="useAutosyncBefore">&useAutosyncBefore.label;</label>
> >+        <textbox id="autosyncValue" type="number" size="3" min="1"
> >+                 max="100" class="autosync"

I'm guessing you meant to point out the indentation here.  I've gone ahead and fixed that.

> >+  if (autosyncPref.value == "") {
> >+    return;
> >+  }
> Unnecessary braces.

Fixed.

> The max should be whatever max value we really can have here.

Hmm...  Bienvenu, what is the max value of that property?

> Initial value of the widget is now 1, i'd suggest it's something else. Not
> sure what, but perhaps a default of say 30 or 14 days would be quite normal?

So, since I'm using the negative of the textfield to store the value when the All option is selected, it seems like it would be easiest (not most correct, just easiest) to change this default in the backend code.

I believe it would need to be changed here:
https://bugzilla.mozilla.org/attachment.cgi?id=397095&action=diff#a/mailnews/mailnews.js_sec1

Bienvenu, does that sound reasonable, or is this a good reason to not special-case values <= 0 (as mentioned in comment #19)?

> Also note this is now mailnews/ code, so you'll have to have someone else
> review most of it.

Yup, I'll do that.  Any suggestions?

(I've got another patch with the above stuff fixed, but I'm going to hold off on posting it until I hear back about the accesskey and max/default value issues, or tomorrow afternoon at the latest.)

Thanks,
Blake.

Comment 26

[Magnus Melin [:mkmelin]]

(In reply to comment #23)
> (In reply to comment #21)
> > Also re the "hidden" class, it should have a different name - especially now
> > that it isn't hidden.
> 
> Uh, I thought that I got rid of it entirely.

Seems you did, sorry about that, I must have misread somehow.

Comment 27

[David :Bienvenu]

> Hmm...  Bienvenu, what is the max value of that property?

The max value is the number of days we can represent in a signed 32 bit integer with seconds as the unit. 

The code uses <= 0 as no limit, so the number of days would be something like 0x7FFFFFF / (60*60*24) which is > 23000 days

Comment 28

[David :Bienvenu]

I could change the code to treat 0 as no limit, and then treat the value as an unsigned 32 bit number...

Comment 29

[Blake Winton (:bwinton) (:☕️)]

Attachment: The previous patch, with max value set, and without storing the previous value. (Depends on patch 397095 for bug 482476.)

(In reply to comment #27)
> > > The max should be whatever max value we really can have here.
> > Hmm...  Bienvenu, what is the max value of that property?
> The max value is the number of days we can represent in a signed 32 bit
> integer with seconds as the unit. 
> 
> The code uses <= 0 as no limit, so the number of days would be something like
> 0x7FFFFFFF / (60*60*24) which is > 23000 days

Hmmm...
So, if they choose days, the max would be 24855,
but if they pick weeks, it would be 3550,
and if they pick months, it would be 801,
and if they pick years, it would be 68,
so I made it dependant on which interval they chose.

And I'm no-longer using the negative of the value to store the previously selected user value if they choose the "All" interval.  This means that, while the value will persist no matter what interval they choose while they're on that page, if they leave the interval on "All", and navigate away and then come back, it'll reset the value to 31.

Thanks,
Blake.

Comment 30

[Bryan Clark (DevTools PM) [:clarkbw]]

Comment on attachment 397675 [details] [diff] [review]
The previous patch, with max value set, and without storing the previous value. (Depends on patch 397095 for bug 482476.)

this looks good to me.  Though it seems like it could use a little spacing between the next line - "To save disk space, do not download for offline use:" - and itself.

Comment 31

[Mark Banner (:standard8)]

Comment on attachment 397675 [details] [diff] [review]
The previous patch, with max value set, and without storing the previous value. (Depends on patch 397095 for bug 482476.)

>+<!-- LOCALIZATION NOTE:
>+  The entities useAutosyncBefore.label, useAutosyncMiddle.label, and
>+  useAutosyncAfter.label appear on a single line in preferences as follows:

I think this should be

<!-- LOCALIZATION NOTE: (useAutosyncBefore.label, useAutosyncMiddle.label, useAutosyncAfter.label):

>+.autosync[disabled="true"] > .textbox-input-box {
>+  color: GrayText;
>+}

This isn't working on Mac at least.

>+    // Otherwise, get the list of possible intervals, in order from
>+    // largest to smallest.
>+    let valuesToTest = [];
>+    for (let i = autosyncInterval.itemCount-1; i >= 0; i--)

nit: space either side of '-'

> function onPreInit(account, accountValues)
> {
>+  gServerType = getAccountValue(account, accountValues, "server", "type", null, false);
>+  hideShowControls(gServerType);
>+  gIncomingServer= account.incomingServer;

nit: add space before = please.

>+  let autosyncInterval = document.getElementById("autosyncInterval");
>+  let autosyncValue = document.getElementById("autosyncValue");
>+  let autosyncPref = document.getElementById("imap.autoSyncMaxAgeDays");
>+  if (autosyncPref.value == "")
>+    return;

Is this the error case you're trying to catch? i.e. a wrong server type? If so, we should at least add a warning in if not actually dumping/throwing maybe.

>+      <textbox id="autosyncValue" type="number" size="3" min="1"
>+               class="autosync" onchange="onAutosyncChange();"
>+               aria-labelledby="useAutosyncBefore autosyncValue useAutosyncMiddle autosyncInterval useAutosyncAfter"/>

According to devmo size is the number of characters - so it should be 5. However on Mac it seems to give a width of 7. Where you intentionally compensating for that?

Comment 32

[Blake Winton (:bwinton) (:☕️)]

(In reply to comment #31)
> >+.autosync[disabled="true"] > .textbox-input-box {
> >+  color: GrayText;
> >+}
> This isn't working on Mac at least.

That's odd.  I do the majority of my testing on Mac.

> >+  let autosyncInterval = document.getElementById("autosyncInterval");
> >+  let autosyncValue = document.getElementById("autosyncValue");
> >+  let autosyncPref = document.getElementById("imap.autoSyncMaxAgeDays");
> >+  if (autosyncPref.value == "")
> >+    return;
> 
> Is this the error case you're trying to catch? i.e. a wrong server type? If
> so, we should at least add a warning in if not actually dumping/throwing
> maybe.

No, it's not actually an error case.  The problem I ran into was that the onAutosyncChange method was being called before (or in the middle of) the initDownloadSettings method, so I was re-setting the autosyncPref.value to a bogus value.

So I'm basically using autosyncPref.value as a hint as to whether I'm done initializing or not, as I hoped to show in the comment on line 114
  // Clear the preference until we're done initializing.
  autosyncPref.value = "";

So, I could go one of two ways here.  Either I could document what I'm doing on line 192, or I could add another attribute to something indicating whether or not I'm done initializing.  (autosyncPref.setAttribute("initialized", "true")?)

Do you have a preference?

> >+      <textbox id="autosyncValue" type="number" size="3" min="1"
> >+               class="autosync" onchange="onAutosyncChange();"
> >+               aria-labelledby="useAutosyncBefore autosyncValue useAutosyncMiddle autosyncInterval useAutosyncAfter"/>
> 
> According to devmo size is the number of characters - so it should be 5.
> However on Mac it seems to give a width of 7. Where you intentionally
> compensating for that?

I don't believe so.  Could I get rid of size entirely, or should I set it to 5?
(It was set to three because I had the max set to 100, and forgot to update it when I changed the max.)

Other than that, good catches, and I'll fix those post haste.

Thanks,
Blake.

Comment 33

[Blake Winton (:bwinton) (:☕️)]

Attachment: The previous patch, with Standard8's suggestions.

(In reply to comment #31)
> I think this should be
> <!-- LOCALIZATION NOTE: (useAutosyncBefore.label, useAutosyncMiddle.label,
> useAutosyncAfter.label):

Fixed.  (In both places.)

> >+.autosync[disabled="true"] > .textbox-input-box {
> >+  color: GrayText;
> >+}
> This isn't working on Mac at least.

Yeah, and it's unnecessary on the other platforms.  Removed.

> >+    for (let i = autosyncInterval.itemCount-1; i >= 0; i--)
> nit: space either side of '-'

Fixed.

> > function onPreInit(account, accountValues)
> > {
> >+  gServerType = getAccountValue(account, accountValues, "server", "type", null, false);
> >+  hideShowControls(gServerType);
> >+  gIncomingServer= account.incomingServer;
> nit: add space before = please.

Fixed.

> >+  if (autosyncPref.value == "")
> >+    return;
> Is this the error case you're trying to catch? i.e. a wrong server type? If
> so, we should at least add a warning in if not actually dumping/throwing
> maybe.

Added a comment explaining my intentions.

> >+      <textbox id="autosyncValue" type="number" size="3" min="1"
> According to devmo size is the number of characters - so it should be 5.

Changed to "5".

Thanks,
Blake.

Comment 34

[Mark Banner (:standard8)]

Comment on attachment 398847 [details] [diff] [review]
The previous patch, with Standard8's suggestions.

Thanks for the update, r=Standard8. Requesting sr as you'll need that as well.

Comment 35

[David :Bienvenu]

Comment on attachment 398847 [details] [diff] [review]
The previous patch, with Standard8's suggestions.

it's a little odd that if you have to specify a number of weeks that turns out to have an equivalent number of months, that the picker switches to months. But it's not critical, and we should get this in for before the string freeze.

Comment 36

[Mark Banner (:standard8)]

As this affects strings and is a blocker, I've checked it in (all comments were previously addressed):

http://hg.mozilla.org/comm-central/rev/52d3e8783d64

Comment 37

[Blake Winton (:bwinton) (:☕️)]

(In reply to comment #35)
> (From update of attachment 398847 [details] [diff] [review])
> it's a little odd that if you have to specify a number of weeks that turns out
> to have an equivalent number of months, that the picker switches to months. But
> it's not critical, and we should get this in for before the string freeze.

Yeah, that seemed like the best option I had without adding another preference just to store the last interval the user picked.  (If you want to open a new bug to add the preference and save the last interval, please assign it to me, and I'll get it in for rc1.)

Thanks,
Blake.

Comment 38

[Bryan Clark (DevTools PM) [:clarkbw]]

(In reply to comment #37)
> (In reply to comment #35)
> > (From update of attachment 398847 [details] [diff] [review] [details])
> > it's a little odd that if you have to specify a number of weeks that turns out
> > to have an equivalent number of months, that the picker switches to months. But
> > it's not critical, and we should get this in for before the string freeze.
> 
> Yeah, that seemed like the best option I had without adding another preference
> just to store the last interval the user picked.  (If you want to open a new
> bug to add the preference and save the last interval, please assign it to me,
> and I'll get it in for rc1.)

It would be nice but I wouldn't block the TB3 release to make this happen.  Perhaps for 3.next we could fix that issue.

Comment 39

[Kadir Topal [:atopal]]

Natural sentences constructed with drop down menus are a localizers nightmare, but having two dropdown menus in one sentence will be just too much to handle for most languages, and adding before- middle- and after-strings won't help much as they dont vary with the content in the drop down menus. 

And isn't it confusing in English as well? Shouldn't it read: "Last 7 weeks of email" instead of just "7 weeks of email"?

Anyway, I don't have a good solutution for this problem, but I hope it will be revised before the final release.

Comment 40

[Blake Winton (:bwinton) (:☕️)]

Yeah, I think I said something similar in comment #10.  Well, maybe a little more optimistically.  Or naively.  ;)

The text reads a little better in English:
"4 weeks of email will be stored on disk."

More to the point, I would really love to make it more localizable, but I don't have enough experience to know what would both work well and look good for all languages, and so I'm going to need some guidance and suggestions from those of us who do.

Thanks,
Blake.

Comment 41

[rsx11m]

I've filed bug 515842 yesterday as I figured that the labels could be improved. While the patch there focuses on adding a descriptive label above that options to make it clearer what they are doing, this bug too received a comment stating possible localization issues.

On the other hand, it may not be as bad as it seems. The fact that you have multiple useAutosync*.label available makes it easier to customize the string. Please see bug 515842 comment #7 for variations which should be possible to achieve without modifying Blake's implemented structure.

Also, a classical "Label: [ ] [ ]" structure would be possible as fallback.

Comment 42

[Kadir Topal [:atopal]]

Even with the classical structure you have a problem. I'd like to say: 
E-Mails are saved for the last: [7] [weeks]

But then you have "all" together with "weeks" in that drop down and it would read:
E-Mails are saved for the last: [x] [all]

I'll add further comments to bug 515842