Closed Bug 498209 Opened 13 years ago Closed 13 years ago

Recipient is no longer a choice for column list display in normal (not Sent) folders

Categories

(Thunderbird :: Folder and Message Lists, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 3.0b3

People

(Reporter: justdave, Assigned: asuth)

References

(Depends on 1 open bug, Blocks 1 open bug)

Details

(Keywords: regression)

Attachments

(1 file, 1 obsolete file)

I have typically had both the Sender and Recipient columns enabled in my normal message folders, because I get a lot of mail addressed to odd places that all forward to me, and it's useful to know who it was actually sent to before I open it.  The Recipient column disappeared, and is no longer available as a choice to add, about the same time bug 474701 landed.
Depends on: gloda-ui-regressions
No longer depends on: 498109
Which build? (See Bug 498291 for "no From if Sent/Drafts")
(In reply to comment #1)
>> Which build? (See Bug 498291 for "no From if Sent/Drafts")

>  The Recipient column disappeared, and is no longer available as a
choice to add, about the same time bug 474701 landed.

This gives you the answer
without wanting to be too aol-esque on this bug, having the ability to view both Sender and Recipient on all folders is incredibly useful.  Please revert this functionality to what it was before.  

My usage case is this:  I have a number of email identities configured, each of which corresponds to various roles / features that I do.  So for both inbound and regular storage mailboxes, I need to be able to see what address an email came from and who it was destined to.

It's not a whole load of extra feature bloat, and it does provide very useful functionality.
Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9.1pre) Gecko/20090612 Lightning/1.0pre Shredder/3.0b3pre ID:20090612031514

did contain the recipient column as an option.  The next nightly,

Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9.1pre) Gecko/20090613 Lightning/1.0pre Shredder/3.0b3pre ID:20090613031711

dropped the column as an option.

So this was lost sometime after the 2009-06-12 build.
Another use case for this:  I have an entire domain with a catch-all on it, so anything sent to any address you make up in that domain works.  Lots of it is spam of course...  which Thunderbird/Shredder happily filters to Junk for me.  But you need to check that every so often to make sure of no false positives.  When I can see who the message was addressed to, it's really easy to tell when a message is spam just by looking at the recipient address since if it was sent to an address I haven't used, it's obviously junk.
I too use *both* sender and recipient in my views, as the information is very useful to me.  I sure see this as a pretty important regression.

For folder such as Trash and Archives, and when the option is set that stores a message's Reply in the folder along with the original, the concept if incoming/outgoing folder really doesn't make sense.  So basing the column choice of From or Recipient seems iffy at best.

Please don't try to make TB think it knows better which columns to show.  I think I'm the better judge about what data I want to see. :-)
Keywords: regression
Flags: blocking-thunderbird3?
I don't think we need to justify this column with use cases. 

This surely wasn't a design choice but rather a serious and unintended regression that breaks a lot of people and will, presumably, be fixed in short order,
targetting at b3
Flags: blocking-thunderbird3? → blocking-thunderbird3+
Target Milestone: --- → Thunderbird 3.0b3
while/if Andrew is looking at the other column stuff...
Assignee: nobody → bugmail
I see that this bug has already been signaled. Just to add my two bits worth: We have 6 users all using the one IMAP account. The recipient column allows us to know when an email is directed to a specific user and also allows us to filter emails to a specific folder. We see in the recipient column, for example, "Peter" when the email is addressed to "Peter <info@companyx.com>".

For us this is moderately important and a great convenience.

Regards
Peter
I'm not going to be able to get to this until I get back on Monday.  I can probably have something for slushy code freeze...

This bug and its friend bug 498286 are regressions that were intended to be improvements on the existing behavior (but still not the ideal situation.)

My current game plan to fix this and improve things is to persist the visible columns in each folder.  If you enter a folder and we have nothing persisted, we default using the following heuristics:

- If the folder is the Inbox or a special folder (sent or otherwise outgoing/saved search/virtual folder), we default to what we think are sane defaults for the folder type.  For the Inbox this means 'recipient' is off by default (but can be enabled).  For the sent folder this means 'from' is off by default (but can be enabled.)

- If the folder is a normal mail folder (aka not handled by the previous case), we steal the current settings from the Inbox for the account.  The goal is to avoid the situation where you do not like our defaults and get frustrated having to customize every single one of your folders.  The goal is also to avoid having a complex UI to deal with the problem.  The goal is also also to be consistent; inheriting the settings from the last folder you looked at is likely to be inconsistent and drive you crazy.

And if we have persisted columns, we simply restore them.  So once you turn on "recipient" in a folder, it will stay on until you turn it off.

This logic would likely be augmented in the future to let extensions that provide custom columns have the chance to make themselves visible without requiring the user to enable them in every folder.  We would accomplish this by persisting what columns are hidden in addition to visible.  If a column is not mentioned at all, but now exists, then we would let extension logic decide whether it wants to make itself visible after being handed the folder to see if it's interested in the folder.

I do have a concern that this approach might still result in some unhappy users who would prefer that we not persist things on a folder basis because they are dynamic with their use of columns.  The solution to that would be to persist columns based on the type of folder (so one persisted setup for the Inbox, one for Sent folders, one for Archive folders, one for Virtual folders, etc.).  I feel like that is more likely to be frustrating and confusing and that that specific use-case might be best addressed by extensions.

clarkbw, I know we've had discussions about this in the past, but your input would be appreciated.  Also, people are on this bug, brief feedback about whether you think this solution would address your use-cases would also be appreciated.  If you think it would not, please describe a use-case where it does not.
(In reply to comment #11)
> - If the folder is the Inbox or a special folder (sent or otherwise
> outgoing/saved search/virtual folder), we default to what we think are sane
> defaults for the folder type.  For the Inbox this means 'recipient' is off by
> default (but can be enabled).  For the sent folder this means 'from' is off by
> default (but can be enabled.)

I've got a slightly more complicated arrangement than I hinted at in comment #3.  For each of my various role accounts (all of which come off the same imap server account), I have a separate folder which I use as both inbox and sent-mail folder for that identity.  So, if I send or receive an email from a particular role, that email will end up in a single folder.  Currently, I've got 6 joint inbox/sent-mail folders of this form.

The reason for doing this is that all my emails for a particular contract are kept separate and thread history is complete (i.e. it includes everyone's emails).  This means that managed-refiling is trivial and you don't have to go hunting through a single sent-mail folder for replies to particular messages.  It also enforces chinese walls for email.

So could you consider adding this as a test case for whatever code you put in?

Right now, there is a bug such that every time I start Shredder, these joint sent-mail/inbox folders are reset so that neither Receipient nor From field are visible, and when I manually reset the fields, only "Recipient" is available.
Re:  Comment #11 From  Andrew Sutherland (:asuth)  

This is a good approach - would it be feasible to implement a user-defined default setup, like in Windows Explorer applying the current view to all folders (Tools->Folder Options, View Tab->Apply to All Folders button).

In any case, please keep up the good work, and I hope this bug gets squashed soon as not having the ability to view both sender and recipient in the same folder is very limiting at the moment.

Thanks,

d
Whiteboard: [needs input/ui-review clarkbw; see comment 11]
(In reply to comment #11)
> clarkbw, I know we've had discussions about this in the past, but your input
> would be appreciated.

Right, I think this system is going to be the best balance of features and general usability.  We had worked hard to create a "Apply Layout to All Folders" system that doesn't get complicated; however every system that was good enough for the power users became a trap for the novice.  Once we start treating the column layout like a template we inherit all the usability issues of templating systems.

This approach seems like a reasonable take on making folders work for the majority of users.  At the same time there is still plenty of room for extensions to provide column templating systems for more advanced configuration of folders.

On one end of the spectrum we have users who could easily remove a column (or more) and then set that as the default (thinking it was 'save'); it's a bear trap for those who don't understand it.  On the other end of the spectrum we have power users with specific foldering needs; finding an extension that manages column templates is an easier task for power users but a pain to have to rely on an extension.

Also I want to note that bug 359270 has a unified 'who' column that might interest some of the people in this bug.
(In addition to comment #14)
> Also I want to note that bug 359270 has a unified 'who' column that might
> interest some of the people in this bug.

If column label of "From" & Recipient" is changed to "Who" at both Inbox type folder and Sent/Drafts type folder, it'll be same one as Penelope's "Unified Who Column" with Bug 400090 fixed(two Who column, two Who in column choice list). I thought this was the reason why change of this time was made.
(Penelope possibly has capability to detect sent mail based on From: header, in addition to automatic recipient display of sent type folder.)

Is next possible? (Can you abandon "single layout for all folders" system?) 
  -------------  ---------------------------  -------------  ------------
                 Sent/Drafts type folder      Inbox type folder  
  Former column  Column choice  Column label  Column choice  Column label
  -------------  -------------  ------------  -------------  ------------
  From           From           From          Who(From)      Who (*1)
  Recipient      Who(Recipient) Who (*2)      Recipient      Recipient
  -------------  -------------  ------------  -------------  ------------
  (*1 : default of Sent/Drafts type folder, *2 : default of Inbox type folder) 
  =>
  (1) Bug 359270 will be fixed, because above is same as current "Unified Who
      column" of Penelope, except label string.
  (2) Bug 400090 won't occur, because no "two Who" appears.
  (3) This bug and Bug 498286 will be fixed.
First: thank you to everyone who is working on this problem. I do that because I want to raise what I hope is a constructive criticism. 

Second: Would somebody please explain just what was the problem with previous arrangement as used in TB3b2. For me it is important that I am able to choose from all available columns in all folders. I need to be able to see "From" and/or  “Recipient" in all folders, including trash. For me the old system of user customization of columns worked well and flawlessly. This customization of columns in TB3b2 applied to each individual folder and did not affect other folders. The fact that this choice is fixed to an individual folder makes the “unified Who” column quite unnecessary. It is also worth noteing that TB3b2 does not have a  "single layout for all folders" system.

In a folder such as “Trash” that may include both received and sent mail it is not a problem understanding what is intended by the terms "From" and  “Recipient". One does not have to be a genius to understand that if the "From" column includes one of your names or email addresses it is from you, otherwise it is from someone else, and  the reverse applies for the “Recipient" column.

I am somewhat concerned that the “fix” that has given rise to this now existing bug was actually unnecessary and an attempt to fix something that was not broken. 

At least for the moment please revert to the previous code (TB3b2) that was at least functional even if it was felt some improvement could be made. At the very most it is only necessary to make intelligent choices in the default columns displayed in each default folder whilst leaving to choice to customize all possible columns to the end user.

Please let us not complicate our lives more than necessary by trying to second guess what the end user may need or want and then dictate those inflexible choices. 

Peter
Sorry a small correction: I think I now understand what was meant by "single layout for all folders". Whilst it is possible to customize the columns visible in each individual folder the actual position of the column, if changed in one folder, is changed for all folders. I can see that it would be desirable to have the flexibility to a different position for a column in each different folder however, for me, this is not very important.

Peter
Peter 

I agree the old version was MUCH better than the current version - which is non-functional enough (can't see who mail is from) that I think I'm going to have to stop testing and go back to a pre-bug version for usage. 

The need for unified Who is different, it - like the worst problem with this bug - is for those of us who choose "Account Settings"/ Copies & Folders / Place replies in folder of message being replied to.  Otherwise you need to use two columns of limited screen real-estate to show both From and To for each message, in addition you can't sort the mails on the Correspondant. 

- Mitra
Sorry - an addition, I also fail to see why that bit of the code can't be regressed to allow both "From" and "Recipient" columns UNTIL whoever wrote it can rewrite it to work.
Mitra et all

Thanks for the explanation. So to summarise what would be a perfect solution would be as follows:
1. Allow user selection of all possible columns including both "From" and "Recipient" in all folders allowing each folder can have different customised settings. (This reverts to pre-bug style(and probably pre bug code))
2. Add a new "Unified Who" column, also user selectable.
3. Allow customisation of column position that is fixed to the individual folder, not to all folders as in the current version.
4. Set reasonably intelligent defaults for all default folders and allow sub folders to inherit the settings of parent folders.

I believe my list also sets some order of priority or importance. Any comments?

Peter
(In reply to comment #20)
> 1. (snip) (This reverts to pre-bug style(and probably pre bug code))

If "single layout for all folders" system is preferable to make it simple, I think change like next(slightly modified version of yours) can be a practical solution. (Pre-bug "holding of two Inbox-type-layout and Sent-type-layout" produced some confusion of users, and it produced some unwated bug reports at B.M.O.)
(Stage-1) (1-a) "From", (1-b) "Recipient", (1-c) "From or Recipient V1"
          Similar idea to yours.
  (1-c) is new column which works like pre-bug "auto-switch to Recipient column
        if Sent type folder".
        Note: I think "swap of From/Recipientin in filtering area if Sent type
              folder" is better to be removed, if still remains.
  (1-a) & (1-b) doesn't have auto-switch feature as other ordinal column doesn't. 
(Stage-2) "From or Recipient V2"
  (2) doesn't have auto-switch feature as other ordinal column doesn't.
  Sent mail is determined based on From: and/or other headers.
  (and/or based on new per mail property of "Sent Mail" or "Draft Mail") 
If label string of column (1-c) is changed to "Who", It'll be almost same as current Penelope's "Unified Who Column" with Bug 400090 fixed.
If label string of column (2) is changed to "Who", it'll be "Unified Who Column" without Bug 400090, with feature to support mail folder which contains both sent mails and received mails.
(Stage-3) Per folder column layout.
I think this is on the right track.

It is clear that the key words to consider in all this are "user choice and flexibility".

For many of us it is important to be able to display both "From" and "Recipient" in all folders (pre bug) whilst for others require, for very good reasons, a  "Unified Who Column". The label “Who” I find a bit strange but it is reasonably easy to understand and I don't have a better suggestion.

If I understand the unified who argument; the idea is to display the sender if it is received mail and the recipient if it is sent mail. Correct? 

These are the first two items to resolve. 

Deciding what the default columns should be for each type of folder can then be decided over time and prior to the ultimate release of TB3. In the mean time the flexibility of user choice should be restored.

Peter
(In reply to comment #22)
> If I understand the unified who argument; the idea is to display the sender if
> it is received mail and the recipient if it is sent mail. Correct? 

Sorry but I don't know about original feature of Eudora's "Unified Who Column".
For "Unified Who Column" of Penelope(Beta of new Eudora based on Tb):
It's simply (a) label string change of both "From" column and "Recipient" column to "Who", (b) utilizing auto-switch feature from "From" to "Recipient" if Sent type folder, (c) without removing "From"(new "Who") from column selection list for Sent type folder, (d) without removing "Recipient"(new "Who") from column selection list for Inbox type folder.
You did both (c) & (d). Developers of Penelope didn't do (c) & (d), so Bug 400090 is exposed.
Because "auto-switch to Recipient if Sent type folder" can do nothing for folder which has both sent & received mails, I proposed (Stage-2) in my comment #21.
I agree with Peter's suggested order of importance - critical to usability is getting the old choice back.

Regarding Penelope - that must have been trying to emulate Eudora's "Who" column which correctly did the switch on a per-message basis, and not succeeding. 

- Mitra
I'm a little befuddled by this concept of "Sent type folder" and "Inbox type folder".

The introduction of the Archives folder completely breaks this concept.  And it was already an invalid categorization for the Trash folder, and for every folder when the setting "Place replies in the folder of the message being replied to" is enabled.

The concept of In or Out for mail folders simple doesn't make sense anymore.

A Unified "Who" field might be better served showing *both* Sender->Recipent, similarly to the way Outlook does this (eg: a From/To column).

Personally, I'm really uncomfortable with TB trying to best-guess which of two columns it should preset/present, when by nature of the problem defies a single selection.  BOTH are more valuable and *always* correct.

It will be interesting to see how this works out.
Whiteboard: [needs input/ui-review clarkbw; see comment 11] → [needs proposed solution/patch iteration asuth, ui-review, review]
Duplicate of this bug: 498286
I notice the AddOn ShowInOut has now been updated and works on the nightly's, and gives us the unified Who column (it calls it "Correspondant") so if not being able to see the Recipient is making TB unusable for you ... (as it is for me)

https://addons.mozilla.org/en-US/thunderbird/addon/3492
For those of us who don't mix in and out in the same folder, and find Thunderbird unusable for the lack of seeing the recipient (because of multiple possible destinations filtered to the same folder) this extension doesn't help any, because it shows the same thing as the existing single column Shredder is showing now.
I was able to get the behavior I need by modifying this extension, however. :)

--- content/showInOut.js.orig   2009-07-11 21:43:34.000000000 -0400
+++ content/showInOut.js        2009-07-13 03:45:32.000000000 -0400
@@ -297,7 +297,7 @@
     // get all correspondent and store
     var corr=new Object;
     var emails = received=='in' || newsgroup ?
-        msgHdr.mime2DecodedAuthor: msgHdr.mime2DecodedRecipients;
+        msgHdr.mime2DecodedRecipients: msgHdr.mime2DecodedAuthor;
     var addrs={};
     var names={};
     var full={};
Yes I agree with Dave. For a single folder for mail in and out this could work because the “From” column will show the sender whether it is one of your email addresses or the name of the person who has sent mail to you.  The “Correspondent” column  always shows the name of the other person. That is to say it does not show your  “sender” or who it was sent to.

In my case this does not help because I have multiple users in an IMAP system that all use the same email address. With the old system we could generally see the person whom the email was addressed because each user has his own name tagged to the address. This name is picked up either from the “Your name” field or from the senders Address book. For example mail addressed to  “Fred Smith <info@xyzxyz.com>” will appear in the “Recipient” column as addressed to Fred Smith. When Fred sends a mail or replies his name will be shown in the “From” column in the Sent folder.

I agree with MrC   

(In reply to comment #25)
> Personally, I'm really uncomfortable with TB trying to best-guess which of two
> columns it should preset/present, when by nature of the problem defies a single
> selection.  BOTH are more valuable and *always* correct.

Sorry to be so long winded about this. I am trying to be as precise and as clear as I can.

Peter
(In reply to comment #29)
> I was able to get the behavior I need by modifying this extension, however. :)

Thanks Dave. I tried it but it is still not a complete solution and does not have the simplicity of allowing both "From" and "Recipient" in all folders.

Peter
Only feature not mentioned in comment 11 is that that we persist the order of columns too.

We no longer try and forbid columns that might not make sense.  The notable exception is the gloda search columns are never visible in the picker, but that's a correctness issue, not a relevancy issue.

We do not persist column widths with the folder; the width persistence is still reliant on XUL (session) persistence.  I feel that a naive implementation would be more frustrating than helpful given the number of 'flexible' things that could impact column widths and cause the persisted values to be annoyingly stale.  Namely, the window could be resized, the sidebar could be resized, etc.  This could likely be mitigated (in a future enhancement) by persisting percentages or something that is a weight rather than a pixel width.  (Maybe that's how XUL would handle it anyways?)
Attachment #388228 - Flags: review?(sid.bugzilla)
Comment on attachment 388228 [details] [diff] [review]
v1 fix as proposed in comment 11 with unit test

dmose, please sign-off on the preference change.  I think this may be covered by the waiver for the folder display "sub-module", but given that the preference was used for dubious purposes (doubling as a thunderbird version indicator), it would be nice.
Attachment #388228 - Flags: superreview?(dmose)
Whiteboard: [needs proposed solution/patch iteration asuth, ui-review, review] → [needs review sid0, sr dmose, ui-r clarkbw]
Attachment #388228 - Flags: ui-review?(clarkbw)
First pass review -- I still need to run with the patch.

Rich review at <http://reviews.visophyte.org/r/388228/>

on file: mail/base/content/folderDisplay.js line 314
>    * If a column does not have a function, it is assumed to be legal for display

s/it is assumed to be legal for display/it is assumed that it should be
displayed by default/


on file: mail/base/content/folderDisplay.js line 373
>   _persistColumnStates: function FolderDisplayWidget__persistColumnStates(aState) {
>     let state = aState ? aState : this._saveColumnStates({});
>     if (!this.view.displayedFolder || !this.view.displayedFolder.msgDatabase)
>       return;
>     let dbFolderInfo = this.view.displayedFolder.msgDatabase.dBFolderInfo;
>     dbFolderInfo.setCharProperty(this.PERSISTED_COLUMN_PROPERTY_NAME,
>                                  JSON.stringify(state));
>     this.view.displayedFolder.msgDatabase.Commit(
>       Components.interfaces.nsMsgDBCommitType.kSmallCommit);
>   },

I don't see a reason for msgDatabase not to be cached.


on file: mail/base/content/folderDisplay.js line 451
>     // if we are still here, use the defaults and helper functions
>     let state = {};
>     for (let [, colId] in Iterator(this.DEFAULT_COLUMNS)) {
>       let shouldShowColumn = true;
>       if (colId in this.COLUMN_DEFAULT_TESTERS) {
>         try {
>           shouldShowColumn = this.COLUMN_DEFAULT_TESTERS[colId](this.view);
>         }
>         catch (ex) {
>           Components.utils.reportError(ex);

Any reason this is in a try/catch block?


on file: mail/base/content/folderDisplay.js line 474
>   /**
>    * Set the column states of the current FolderDisplay to the provided state.
>    *

We're also handling inactive folder displays here, so s/current/this/ seems
like a good choice.


on file: mail/base/content/folderDisplay.js line 535
>   /**
>    * A dictionary that maps column ids to dictionaries where each dictionary
>    *  has the following fields:
>    * - id: The column id of the column.

No need for the |id| property.


on file: mail/base/content/folderDisplay.js line 573
>     let columnStates = aState || {};

This is ugly, and I don't see any callers that actually use aState. Why not
get rid of _saveColumnStates and make it live up to its name, and have an
additional _getColumnStates that returns the states? _getColumnStates looks
like useful information anyway, so you could make it public. You could also
handle inactive folder displays by returning _savedColumnStates.


on file: mail/base/content/folderDisplay.js line None

Hooray! The great thing about deleting code is that it doesn't even show up in
blame.


on file: mail/base/content/mailWindowOverlay.js line 1923
>       desiredColumns: {
>         flaggedCol: {
>           id: "flaggedCol",
>           visible: true,
>         },
>         subjectCol: {
>           id: "subjectCol",
>           visible: true,
>         },
>         senderCol: {
>           id: "senderCol",
>           visible: true,
>         },
>         dateCol: {
>           id: "dateCol",
>           visible: true,
>         },

No need for |id| here either.

on file: mail/base/content/mailWindowOverlay.js line 1971
>         aTab.folderDisplay.setColumnStates(aTab.mode.desiredColumnStates);

You want aTab.mode.desiredColumns here.

on file: mail/base/content/messageWindow.js line 161
>   // we don't care about columns.
>   setColumnStates: function () {},
>   _depersistColumnStateFromDbFolderInfo: function () { return null; },
>   _getDefaultColumnsForCurrentFolder: function () { return null; },
>   _persistColumnStates: function () {},
> 

Although I don't see any callers right now that haven't already been
overridden, I think we also want to override _saveColumnStates and
_restoreColumnStates for correctness.


on file: mail/base/content/msgMail3PaneWindow.js line 736
>     dump("UpgradeThreadPane: ex = " + ex + "\n");

Please change the name of the function here, or better yet use Cu.reportError.


on file: mail/test/mozmill/folder-display/test-columns.js line 96
> function show_column(aColumnId) {
>   mc.e(aColumnId).removeAttribute("hidden");
> }

The |hidden| property should work here.


on file: mail/test/mozmill/folder-display/test-columns.js line 105
> function hide_column(aColumnId) {
>   mc.e(aColumnId).setAttribute("hidden", "true");
> }

Same here.


on file: mail/test/mozmill/folder-display/test-columns.js line 139
> dump("INBOX: " + folderInbox +"\n");
> if (folderInbox)
>   dump("blah blah: " + folderInbox.prettyName + "\n");

If you'd like to keep the dump around, please fix up the indentation and use
something more descriptive than "blah blah" ;)


on file: mail/test/mozmill/folder-display/test-columns.js line 142
>   enter_folder(folderInbox);
>   mc.sleep(2000);

enter_folder already wait[s]_for_all_messages_to_load, so do we really need to
sleep?
(In reply to comment #34)
> on file: mail/base/content/folderDisplay.js line 451
> >     // if we are still here, use the defaults and helper functions
> >     let state = {};
> >     for (let [, colId] in Iterator(this.DEFAULT_COLUMNS)) {
> >       let shouldShowColumn = true;
> >       if (colId in this.COLUMN_DEFAULT_TESTERS) {
> >         try {
> >           shouldShowColumn = this.COLUMN_DEFAULT_TESTERS[colId](this.view);
> >         }
> >         catch (ex) {
> >           Components.utils.reportError(ex);
> 
> Any reason this is in a try/catch block?

The default columns setup and the testers are meant to be half of a more fully developed extension point for custom columns.  I've added a comment to this effect and made it force shouldShowColumn to false if an exception happens.


> on file: mail/base/content/folderDisplay.js line 573
> >     let columnStates = aState || {};
> 
> This is ugly, and I don't see any callers that actually use aState. Why not
> get rid of _saveColumnStates and make it live up to its name, and have an
> additional _getColumnStates that returns the states? _getColumnStates looks
> like useful information anyway, so you could make it public. You could also
> handle inactive folder displays by returning _savedColumnStates.

It does get used by _persistColumnStates.  The fundamental messiness comes from us having two real cases where we persist things:

1) The FolderDisplayWidget is active and the user modified the columns by hiding/reordering and we want to persist that.

2) The user entered a folder that did not have any persisted column state so we want to load and persist the appropriate defaults for that folder.  Because this can happen in the background, we want to explicitly pass in that default state we created rather than use the active column state (since that can correspond to somebody else in the foreground).

I agree that this is much uglier than it should be.  I'll take a whack at simplifying the tangled web given that there are really only two callers.


> on file: mail/test/mozmill/folder-display/test-columns.js line 96
> > function show_column(aColumnId) {
> >   mc.e(aColumnId).removeAttribute("hidden");
> > }
> 
> The |hidden| property should work here.

I've had to deal with a number of startup related bugs in our UI that are a result of XBL bindings not getting bound until they are displayed.  No XBL bindings, no properties.  It doesn't make me happy, but it's much safer to just directly mess with the attributes, especially if the de-multiplexing results in the introduction of more dynamically created XBL-bound elements.
 

Everything else I've already fixed locally.  Going to try and de-ugly the persistence now.
Revised patch with requested changes made.

I'm losing the clarkbw ui-review request because he already signed off on the strategy in comment 14 (and some verbal discussion).  Additional clarkbw feedback is of course always appreciated.
Attachment #388228 - Attachment is obsolete: true
Attachment #388360 - Flags: superreview?(dmose)
Attachment #388360 - Flags: review?(sid.bugzilla)
Attachment #388228 - Flags: ui-review?(clarkbw)
Attachment #388228 - Flags: superreview?(dmose)
Attachment #388228 - Flags: review?(sid.bugzilla)
Whiteboard: [needs review sid0, sr dmose, ui-r clarkbw] → [needs review sid0, sr dmose]
Comment on attachment 388360 [details] [diff] [review]
v2 fix as proposed in comment 11 with unit test
[Checkin: Comment 42]

> 
>+  // we don't care about columns.
>+  setColumnStates: function () {},
>+  _depersistColumnStateFromDbFolderInfo: function () { return null; },
>+  _getDefaultColumnsForCurrentFolder: function () { return null; },
>+  _persistColumnStates: function () {},
>+  _saveColumnStates: function () {},
>+  _restoreColumnStates: function() {},
>+

Please override getColumnStates here with a function that returns null, and please update the documentation for the real getColumnStates to tell callers to be prepared for null.

r- because the following tests fail for me, and I can easily reproduce the failures by hand:
test_column_visibility_persists_through_tab_changes
test_column_visibility_persists_through_folder_changes
test_column_reordering_persists
Attachment #388360 - Flags: review?(sid.bugzilla) → review-
(In reply to comment #37)
> test_column_visibility_persists_through_folder_changes
> test_column_reordering_persists

Actually, these two are probably due to the blowup caused by the tab change test failing. I can reproduce the tab change failure, though, both with folder tabs and with gloda search tabs.
(In reply to comment #37)
> r- because the following tests fail for me, and I can easily reproduce the
> failures by hand:
> test_column_visibility_persists_through_tab_changes

The reason this is failing is that in FolderDisplayWidget.makeInactive, you're saving the column states after setting this._active to false. Saving column states before setting this._active to false allows things to work again.
Comment on attachment 388360 [details] [diff] [review]
v2 fix as proposed in comment 11 with unit test
[Checkin: Comment 42]

r+, assuming
1. the bug is fixed
2. a comment is added explaining why we do what we do
3. the fix is reviewed.
Attachment #388360 - Flags: review- → review+
Whiteboard: [needs review sid0, sr dmose] → [needs sr dmose]
Whiteboard: [needs sr dmose] → [needs updated patch, sr Standard8]
Attachment #388360 - Flags: superreview?(dmose) → superreview?(bugzilla)
Comment on attachment 388360 [details] [diff] [review]
v2 fix as proposed in comment 11 with unit test
[Checkin: Comment 42]

sr=Standard8 on the mailnews/ changes.
Attachment #388360 - Flags: superreview?(bugzilla) → superreview+
Whiteboard: [needs updated patch, sr Standard8] → [needs updated patch + review of changes]
pushed: http://hg.mozilla.org/comm-central/rev/c7415baf5df0

I had philor review the fix interdiff-style; I am assuming that's what you meant so that this could land before the freeze.

Also, my bad for making a structural changes and not re-running the unit tests.

In terms of the dummy functions, I introduced getColumnStates and just had it return an empty object.  I also updated the other return-dummies to do the same.  No point complicating callers with code that should never actually run/be called.  (And if someone does call it they will get something valid, although they may end up turning off all their columns...)
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Thanks guys for getting this sorted out. I have just tried the latest update of shreder and everything seems to be fine.

Two observations: 

I note that you have decided to make new folders inherit the properties of the inbox if in "Local Folders" or, if a subfolder, to inherit the properties of the parent folder. I think this is a good solution.

I see that column widths are universal and agree with someone who said it would be nice to be able to customise them to the individual folders

Thanks again
Peter
(In reply to comment #43)
> I note that you have decided to make new folders inherit the properties of the
> inbox if in "Local Folders" or, if a subfolder, to inherit the properties of
> the parent folder. I think this is a good solution.

We do not inherit from the parent folder, just the Inbox.  My assumption was that in most cases only 'leaf' folders would contain messages, and their parents would exist only for place-holding.  For example, in the Archives folder, I would not expect the 'root' Archives folders to have meaningful columns configured, even if your storage mechanism supports it having messages in it.  (Many IMAP situations do not allow folders that have messages to have sub-folders.)

I am not opposed to inheriting from the parent folder, I'm just concerned that it would make things less intuitive and less likely to do something reasonable.  I'd suggest proposing a very explicit strategy with use-cases on mdat ( http://groups.google.com/group/mozilla.dev.apps.thunderbird/topics ) rather than trying to have the discussion on a bug.
 
> I see that column widths are universal and agree with someone who said it would
> be nice to be able to customise them to the individual folders

I'm going to implement a prototype extension that provides this functionality as a basis for people to hack on in order to make testable proposals.
What I had hoped to see was News Accounts having a set Col once and then all groups inheriting. I had to go through and reset every group to restore my Col configuration that existed prior to the patch landing.
I also had to go through and individually reset all of mine after this patch landed, too.  Basically, any mailbox whose column list I had already customized failed to inherit from Inbox, and I could find no way to "undo" the customization so that it would inherit from the Inbox again.
I would have thought that the "Restore defaults" should make it inherit from the Inbox again. At least that would be logical. Perhaps if something is not functioning here it needs to be reported as a new bug.

As we are testing Shredder perhaps previous versions of Shredder have affected settings in such a way that unexpected results occur. It needs to be tested as for a new user or someone upgrading from TB 2. Does it not?

Peter
(In reply to comment #46)
> I also had to go through and individually reset all of mine after this patch
> landed, too.  Basically, any mailbox whose column list I had already customized
> failed to inherit from Inbox, and I could find no way to "undo" the
> customization so that it would inherit from the Inbox again.

Prior to the landing of the patch on this bug, there was no per-folder persistence of columns.  There was a simple global persistence of columns that persisted 1) what columns were visible and 2) whether special columns like Location were visible when they were legal.  When you changed folders, some longstanding heuristics would make some minor changes while benefiting from #2.  When bug 474701 landed we lost #2 because it was wacky in the face of tabs (and accordingly new, stronger heuristics were brought into play as a stop-gap measure.)

As soon as you ran a build with the patch, any time a folder was loaded for display that did not already have persisted columns, the set of columns would be initialized to default values (and persisted).  In the case of plain vanilla folders, these defaults would come from the current state of the Inbox for the account the folders belong to.

As such, I'm not sure what you mean when you say "any mailbox whose column list I had already customized."  Did you click around on folders to see what their state was before initially customizing the Inbox to your liking?  If you immediately customized the Inbox once the patch landed, unless the folders you visited were not subject to inheritance from the Inbox, the situation you are describing suggests a bug or deficiency in the implementation.  Please elaborate on the type of folders (and account) with suspect behavior and whether the Inbox was customized prior to first visiting after the landing of the patch.
(In reply to comment #47)
> I would have thought that the "Restore defaults" should make it inherit from
> the Inbox again. At least that would be logical. Perhaps if something is not
> functioning here it needs to be reported as a new bug.

I totally blanked on the existence of this option in the column picker.  Please feel free to log a bug dependent on this bug that proposes doing as you say.  It's still fairly inconvenient to have to do that for multiple folders, but it certainly does make a lot more sense for the option to do as you propose.

> As we are testing Shredder perhaps previous versions of Shredder have affected
> settings in such a way that unexpected results occur. It needs to be tested as
> for a new user or someone upgrading from TB 2. Does it not?

No, this patch ignores all persistence settings prior to the landing of this patch.  An upgraded profile is the same as a new profile.

The code for the unit tests for this functionality can be found here if you are interested in what we actually test:
http://mxr.mozilla.org/comm-central/source/mail/test/mozmill/folder-display/test-columns.js
The per-mailbox persistence seems to have been in place before this patch landed.  In fact, they started doing per-mailbox persistence about the same time the Recipient column disappeared as a choice.  Any mailbox that I screwed with trying to get the Recipient column back before *this* patch landed suffered from the mentioned problem of being stuck that way and having to have it manually added.  The mailboxes I hadn't touched before had the Recipient column back in them again (just like my Inbox, which was the first one I fixed after this landed) when first opened.  Most of the folders I was messing with are in an IMAP account.  Note that two or three days before this patch landed I installed ShowInOut and hacked it, as noted in comment 27 through comment 29.  Whether than had any effect on it I don't know.
Blocks: 504989
(In reply to comment #49)
> I totally blanked on the existence of this option in the column picker.
> Please feel free to log a bug dependent on this bug that proposes doing as
> you say. It's still fairly inconvenient to have to do that for multiple
> folders, but it certainly does make a lot more sense for the option to do as
> you propose.

bug 504989 filed.
No longer blocks: 504989
Depends on: 504989
1) It's hard to find this bug because the bug title doesn't reflect the major focus of the patch.

2) It's not very nice to make everyone manually hide/show the desired columns for all of our RSS feeds, one at a time.  That was very painful and time-wasting.  If you're going to wipe out our previous settings, then please at least let RSS mailboxes default to the primary email account's Inbox settings.  That's better than your "appropriate defaults".  They might be appropriate for you but not for everyone.

3) I think there should be a warning in the release notes that users must choose the columns in the Inbox before they visit any other mailboxes.
(In reply to comment #52)
> 2) It's not very nice to make everyone manually hide/show the desired columns
> for all of our RSS feeds, one at a time.  That was very painful and

I agree.  The solution for RSS and Newsgroups is less than ideal.  Please open a new bug on that use case.  The suggestion of using the default email account is a good one; it is a definite rule that avoids us having to randomly choose one.  (Albeit not entirely obvious; I had to go to the account manager to see the "Set as Default" button to be sure we exposed such a thing in the UI.)

Although that options sounds good for the newsgroup case, perhaps for the RSS case we would just want a better set of (specialized) defaults?
Whiteboard: [needs updated patch + review of changes]
(In reply to comment #53)
> Please open a new bug on that use case.

Thanks, I created bug 510475.
Attachment #388360 - Attachment description: v2 fix as proposed in comment 11 with unit test → v2 fix as proposed in comment 11 with unit test [Checkin: Comment 42]
You need to log in before you can comment on or make changes to this bug.