515842 - Make labels for IMAP autosync policies more descriptive to avoid mixup with retention settings

Status: RESOLVED FIXED

(MailNews Core :: Account Manager, defect)

Description

[rsx11m]

Labels again. :-)

Quite some effort was spent in bug 410597 to make the retention-settings labels as unambiguous as possible. In contrast, the label "[All] email will be stored on disk" followed by the "To save disk space, ..." label introduced in bug 510707 is not very clear and can be improved. The suggestion is to add a label on top of the new UI, pulling the "To save disk space" upfront, and to provide some more specifics that this only affects the local offline copies.

Comment 1

[rsx11m]

Attachment: Proposed patch

This patch adds a "local copies can be restricted by age or size" on top and makes the "Do not download" label more specific that messages are kept on the server. This also splits the doNotDownload.label into IMAP and NNTP versions.

In addition, it establishes an access key (the control was specified but not the key in the original patch). I've associated it with the menu control rather than the value, as the latter is disabled when "All" is selected.

I'm flagging for ui-review only at this time. Unless you want this for beta 4 already (if that's still possible anyway), I'll request r?/sr? after the code freeze goes into effect.

Comment 2

[rsx11m]

Attachment: Screenshot of proposed changes

Current labels on the left, proposed ones on the right
(this was taken before the accesskey was added).

Comment 3

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

thanks for picking this up!

Comment 4

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

Comment on attachment 399926 [details] [diff] [review]
Proposed patch

I like the first part.  I'm only wondering if we can sharpen up this second part.

"Do not download and keep on server only:"

Any other suggestions?

I was thinking something like this:

"Messages will not be downloaded for offline use when:"

Comment 5

[Vlado Valastiak [:wladow]]

Guys, I think this UI is ugly and hard to localize properly. Adding a descriptive note is fine and really needed, but I'd suggest to separate autosync interval setting 'All' from the dropdown and to use a radio button for it instead.

Like this (trying ASCII art):

------------------------------------------------
Emails stored on disk:
     (o) all     ( ) not older than [ ] [  ↓]

(wording is up to you)

Another concern: do we really need so many time interval settings? I mean why not get rid of the dropdown and use the days interval only. This would match our other time interval settings used thorough our UI as well as history setting in Firefox. Moreover it will be easier for localizers to localize it properly.

Comment 6

[rsx11m]

(In reply to comment #4)
> "Do not download and keep on server only:"
> "Messages will not be downloaded for offline use when:"

I was under the impression from bug 410597 (and others) that you wanted to avoid "offline" where IMAP synchronization is meant, to avoid conflict with the "offline" for going offline, and we explicitly called the server as such for the retention period. I figured the "keep on server" is important to properly distinguish this from the "delete on disk and server" in the retention block.

(In reply to comment #5)
> I think this UI is ugly and hard to localize properly.

That's what Blake ended up with in bug 510707 after a couple of iterations, with multiple strings to support localization (those are empty in en-US).
Adding the radio-buttons would require changes in his JavaScript part.

Comment 7

[rsx11m]

The oddity may be that the line starts with an input field and not with a text. Following could be implemented without paging Blake to change the structure:

(1) Download and store locally [ ] [ ] messages
(2) Download and keep [ ] [ ] messages for offline use

or similar, in which case I'd change the accesskey to match useAutosyncBefore.
Note that (2) reintroduces "offline use" if you prefer that.

Comment 8

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

(In reply to comment #6)
> > I think this UI is ugly and hard to localize properly.
> That's what Blake ended up with in bug 510707 after a couple of iterations,
> with multiple strings to support localization (those are empty in en-US).
> Adding the radio-buttons would require changes in his JavaScript part.

I have no problem changing the javascript, if it leads to a better UI.
(The changes would likely be fairly minimal, no matter what the UI ends up as.)

Later,
Blake.

Comment 9

[rsx11m]

Hi Blake, I'm happy to finalize the patch in attachment 399926 [details] [diff] [review] for
any label changes like those proposed in comment #7 / bug 510707 comment #41 or similar. I'd take your offer and turn it over to you though if more substantial changes with radio buttons, etc., are needed to get it done properly.

Bryan, any suggestions?

Comment 10

[Francesco Lodolo [:flod]]

Typo restriced -> restricted

I still think that a structure similar to the one proposed in comment 5 is easier to localize (and prefs like "not older" involve too much thinking).

------------------------------------------------
(o) Keep stored on disk all messages     
( ) Delete messages older than [ ] [  ↓]

The current situation is quite a l10n nightmare: for example in Italian we're using the string before the two dropdown menus, the result is something like "Keep messages on disk [number] [forever/days/etc.]". 

The problem is that the first menu is disabled if you choose "forever", and that doesn't make much sense. With the proposed structure, both menus are disabled if you choose to keep all messages on disk.

BTW, adding [e-mail] to involve other localizers

Comment 11

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

I'm happy to move to a structure like proposed in comment 5. Do we have suggestions for the wording to use there?

Comment 12

[Rimas Kudelis [use needinfo to ping]]

I agree with Francesco. "Keep 31 All email" doesn't make much sense. flod's UI proposal seems fine to me, though may I also suggest to rethink whether we need the interval chooser at all. Perhaps just days would be enough?

Alternate UI/wording suggestions:
UI:
 [v] Remove messages older than [____][  ↓] from cache
or:
 [v] Remove messages older than [____] days from cache
(but perhaps months or weeks would be more appropriate?)

This would require two or three accesskeys to be really accessible, but is common enough in Thunderbird.

Wording:
  (o) Keep all messages in cache
  ( ) Remove messages older than [____] [   ↓] from cache
or:
  (o) Keep all messages in cache
  ( ) Remove messages older than [____] days from cache
(again, perhaps months or weeks would be more appropriate?)

I used "cache" instead of "disk" in my examples, as "removing from cache" doesn't sound as scary as "deleting from disk" to me. :) Exact wording is up to the UX people, I guess.

Comment 13

[Rimas Kudelis [use needinfo to ping]]

Uh, and it just hit me that two radios will require 3 or 4 accesskeys, while the checkbox will only require 2 or 3.

Comment 14

[Rimas Kudelis [use needinfo to ping]]

BTW, could we make the scope of this bug a little wider? I think all of the options shown in Attachment 399928 [details] require some sort of work. In particular:

1) autosync and retention options are very similar in nature, so I think they should have similar interfaces and similar default values E.g. has anyone noticed that the default disabled value for removing mail from cache is 31 days, but the default disabled value for deleting old mail is 30 days?

1a) with gigabyte size mailboxes, are retention options still useful, or are they just a potential cause for a headache? IMO, retention options could be moved into an extension.

2) "To save disk space, do not download for offline use:" looks like a header for a list, but the list only consists of one item. I think this could be redone as follows:
   [v] To save disk space, do not cache messages larger than [___] KB

Comment 15

[Kadir Topal [:atopal]]

As I said in 510707, I don't have a good solution, but maybe not putting a date type (weeks/years) together with a quantifier in one dropdwon menu would help. Maybe a way to select between all/none and an intervall would be in order.  And keep in mind, I'm localising into German, which is pretty similar to English, other languages might pose even bigger problems.

[x] Save mails on disk
    (o) All E-Mails
    (o) for the last [3] [weeks]

Comment 16

[rsx11m]

Ok, let's get back to the main reason I've opened this bug, which was to more clearly distinguish the autosync policy settings from the retention settings. The key components in the labels would be:

 - autosync: Download and keep locally... (won't do anything on the server)
 - retention: ... local copies and originals on the server.

That distinction has to be part of any label, so "Save mails on disk" and using "cache" in this context (which is a completely different thing) won't do well.

Re comment #14, I don't think the retention-policy settings or labels should be touched any more at this late time. As for the autosync option, changing its default to 30 rather than 31 days would be consistent, I agree. And thanks for pointing out the typo in new description, I've missed that.

I'm adding Ian and Neil to comment as this also affects the suite.

Comment 17

[rsx11m]

Another idea to make the autosync settings more similar to the retention settings may be to provide a more detailed initial description (it spans two lines for the retention as well), followed by the list of radiobuttons and a checkbox for additional restrictions by download size.

How about the following, which should address most of the issues raised here:


To save disk space, downloading messages from the server and keeping them
as local copies for offline use can be restricted by age or size.

 (o) Download all messages locally regardless of age
 ( ) Download only messages of the last [30] [days]
 [ ] Do not download messages larger than [50] KB

Comment 18

[Rimas Kudelis [use needinfo to ping]]

My point in comment #14 was that except the "...and originals on the server" part, autosync and retention settings are basically the same, so there's no point why their UI's should differ (except the wording), because whatever we decide is the best UI, should suit both cases very well, so, why not:


To save disk space, downloading messages from the server and keeping them
as local copies for offline use can be restricted by age or size.

 (o) Download all messages locally regardless of age
 ( ) Download only the most recent [2000] messages        (if implemented)
 ( ) Download only messages of the last [30] days
 [ ] Do not download messages larger than [50] KB


Note: I removed the drop-down and I think that instead of days, we could use weeks, as that's still unambigous and precise enough. But then I think we should do that with retention policy too. Or we could have the drop-down in both radio groups, but it doesn't make sense to only have it in one.
Note2: I don't feel like the word "download" fits best here, but I'll leave it up to English speakers...

Comment 19

[rsx11m]

(In reply to comment #18)
>  ( ) Download only the most recent [2000] messages        (if implemented)

This specific feature is not implemented, but there are other bugs pending
(see references in bug 482476) for space, download-rate, and on-demand-only limitations (thus, those are quite different from the retention settings).

"Download" relates more to the process of transferring data from the server
to the local offline store, whereas "store" somehow implies they are already
on the disk.

Comment 20

[Magnus Melin [:mkmelin]]

Rimas: retention settings are different in that the autosync settings are not necessarily about disk space but also about what to to have downloaded in the first place - which will take time, bandwidth and processing you may want to use at a certain in the situation.

Many of the suggestions so far sound on the longish side imo. Less is more. I like something short like comment 15 (but with somewhat changed wording).

Comment 21

[Robert Kaiser]

What about

  Download message bodies for [all messages           v] [ 3] [days   v]
                              |messages from the last  |      |weeks   |
                                                              |months  |
                                                              |years   |

?

Comment 22

[Francesco Lodolo [:flod]]

(In reply to comment #21)
> What about
> 
>   Download message bodies for [all messages           v] [ 3] [days   v]
>                               |messages from the last  |      |weeks   |
>                                                               |months  |
>                                                               |years   |
> 

Assuming that you can fit this gracefully in a single line, it's not ok: last drop-down menu could have terms with different genres, while the adjective ("last") is in the first drop-down menu. Also moving "last" to the final drop-down is not an option.

Example (Italian):
[messaggi degli ultimi] 5 [giorni]
[messaggi delle ultime] 5 [settimane]

Comment 23

[Rimas Kudelis [use needinfo to ping]]

(In reply to comment #21)
> What about
> 
>   Download message bodies for [all messages           v] [ 3] [days   v]
>                               |messages from the last  |      |weeks   |
>                                                               |months  |
>                                                               |years   |
> 
> ?

This way, it may sound like this in the end:
Download message bodies for all messages 3 days. In the end, this is almost the same as what we have now...

By the way, what happens when the message is older than the specified timeframe, but has been downloaded already? Are such messages being removed from disk, or does this setting apply exclusively to downloading the messages from server?

Comment 24

[rsx11m]

According to bug 482476 comment #18, the retention backend for local copies is used to cleanup the offline store if an entry exceeded the selected time frame, while the server versions are not touched (if I read this correctly). Thus, the phrase "download and keep" would express that.

I'd agree with the concerns on comment #21, this still would retain the natural sentence l10n issue. My favorite remains comment #17, also with respect to the overall visual appearance of that pane and the similarity of the option blocks.

(In reply to comment #22)
> [messaggi degli ultimi] 5 [giorni]
> [messaggi delle ultime] 5 [settimane]

Now, that's a potential bummer for the interval drop-down menu. While it may be possible to change the introductory label with its value, it would probably be quite confusing to the user (and the menu may shift with different labels).

> (In reply to comment #18) I think that instead of days, we could use
> weeks, as that's still unambigous and precise enough.

The underlying preference has a unit of days as well, thus it seems to be a natural choice (it's calculated back to days from whatever interval you choose in the current UI implementation). It's fairly easy to think of a quarter year (or 3 months) as 90 days (3m*30d), I'd have to think twice to come up with the correct number of 13 weeks. Once you get into years, either requires some math.

> (comment #20) Many of the suggestions so far sound on the longish side imo.

Yes, if something is supposed to happen in time for 3.0, we'd have to agree on some change here by the end of this week so that it can be implemented during the next week (i.e., in time for the final string freeze).

Comment 25

[Pavel Franc - Mozilla.cz]

I prefere any solution separating the "all" and "n days" options (#5, #17, ...). And I would strongly suggest not to use multiple inputs/selects for one option - it's not only hard to localize but IMO confusing for users.

Comment 26

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

Comment on attachment 399926 [details] [diff] [review]
Proposed patch

clearing this from my queue.  I think I agree that Comment #17 looks like the best available option for consistency with other options even if it takes up a more room.

Comment 27

[rsx11m]

Thanks for the feedback, I'll proceed towards a new patch following comment #17 then. I'm still concerned with comment #22 though:

> [messaggi degli ultimi] 5 [giorni]
> [messaggi delle ultime] 5 [settimane]

Would a less natural-sentence construct resolve this problem, something like

> ( ) Synchronize recent messages only, limit [30][days]

with the "limit" part separated from the primary label? This uses "Synchronize" rather than "Download" to make the "download & keep" context more clear.

Comment 28

[Francesco Lodolo [:flod]]

(In reply to comment #27)
> Would a less natural-sentence construct resolve this problem, something like
> 
> > ( ) Synchronize recent messages only, limit [30][days]
> 
> with the "limit" part separated from the primary label? This uses "Synchronize"
> rather than "Download" to make the "download & keep" context more clear.

I suppose that it could be a problem if you need an article for days/weeks/years (this is not the case for Italian). 

Personally I don't like this structure (is sounds like we're out of space and need to shorten the sentence).

Comment 29

[rsx11m]

I'll have an updated patch ready by tomorrow with radiobuttons on the top level and an "all"/"by age" choice. Space for the label may indeed be an issue as now an additional checkbox (and an indent) is added to the original version. Labels may still be subject to discussion, but at least the general structure would be in place for that options block.

Comment 30

[rsx11m]

Attachment: Patch using radiobuttons (v2a)

This patch implements comment #17, using "Synchronize" rather than "Download" for the age-related labels. Also, the second label is shortened to fit into the horizontal space available (screenshot follows). Further issues addressed in this patch:

- Selecting the "Synchronize all" option disables both textbox and drop-down
  menu for the second option;

- useAutosync is replaced with ageAutosync as further options may be added in
  the future;

- this patch includes the bug 515614 fix to consistently use "-1" for "all";

- changes default from 31 to 30 days per comment #14, also avoiding that
  accepting the default and returning to the pane flips it to 1 month instead
  (which is still 31 days);

- despite bug 510707 comment #31 I've changed the size of the textbox from 5
  to 4, thus corresponding to the textbox underneath, and assuming that values
  of more than 9999 days are unlikely to be used.

Note that the drop-down menu to select the time interval is retained. While one may argue that "Years" could be overdone, it still sounds plausible to use "Days", "Weeks", or "Months" to specify that value (how many days are 18 months?). The obvious trade-off is to either send a lot of Blake's patch to the attic and hard-wire the interval (for which I'd suggest a "days" granularity) or to hope that some useful localized representation of that option in any language is possible. I'd still say though that this option will likely be more used than the age-related retention settings and is therefore well justified.

Comment 31

[rsx11m]

Attachment: Patch using checkbox (v2b)

This patch is identical in functionality but replaces the two radiobuttons with a single checkbox. The rationale is to reduce the space needed a bit and also to make that block more consistent across account types (both POP3 and NNTP accounts have a "Do not download..." block with checkboxes only).

- The introductory description is shortened and also uses "don't download
  and keep" to provide the same context as for the other account types;

- this allows to shorten the label to "Messages older than [ ] [ ]" which
  looks more similar to the download-size restriction underneath, and the
  "than" may be more l10n-neutral than the "last" or "recent" component
  relating to days/weeks/months in the original patch;

- there is no explicit "Synchronize" in the checkbox label, to keep it simple
  (and it's mentioned in the headings), but the context should be unambiguous.

- keeping in mind that bug 511118, bug 506024, and bug 508276 would add further
  autosync-policies, a checkbox version may be easier to extend by similar
  constructs, whereas radiobuttons would suggest more of an either/or policy
  definition (retention policy is a mix of those).

With respect to the last item, while none of those related bugs look at the moment as if they would make the string freeze next week, it's quite likely that this options block will be revisited in the future towards the next major release.

I'm putting this up for ui-review as it appears to be the more concise and practical version.

Comment 32

[rsx11m]

Attachment: Comparison v2a (left) vs. v2b (right)

Windows XP with Windows Classic shown here, also tested on Linux/KDE resulting in similar spacing.

Comment 33

[rsx11m]

A quick follow-up to am-offline.dtd:

For doNotDownloadImap.label, the phrase "and keep locally for offline use" should read "and keep local copies for offline use" as in comment #17. In this way, it is unambiguous that their originals are not removed from the server.

The localization note in v2b contains a reference to useAutosync.ByAge, which is not applicable in that version and should be removed.

Bryan: I assume that a ui-review+ from you covers these changes too. In case you prefer v2a over v2b, please consider the ui-review request also valid for attachment 401759 [details] [diff] [review] (I didn't want to flag 2 patches at the same time).

Comment 34

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

Comment on attachment 401759 [details] [diff] [review]
Patch using radiobuttons (v2a)

Thanks for heading all this up.

Looking over your comparison in attachment 401761 [details] I think a key element for this (v2a) is radio displaying the current state.  I like the checkbox layout of v2b because I think it lines up well and is a bit more concise but I think it requires reading the preceding text (not something that we can have the luxury of expecting people to do) to know what the current state of TB is.

Comment 35

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

(In reply to comment #33)
> For doNotDownloadImap.label, the phrase "and keep locally for offline use"
> should read "and keep local copies for offline use" as in comment #17. In this
> way, it is unambiguous that their originals are not removed from the server.

Agreed.

> The localization note in v2b contains a reference to useAutosync.ByAge, which
> is not applicable in that version and should be removed.
> 
> Bryan: I assume that a ui-review+ from you covers these changes too. In case
> you prefer v2a over v2b, please consider the ui-review request also valid for
> attachment 401759 [details] [diff] [review] (I didn't want to flag 2 patches at the same time).

No problem.  Feel free to flag both in the future, it doesn't happen often enough to be an issue.

Comment 36

[rsx11m]

Comment on attachment 401760 [details] [diff] [review]
Patch using checkbox (v2b)

All right, I'll go with v2a and the radiobuttons then. I've noticed though that offlineNotDownload.label is also used for POP3 and NNTP accounts and looks a bit strange there. Thus, I'll need to do some tweaking to have different labels for the same preference depending on the account type, using the long label for IMAP accounts only.

Comment 37

[rsx11m]

Attachment: [checked in] Proposed patch (v3)

This is an update of attachment 401759 [details] [diff] [review] (v2a) as follows:

 - String changes per comment #33 and comment #36;
 - offlineNotDownload label has been reinstated for POP3 and NNTP;
 - for IMAP only, autosync.notDownload replaces offline.notDownload;
 - controls are associated for all ageAutosync* labels.

Note that limit_offline_message_size is now associated with both the existing offline.notDownload and the new autosync.notDownload controls. This is similar to disable_button.selectFolder being related to both selectNewsgroupsButton and selectImapFoldersButton. Either offline.notDownload or autosync.notDownload are hidden depending on the account type. Only offline.notDownload is used to write back the preference value, thus any change in autosync.notDownload is copied to offline.notDownload for IMAP accounts to ensure the correct pref value.

Carrying forward ui-review+ and requesting r?/sr? for this patch. Thanks to Blake and David Bolter for their e-mail feedback.

Comment 38

[rsx11m]

@standard8: Mark, any estimate when you can take care of the review?

This patch has l10n-string changes and thus doesn't make much sense any more after the string freeze kicks in (3 days from now). Thanks.

Comment 39

[rsx11m]

Final r?/sr? ping for this patch.

(In reply to comment #5)
> Guys, I think this UI is ugly and hard to localize properly.

Unless this is getting some serious attention by the reviewers, is approved and checked in within the next 36 hours or so, or it gets some late-l10n exception by whoever can grant that, it looks like TB 3.0 users and localizers will have to live with the current version. I can't do more than putting up the patch for review a week before the string freeze and pinging the people who are supposed to approve it.

Comment 40

[David :Bienvenu]

Comment on attachment 402083 [details] [diff] [review]
[checked in] Proposed patch (v3)

you don't need the temp vars here, and I think the code is actually clearer if you remove the temp vars.

+  // this function is called when the autosync version of offline.notDownload is changed
+  // it simply copies the new checkbox value over to the element driving the preference
+  let offlineNotDownload = document.getElementById("offline.notDownload");
+  let autosyncNotDownload = document.getElementById("autosync.notDownload");
+
+  offlineNotDownload.checked = autosyncNotDownload.checked;

Comment 41

[Mark Banner (:standard8)]

Comment on attachment 402083 [details] [diff] [review]
[checked in] Proposed patch (v3)

a=Standard8 on the basis that this is an improvement for localisation and it makes the whole pref clearer. Should be lowish risk as well.

Comment 42

[Mark Banner (:standard8)]

Given the string freeze deadline, I've taken the liberty of addressing David's comment and checked the patch in:

http://hg.mozilla.org/comm-central/rev/5c30e830c725

Comment 43

[rsx11m]

Great, thanks!

Comment 44

[Rimas Kudelis [use needinfo to ping]]

For some reason ageAutosyncAfter.label doesn't appear in the interface to me (Windows, lt). Can anyone confirm that or is it just me?

Comment 45

[rsx11m]

You are right... sorry, seems to be some stupid typo!
I'll have a follow-up patch in a few minutes.

Comment 46

[rsx11m]

Attachment: [checked in] Follow-up patch (v3.1)

Here is the story: I've pulled the labels from bug 510707 inside the <label/> construct, but used label="" instead of value="", thus they don't show up.
Just another example that trying to optimize things may be dangerous...

This is the follow-up patch, now verified with ageAutosync{Middle,After}.label being functional. I've also removed the flex="1" from ageAutosyncAfter as it doesn't make any difference in appearance (should group left-to-right anyway).

Sorry for the glitch.

Comment 47

[David :Bienvenu]

Comment on attachment 403790 [details] [diff] [review]
[checked in] Follow-up patch (v3.1)

thx for the quick fix.

Comment 48

[Francesco Lodolo [:flod]]

Sorry but I completely lost track of this bug.

Maybe I'm wrong, but I see quite a l10n problem in these strings (same issue explained in comment 22): how I am supposed to match "recent" with different genders (male/female)? 

Right now I'm using a string that sounds like "Limit sync to [xx] [days/weeks/etc.]", but that's far from optimal.

Comment 50

[Mark Banner (:standard8)]

Comment on attachment 403790 [details] [diff] [review]
[checked in] Follow-up patch (v3.1)

Checked in: http://hg.mozilla.org/comm-central/rev/5d5ba876db04

Comment 51

[rsx11m]

(In reply to comment #48)
> Maybe I'm wrong, but I see quite a l10n problem in these strings (same issue
> explained in comment 22): how I am supposed to match "recent" with different
> genders (male/female)? 

Just to respond to that, you are right that the patch clarifies the function
of that setting and splits the "All" case off the menu (which was the primary concern), it couldn't take care of the natural-sentence issues in comment #22. The alternative would have been the right part of attachment 401761 [details],
with (v2b) offering a "Messages older than: [xx][days]" construct. However, per comment #34, the current variant (v2a) was chosen for the sake of clarity.

> Right now I'm using a string that sounds like "Limit sync to [xx]
> [days/weeks/etc.]", but that's far from optimal.

The (v2b) version would have required a similar phrasing you have chosen now. It's a trade-off, but should be an improvement over bug 510707 and hopefully will work out somehow for all languages. It's tricky to find a universal solution for this rather complex construct...