326584 - Message Aging: clean up UI, remove "Always delete read messages" checkbox

Status: RESOLVED FIXED

(Thunderbird :: Account Manager, defect)

Description

[Marcia Knous [:marcia]]

I am confused about the fact that you can have the radio button which says
"don't delete any messages" and the checkbox which says "Always delete
read messages" both checked. 

From David: Thunderbird honors the the "always delete read messages" checkbox. The "don't delete any messages" text is basically wrong - it should be something like "don't purge messages by age or number of messages" or the UI should be reworked to make more sense. Perhaps all checkboxes. Originally, I think the dialog used the wording "keep all but the old messages", "keep all but XX messages" and then "delete unread messages" but someone "improved" it. 

I suggest we improve the wording for this or change the UI to make it more clear for users which preference is being honored.

Comment 1

[rsx11m]

> (excerpt from bug 11055 comment #49) Yes, that option really deletes
> all read mail. It's intended for newsgroups where you never want to keep read
> messages - it makes a little more sense for news, I guess, because you're only
> deleting the headers. I'm open to removing it if people think no one would ever
> use it, or hiding it for non-news.

I don't really see a use case to delete *all* read messages, other than the one stated here for news messages. Thus, hiding it for local or mail folders would resolve the issue and avoid an accidental "delete all my mailboxes" triggered
by that functionality (based on the logic in bug 410597, that ambiguity alone should warrant blocking-tb3 status for this bug).

Comment 2

[rsx11m]

Attachment: Current Synchronization & Storage labels

This shows the appearance for a news account as is covers all options.
Note that there are more labels which are ambiguous and could be improved, underlined in yellow.

The "Only message bodies less than [__] days old" option is only available
for news accounts. It is not clear whether this is meant in the context of "permanently deleted" or "Keep only message bodies", apparently making a substantial difference. I'd guess that it is meant in the sense of "Delete
only message bodies more than [__] days old" as a counterpart to the general "Delete messages more than [__] days old" option.

The question aside whether or not "Always delete read messages" should be hidden for non-news accounts (I'd consider the opposite logic "Always keep unread messages" more useful and consistent with the "Always keep starred messages", but this would require some backend changes), following new labels should represent the options better:

  (x) Don't delete any messages based on their age
  ( ) Delete all but the most recent [__] messages *
  ( ) Delete messages more than [__] days old

  [ ] Delete all read messages regardless of their age
  [x] Always keep starred messages
  [ ] Delete message bodies more than [__] days old **

* "last" is ambiguous as it doesn't clearly indicate time, "most recent"
  should be more specific.

**does everybody understand this as headers being retained for such messages
  unless they are "fully" delete by one of the other options?

Comment 3

[rsx11m]

Nominating for "wanted" status. If it's just about hiding the "delete read" box where it's not useful (rather than changing its backend logic) and rephrasing the labels, this should be feasible for 3.0rc1.

Comment 4

[Magnus Melin [:mkmelin]]

I agree this is an anti-feature, and we should probably get rid of it all together...

Comment 5

[rsx11m]

Removing the checkbox for "Delete all read messages" from the account manager for all account types would certainly simplify things. In that case, the "Don't delete any messages" label would be unambiguous and no "based on their age" or "regardless of their age" addition necessary. The functionality should still be retained as hidden pref, in case someone considers the nonfeature as a feature.

Comment 6

[Magnus Melin [:mkmelin]]

Keeping a feature because somewhere out there 3 or 4 people *might* possibly actually be using it is not a good reason to keep it, especially as choosing it mistakenly can cause major dataloss. This should be easy to do as an extension and the manual workaround is a a no more than a few clicks.

Comment 7

[rsx11m]

Since nobody stated any reason to retain that checkbox in the Account Manager UI, I'm retargeting this bug to remove it and clean up the labels in the message aging section of Synchronization & Storage / Disk Space. I don't see the necessity to remove the pref and its backend completely, but if desired
to do so, this could be handled in a MailNews/Backend follow-up bug.

Comment 8

[rsx11m]

Attachment: Proposed fix

This patch removes the "Always delete read messages" checkbox and clarifies a couple of other labels:

(x) Don't delete any messages
( ) Delete all but the most recent [__] messages
( ) Delete messages more than [__] days old
[x] Always keep starred messages
[ ] Remove bodies from messages more than [__] days old

Since the context of message aging is now unambiguous, I didn't change the first label. Instead of "most recent" also "newest" could be used (that's the choice in the SeaMonkey Folder Properties dialog).

Comment 9

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

Comment on attachment 382029 [details] [diff] [review]
Proposed fix

The "most recent" wording seems much clearer than "newest".  Thanks, this looks great!

Comment 10

[rsx11m]

Bryan, thanks. I'll wait for the bug 474525 fix to check in and will then post an updated patch here for r/sr with the SM-Help changes added.

Comment 11

[rsx11m]

Attachment: Proposed fix (v2)

New patch with complete suite/ updates, nothing changed in either mail/
or mailnews/ parts.

Carrying forward ui-review+ from attachment 382029 [details] [diff] [review].

Comment 12

[Mark Banner (:standard8)]

Comment on attachment 382147 [details] [diff] [review]
Proposed fix (v2)

>     <hbox align="center" class="indent">
>       <vbox>
>-        <checkbox id="retention.keepUnread" wsm_persist="true"
>-                  label="&retentionKeepUnread.label;"
>-                  accesskey="&retentionKeepUnread.accesskey;"
>-                  checked="false"/>
>         <checkbox id="retention.applyToFlagged" wsm_persist="true"
>                   label="&retentionApplyToFlagged.label;"
>                   accesskey="&retentionApplyToFlagged.accesskey;"
>                   checked="true"/>
>       </vbox>
>     </hbox>

Now we have only one item, we don't need the additional vbox element.

>           <hbox class="indent">
>             <vbox>
>-              <checkbox id="retention.keepUnread" wsm_persist="true"
>-                        label="&retentionKeepUnread.label;"
>-                        accesskey="&retentionKeepUnread.accesskey;"
>-                        observes="retention.keepMsg" checked="false"/>
>               <checkbox id="retention.applyToFlagged" wsm_persist="true"
>                         label="&retentionApplyToFlagged.label;"
>                         accesskey="&retentionApplyToFlagged.accesskey;"
>                         observes="retention.keepMsg" checked="true"/>
>             </vbox>
>           </hbox>

ditto.

r=Standard8 with those fixed.

Comment 13

[[e-mail]]

Comment on attachment 382147 [details] [diff] [review]
Proposed fix (v2)

There's no backend to keep bodies by days - It looks to have got lost :-(

Comment 14

[rsx11m]

Attachment: Final patch [backed out, comment #24]

For checkin on comm-central with <vbox> removed per comment #12.

The indentation in am-offline.xul is somewhat inconsistent, thus I retained
the level to match the ones in the other entries.

Comment 15

[rsx11m]

(In reply to comment #13)
> There's no backend to keep bodies by days - It looks to have got lost :-(

Couldn't find anything filed on that, but if the backend is indeed unfunctional, this sounds like a follow-up bug to make it work again...

Comment 16

[Mark Banner (:standard8)]

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

Comment 17

[Alex Ihrig [:Thunderbird_Mail_DE]]

I'm wondering, what happens for people who have enabled the now removed option? Is the option still working in the background / backend? IMHO, this would cause more "dataloss" than before.

Comment 18

[rsx11m]

Only the checkbox has been removed, the preference itself is defined on a per-server basis and accessible as mail.server.server#.keepUnreadOnly

This is essentially the same situation as with any other preference for which the  UI was removed, but which can still be accessed through the Config Editor.

Comment 19

[Magnus Melin [:mkmelin]]

Except that's not possible for per-folder settings...

Comment 20

[rsx11m]

Ok, that's a good point, those flags are probably hiding in the msf files.
What's the solution? Migrating the settings to keepUnreadOnly="false"?

Comment 21

[rsx11m]

*** PLEASE BACK OUT IMMEDIATELY !!! ***

That's a major glitch - while running this on a larger profile and monitoring mail.server.server#.keepUnreadOnly, I've noticed that in certain circumstances that preference can flip to *true* when visiting and leaving Disk Space.

Also, when creating a new account, that pref is first not existent, but if you enter Synchronization & Storage / Disk Space for that account the pref is created, again with a value of "true".

Sorry for that, the first case didn't seem to occur when testing with a different profile, and I didn't test the setup-from-scratch case.  :-[

Comment 22

[rsx11m]

First report in the MZ Forums already, I've posted a warning there.
This is bad, need to look what the backend is doing...

It appears that the respective variable is nowhere initialized and only got initialized by the checked="false" entry in the previous version. I'm wondering though why it is unpredictably changing when the pref is present but nowhere used any more in the UI code. What did I miss?

Comment 23

[Mark Banner (:standard8)]

I've seen the comments and am taking a look at this.

Comment 24

[Mark Banner (:standard8)]

Backed out: http://hg.mozilla.org/comm-central/rev/e59b05bbbb64

I'll fire off some new nightlies once the normal builds have started cycling green.

So here's my best guess as to the issue:

- nsMsgRetentionSettings doesn't initialise any of its member variables [1].
- The retention code for loading and saving prefs creates and uses separate nsIMsgRetentionSettings instances - not transferring the existing prefs between them, see [2] and the code that uses about it.

The uninitialised nature of nsMsgRetentionSettings then means that in saving the prefs in [3] we're using an uninitialised value for keepUnreadOnly.

[1] http://mxr.mozilla.org/comm-central/source/mailnews/db/msgdb/src/nsMsgDatabase.cpp#4866
[2] http://mxr.mozilla.org/comm-central/source/mailnews/base/resources/content/retention.js
[3] http://mxr.mozilla.org/comm-central/source/mailnews/db/msgdb/src/nsMsgDatabase.cpp#4483

Comment 25

[Mark Banner (:standard8)]

Note: I decided to go for backout rather than trying to produce a fix as a) its faster, and b) folks can double-check their preferences whatever we end up doing.

Comment 26

[rsx11m]

Thanks, I followed the comm-central pushlog and saw the backout...

Comment 27

[rsx11m]

Based on Mark's pointers given in comment #24, it appears that the issue [2] (retention.js) causes the main problem here by using the UI elements to forward the preference values from onInit() to onSave().

In am-offline.js, onInit() uses initRetentionSettings() to set the UI values from gIncomingServer using its own retentionSettings instance. In onSave(), another retentionSettings is created from saveCommonRetentionSettings() which knows nothing about the initial settings and only retrieves those from the UI status. This instance is then assigned to gIncomingServer.retentionSettings, thus leaving retentionSettings.keepUnreadMessagesOnly in an undefined state.
In folderProps.js, the same thing happens in folderPropsOKButton() with gMsgFolder.retentionSettings.

As a workaround to the problem, keepUnreadMessagesOnly could be initialized from gIncomingServer and gMsgFolder before assigning the new instance back, assuming those won't change between onInit() and onSave(). This should work but not necessarily be nice.

Another solution may be to leave the UI backend completely intact and just hide retention.keepUnread, which would satisfy the primary charge of this bug to avoid accidental use of this option (still not addressing the migration issue in comment #17 and comment #19 though, but the hidden checkbox could be easily made visible by a userChrome.css entry then). That's a minimal change, along with the new labels, and would defer any backend work for later.

I can try to start with the first solution, which would retain most of the current patch, and go with the hidden="true" version as a fallback. Either way, a follow-up bug to clean up the backend with proper initialization and more clearer flow of the preferences would probably be a good idea.

Comment 28

[rsx11m]

(In reply to comment #27)
> As a workaround to the problem, keepUnreadMessagesOnly could be initialized
> from gIncomingServer and gMsgFolder before assigning the new instance back,
> assuming those won't change between onInit() and onSave().

Ok, I've tried that, but it seems that gIncomingServer in onInit and onSave are not the same instances either. Following change for onSave() in am-offline.js:

>      var retentionSettings = saveCommonRetentionSettings();
>  
> +    retentionSettings.keepUnreadMessagesOnly = gIncomingServer.retentionSettings.keepUnreadMessagesOnly;
>      retentionSettings.daysToKeepBodies = document.getElementById("nntp.removeBodyMin").value;
>      retentionSettings.cleanupBodiesByDays = document.getElementById("nntp.removeBody").checked;
>  
>      downloadSettings.downloadByDate = document.getElementById("nntp.downloadMsg").checked;
>      downloadSettings.downloadUnreadOnly = document.getElementById("nntp.notDownloadRead").checked;
>      downloadSettings.ageLimitOfMsgsToDownload = document.getElementById("nntp.downloadMsgMin").value;
>  
>      gIncomingServer.retentionSettings = retentionSettings;

should copy keepUnreadMessagesOnly from gIncomingServer to the same variable in retentionSettings returned by saveCommonRetentionSettings(); when checking in the Config Editor, keepUnreadOnly is set to false now every time when leaving the Disk Space / Synchronization & Storage pane. So, it's probably initialized that way when the gIncomingServer instance is created, or I've only moved the non-initialization problem from retentionSettings to gIncomingServer.

It appears that the XUL checkbox is the only glue connecting those two ends...

Comment 29

[rsx11m]

Yepp, this didn't help. I've made a small change by introducing a local
variable for gIncomingServer.retentionSettings and now it flips to "true" instead. Digging in AccountManager.js didn't bring me much further; those on...() functions are called in a rather generic way, with gIncomingServer
being set in onPreInit() from an "account" parameter.

Unless someone has a better suggestion, the two options I'd see are #1 to set
the variable to false (thus essentially disabling the feature) or #2 to go with
the hidden checkbox variant to retain the XUL functionality of intermediate storage of the current preference values.

Comment 30

[rsx11m]

Now, this is getting funnier by the minute. I've noticed that changing the
pref doesn't update the checkbox in the Account Manager either, using today's Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1pre) Gecko/20090612 Shredder/3.0b3pre nightly build.

Steps to reproduce:
1. Open Synchronization & Storage and verify "delete read messages" status;
2. close Account Manager;
3. open Config Editor and toggle respective mail.server.server#.keepUnreadOnly;
4. close Config Editor and go back into Account Manager;
5. note that checkbox has *not* been updated, click Ok;
6. go back into Config Editor and note that pref has been toggled back.

This doesn't look right to me. As for continuing the bug here, I tend to leave the backend untouched based on the recent tests and just hide the box as the proposed fallback. It could be easily activated by an extension adding a label and removing the hidden attribute, or I could give it a label and a class name by which it can be made visible with a simple userChrome.css entry.

Comment 31

[Magnus Melin [:mkmelin]]

You may be interested in bug 472203. I *think* it's just some strange UI issue but may well be related.

Comment 32

[rsx11m]

Attachment: Updated fix (v3.2)

This is the patch which just hides the checkbox in the UI and updates the labels, but leaves everything else in the backend in place. From the user's point of view, the UI looks exactly like the previous patch. I'm re-requesting ui-review for the option to unhide that box though if desired (comment #17 and comment #19). For that case, the label was retained but extended, otherwise you would only see an empty checkbox there (the accesskey definition was removed).

Following details:
1. no changes to retention.js (I'll leave this for bug 472203 or another one);
2. both XUL parts have the keepUnreadOnly checkbox hidden by default;
3. they can be unhidden with a userChrome.css entry if desired;
4. if unhidden, the label is more specific ("overrides age settings");
5. am-offline.js no longer toggles visibility for deferred POP accounts;
6. this also includes the fix for bug 497383 (missing hidefor="" attribute).

Testing scenarios:
 - work with existing accounts;
 - set up account from scratch;
 - remove msf files from folders;
 - (a) set "delete read" for folders and accounts when visible,
   (b) hide box and revisit account manager/folder props,
   (c) unihide box and verify settings;
 - checked Config Editor and msf/mork entries.

For all I can tell, this behaves "nice" now - no more unintended switches,
true and false values were retained, initialization of keepUnreadOnly on new accounts with false as intended.

Re comment #31: Yes, I've observed bug 472203 as well while testing, without finding any specific pattern when "use server defaults" switches. That bug is specific to the Folder Properties whereas I saw comment #30 in the Account Manager, nevertheless that may be related of course.

Comment 33

[[e-mail]]

Comment on attachment 383186 [details] [diff] [review]
Updated fix (v3.2)

>+<!-- LOCALIZATION NOTE: Unhide with .keepUnreadOnly { display: inline !important; } -->
display: -moz-box; (see line 16 of xul.css)

Comment 34

[rsx11m]

(In reply to comment #33)
> display: -moz-box; (see line 16 of xul.css)

Thanks, I figured that there should be a -moz-something but managed to miss
the "*" a few lines up. Interesting that this works without "important!" as it overrides the "display: none;" associated with the hidden="true" attribute.

Comment 35

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

Comment on attachment 383186 [details] [diff] [review]
Updated fix (v3.2)

ok, it'd be good to have some docs put on in the knowledge base or somewhere about this.  I don't really like userChrome solutions but it does seem appropriate here.

Comment 37

[rsx11m]

(In reply to comment #35)
> ok, it'd be good to have some docs put on in the knowledge base or somewhere
> about this.

I have already added a warning to http://kb.mozillazine.org/Message_aging on the ambiguity of the current labels and will update this once the fix is in.

A new patch addressing comment #33 with the updated notes is ready, but I'll wait for Mark's review before posting it in case other changes are necessary.

Comment 38

[Mark Banner (:standard8)]

Comment on attachment 383186 [details] [diff] [review]
Updated fix (v3.2)

r=Standard8 with Neil's requested change.

Comment 39

[rsx11m]

Attachment: [checked in] Updated patch for checkin

Thanks, this is ready to be pushed on comm-central with comments changed.

Comment 40

[Mark Banner (:standard8)]

Comment on attachment 384081 [details] [diff] [review]
[checked in] Updated patch for checkin

Checked in: http://hg.mozilla.org/comm-central/rev/173f8884efed

Comment 41

[rsx11m]

I'm currently using the Win32 tinderbox build 20090619055905 and thus far didn't see any problems with the checkbox either hidden or unhidden.

I've filed a couple of follow-up bugs covering comment #13 (news backend), comment #30 (specific for this issue), and a more general report on the backend not being initialized and only glued through the XUL part (comment #27 and the following ones). I'm adding those bugs as dependency to keep track of them.