562266 - "Apply columns to..." should honor special folders

Status: RESOLVED FIXED

(Thunderbird :: Folder and Message Lists, defect)

Description

[raoul bhatia]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US; rv:1.9.2.3) Gecko/20100401 Firefox/3.6.3 (.NET CLR 3.5.30729)
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US; rv:1.9.2.5pre) Gecko/20100426 Lanikai/3.1b2pre

Bug 505035 has been addressed and things seem to work quite well.
one thing i would like to point out:

1. reset the Inbox columns
2. add the size column
3. propagate this settings through the whole account.
4. switch to sent folder
5. see that "Recipient:" is gone and "From:" has appeared.

i, somehow, would expect this not to happen. a) it should either be *default* 
size for all folders (so sent would contain Recpient: instead of From:) or the 
b) sent-folder should always *replace* From: with Recipient.

i hope i could clarify the small difference.
a) takes the default columns and records the modifications, just like a "diff"
b) stupidly replaces From: with Recipient:

what do you think?

Thanks,
Raoul

Reproducible: Always

Comment 1

[Andrew Sutherland [:asuth] (he/him)]

Thank you for filing the new bug.

For triage purposes, this is considered a low priority but if someone wants to provide a patch with unit tests it would be likely be accepted.

Comment 2

[Ben Bucksch (:BenB)]

I vote for option b) above, because it would work better after bug 36489 was fixed and is the default.

Comment 3

[Rimas Kudelis [use needinfo to ping]]

I too favor option b). The reason is pretty simple: I think that's what I would expect after applying certain column set to other columns.

Furthermore, if we were to always apply only a diff of added non-default and removed default columns, there wouldn't be a way to just drop certain non-default column from being displayed in all folders. So, option a) seems really blurry to me.

Comment 4

[Charles]

I agree, option b is best...

Also - and maybe this is what is implied by the 'special folders' reference in the bug summary, but I just wanted to state it explicitly:

This should apply not only to the Sent folder, but also to the Drafts and Templates folders.

Comment 5

[Charles]

(In reply to comment #1)
> Thank you for filing the new bug.
> 
> For triage purposes, this is considered a low priority but if someone wants to
> provide a patch with unit tests it would be likely be accepted.

Andrew - this is actually a major pain for those of us with a lot of accounts, so, I'd appreciate this being treated a *little* better than just 'low priority' (which usually means it will never be fixed).

I have 16 accounts (right now, I had more before) - so, if I change my defaults on all of my accounts (which I do every time for new installations), I have to then go back and change these 3 folders (Sent, Drafts and Templates) for all 16 accounts - so, in my case, that is 3x16=48 separate changes I have to make to fix all of these folders.

Comment 6

[Charles]

Also, for anyone else with lots of accounts, I just opened a related enhance ment request - bug 579888 - to enhance the 'Apply columns to' feature by adding a 'All Folder in All Accounts' function.

Comment 7

[Charles]

One more minor nit... The 'Trash' folder should show *both* (From and Recipient)...

Comment 8

[Jim Porter (:squib)]

Attachment: Part 1: cleanup

Comment 9

[Jim Porter (:squib)]

Attachment: Part 2: the fix

This probably needs a test, but it works for me.

Comment 10

[Charles]

Awesome to see this getting fixed!

As for my last comment about the Trash needing both - that would now be a good candidate for just getting 'Correspondents'.

thanks Jim!

Comment 11

[:aceman]

Comment on attachment 8747482 [details] [diff] [review]
Part 2: the fix

Review of attachment 8747482 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks, works for me.

::: mail/base/content/threadPaneColumnPicker.xml
@@ +150,1 @@
>            let propName = gFolderDisplay.PERSISTED_COLUMN_PROPERTY_NAME;

Please move this below where it is used now.

@@ +169,5 @@
> +
> +          let amIOutgoing = isOutgoing(gFolderDisplay.displayedFolder);
> +          function colStateString(folder) {
> +            return isOutgoing(folder) == amIOutgoing ? myColStateString :
> +              swappedColStateString;

Please add some parentheses to make this readable.

::: mail/base/modules/MailUtils.js
@@ +352,5 @@
>      function folder_string_setter_worker() {
>        for (let folder in fixIterator(allFolders, Ci.nsIMsgFolder)) {
>          // set the property; this may open the database...
> +        let value = typeof aPropertyValue == "function" ?
> +          aPropertyValue(folder) : aPropertyValue;

Please add some parentheses to make this readable.

Comment 12

[Jim Porter (:squib)]

Attachment: Part 2 v2: the fix (with tests!)

I added tests and fixed the review comments.

Comment 13

[Jim Porter (:squib)]

Attachment: Part 2 v3: the fix (with tests!)

Whoops, I missed one review comment.

Comment 14

[:aceman]

Comment on attachment 8748002 [details] [diff] [review]
Part 2 v3: the fix (with tests!)

Review of attachment 8748002 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks.

Comment 15

[:aceman]

Ready for checkin?

Is this intended for TB45.x to help a small bit the users needing to revert their folder columns away from Correspondents? :)

Comment 16

[Jim Porter (:squib)]

I plan to request uplift to 45, but I'm not convinced either way whether it should be. I do know that I want this and some other patches to land before we try to upgrade to Correspondents in the future. (I still think the Correspondents upgrade is valuable, but we did it prematurely and handled a number of subtle issues pretty poorly.)

Comment 17

[Jim Porter (:squib)]

Comment on attachment 8748002 [details] [diff] [review]
Part 2 v3: the fix (with tests!)

Richard: I've made one UX change here. When you click "Apply columns to..." in the thread pane, it checks whether the source and target folders are outgoing. If they're both (or neither) outgoing, the behavior is the same as it used to be. If one is outgoing and the other is incoming, we check the columns to be applied. If the user has From or Recipient in the source (but not both!) we swap them before applying: From becomes Recipient and vice versa.

Hopefully that explains things.

Comment 18

[Richard Marti (:Paenglab)]

Comment on attachment 8748002 [details] [diff] [review]
Part 2 v3: the fix (with tests!)

Very well explained. It works great and can help revert the correspondent changes in one click. Thank you for this change!

Comment 19

[Jorg K (CEST = GMT+2)]

Sorry, bug 1271108 most likely made your patch rot with respect to test-columns.js. Should be an easy rebase.

Comment 20

[:aceman]

Attachment: Part 1: cleanup v2

Added proper patch headers.

Comment 21

[:aceman]

Attachment: Part 2: the fix (with tests!) v4

Added patch headers and unbitrotted the test.
Also fixed a copy&paste error
 test_apply_to_folder_and_children.EXCLUDED_PLATFORMS = ["linux"];
to
 test_apply_to_folder_and_children_swapped.EXCLUDED_PLATFORMS = ["linux"];

I will now push these patches. Successful try run:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=e3c8ddd38e4c3c60673d516a87905eb6af99ea8d

Comment 22

[:aceman]

https://hg.mozilla.org/comm-central/rev/d33cefe42224d92fec17855790b0a792008cc973
https://hg.mozilla.org/comm-central/rev/605025ff974d79d113a4aca07090ef6ea8799df6

Comment 23

[Charles]

Really glad to see this getting fixed.

Any chance for back-porting to 45?

Comment 24

[Wayne Mery (:wsmwk)]

(In reply to Charles from comment #23)
> Really glad to see this getting fixed.
> 
> Any chance for back-porting to 45?

is already asked and answered in previous comments

Comment 25

[Charles]

Sorry, missed it in  the *one* comment where it was mentioned... thanks, but until someone ads the flag, it hasn't even been requested, right?

Just noticed I can add the flag, so I'm doing it now, but I'm not sure - I think you start the process by setting the tracking flag?

Comment 26

[Jim Porter (:squib)]

Having thought about it more since comment 16, I'd lean strongly towards *not* uplifting this. This is a new feature, and I don't think we should be shipping new features outside of our normal feature releases. (We could make an argument for allowing new features during minor releases, but that's a significant change in policy, and I don't think this bug is strong evidence that we should make that change.)

Comment 27

[:aceman]

It was only proposed to uplift this because it may help users to recover from the unexpected correspondents columns change in 45.
Otherwise as a new feature it would not apply for uplift.

Comment 28

[Jim Porter (:squib)]

Yeah. My main worry is that there may be subtle issues that we haven't anticipated that could cause problems for users. I'd rather let this at least get to beta without uplifting so we have a chance to let people test this out.

Comment 29

[Wayne Mery (:wsmwk)]

(In reply to :aceman from comment #27)
> It was only proposed to uplift this because it may help users to recover
> from the unexpected correspondents columns change in 45.
> Otherwise as a new feature it would not apply for uplift.

Actually, it might even if it didn't help with a prior regression - because over the past year we have been uplifting some feature changes into esr. but ....

(In reply to Jim Porter (:squib) from comment #28)
> Yeah. My main worry is that there may be subtle issues that we haven't
> anticipated that could cause problems for users. I'd rather let this at
> least get to beta without uplifting so we have a chance to let people test
> this out.

+1

Comment 30

[Kent James (:rkent)]

Comment on attachment 8756421 [details] [diff] [review]
Part 1: cleanup v2

Per comment 28 let's land this in beta47

Comment 31

[Jorg K (CEST = GMT+2)]

https://hg.mozilla.org/releases/comm-beta/rev/b99aed4b20bf
https://hg.mozilla.org/releases/comm-beta/rev/f157ee767a16

Comment 32

[Jorg K (CEST = GMT+2)]

https://hg.mozilla.org/releases/comm-aurora/rev/d76850acd598
https://hg.mozilla.org/releases/comm-aurora/rev/48057fe33922

Comment 33

[Kent James (:rkent)]

Given that the patch author recommended against uplift, it would take a strong argument to uplift this to esr45. Let's skip it for 45.3

Comment 34

[Jim Porter (:squib)]

I'd be ok with uplifting this once it went through the full set of trains, so to speak. Since this landed in 49, that would mean uplifting for 45.4.

However, I don't think we have a clear policy on what new features we can land in minor releases, and this might be a sign we need to think about that. I'm totally ok with shipping new things in minor releases provided we agree on what's ok to ship (e.g. small additions that don't break any workflows). Without guidelines though, I think we should avoid making special cases if we can help it.

Comment 35

[Charles]

I would only point out that Thunderbird only gets new major releases once per year - ie, only gets 'minor releases' for an entire year (the time between 'ESR' releases)...

For that reason, I do believe that some kind of formal policy that allows for the inclusion of new features should be developed, that has a much lower bar than, say, Firefox. And one of those bars should definitely be the developer who provided  the code for the new feature be willing to stick around and fix and unexpected problems it may cause.

Comment 36

[Kent James (:rkent)]

Decision time for tb 45.4.0 I don't have a strong opinion myself on the technical issues.

Comment 34 and comment 35 suggested that this patch would be a candidate for uplift in conjunction with a newly defined policy on adding features in point releases, but such a new policy has not been done.

I think I would view this bug differently. It is really about mitigating the changes associated with the Correspondents column, which are changes that we already been backporting into TB 45. So while I think a policy would be great, I don't think we need consider this bug a precedent, it is merely part of an overall mitigation strategy in TB 45 for issues associated with the Correspondents column. So I am leaning toward uplifting.

Any more comments?

Comment 37

[Wayne Mery (:wsmwk)]

I agree with the rationale of this helping with mitigation

Comment 38

[Jorg K (CEST = GMT+2)]

(In reply to Wayne Mery (:wsmwk, NI for questions) from comment #37)
> I agree with the rationale of this helping with mitigation

(In reply to Kent James (:rkent) from comment #36)
> Any more comments?
I have more comments: Much too late, sadly.
The damage was done shipping the aggressive correspondents column in TB 45.0, practically withdrawing it again in TB 45.1, shipping more fixes in TB 45.1.1 (gloda columns) and TB 45.3 (advanced search columns) and now shipping a *key* feature to undo the damage in TB 45.4. By all means, ship it, but to undo the damage that's already done, it's far too (xxx) late, mate - and in Australia they would add a word starting with "f" or "b" where indicated :-(

Comment 39

[Jim Porter (:squib)]

On the contrary, I'd consider the release of the Correspondents column much too early, and this is right on time. ;)

I don't have a problem with releasing this, but I still think it's worth discussing how to fit changes like this (small new features that we think are important an non-disruptive) into point releases. This may also require us to look at how to handle localization in point releases; luckily, this patch doesn't require strings, but some similar changes in the future might.

Comment 40

[Kent James (:rkent)]

The "Ayes" have it.

http://hg.mozilla.org/releases/comm-esr45/rev/8e986494a5d1
http://hg.mozilla.org/releases/comm-esr45/rev/9ac71cfc4629

Comment 41

[Kent James (:rkent)]

It appears that pushing this patch to esr45 has caused a test failure:

TEST-UNEXPECTED-FAIL | /builds/slave/test/build/tests/mozmill/folder-display/test-columns.js | test-columns.js::test_apply_to_folder_no_children_swapped

Any thoughts? (I'm rerunning the build before this patch as a test).

Comment 42

[Jorg K (CEST = GMT+2)]

Failure:
INFO -  SUMMARY-SKIP | test-columns.js::test_apply_to_folder_and_children
INFO -  SUMMARY-UNEXPECTED-FAIL | test-columns.js | test-columns.js::test_apply_to_folder_no_children_swapped
INFO -    EXCEPTION: Found visible column 'recipientCol' but was expecting 'senderCol'!
INFO -  desired list: threadCol,flaggedCol,attachmentCol,subjectCol,unreadButtonColHeader,senderCol,junkStatusCol,dateCol
INFO -   actual list: threadCol,flaggedCol,attachmentCol,subjectCol,unreadButtonColHeader,recipientCol,junkStatusCol,dateCol
INFO -      at: test-columns.js line 116
INFO -         assert_visible_columns test-columns.js:116 1
INFO -         test_apply_to_folder_no_children_swapped test-columns.js:505 3

Thoughts:
1) Strange that it fails on ESR45 and not elsewhere.
2) I compared test-columns.js on c-esr45 and c-c and they are the same apart from a Linux exclusion.
   Thus: It needs more looking into ;-) Perhaps Jim can enlighten us.

Comment 43

[Kent James (:rkent)]

I'd really like to start 45.4.0 and this is currently the only issue. Unless someone has a good idea in the next couple of hours, I'll need to back this out from comm-esr45 for 45.4.0

Comment 44

[Jorg K (CEST = GMT+2)]

I wish I could help you here, but I can't. Looking again at the test, here is what it tries to do:
 * Change settings in an incoming folder, apply them to an outgoing folder that
 * also has children. Make sure the folder changes but the children do not.

It fails on line 505:

     // But not the children.
     be_in_folder(folderChild1);
505  assert_visible_columns(INBOX_DEFAULTS);

So the test wants to check that the columns for the children haven't changed, but in fact they have changed since a 'recipientCol' is found which is not in the default for an inbox, there we should have a 'senderCol' (Sender=From).

So as far as I can see the test has detected a real functional failure and there is no option but to back out.

Comment 45

[Jorg K (CEST = GMT+2)]

OK, Kent, can you run the test locally? From the object directory run:
mozmake SOLO_TEST=folder-display/test-columns.js mozmill-one

Before you do change line 490 from
hide_column("correspondentCol");
to
hide_column(useCorrespondent ? "correspondentCol" : "senderCol");
or
hide_column(useCorrespondent ? "correspondentCol" : "recipientCol");
Try both.

Remember, TB 45 doesn't have the correspondent column and this test landed on a version which does so the test doesn't work when there is no correspondent column.

I don't have a local build of ESR45.

Comment 46

[Jorg K (CEST = GMT+2)]

And there is another one of those in line 526.

Comment 47

[Jim Porter (:squib)]

Yeah, that's probably the issue. Nightly (and eventually 52) will default to the correspondent column, right? But not in 45.

Comment 48

[Kent James (:rkent)]

Backout out from comm-esr45 due to test failures.
http://hg.mozilla.org/releases/comm-esr45/rev/a3f6eea63189

But with more recent information I may backout the backout. Testing locally.

Comment 49

[:aceman]

Should it NOT matter whether correspondents is shown or not thanks to bug 1271108 ? Was that incomplete and also needs changes in comment 45?

Comment 50

[Jim Porter (:squib)]

Comment on attachment 8756425 [details] [diff] [review]
Part 2: the fix (with tests!) v4

Review of attachment 8756425 [details] [diff] [review]:
-----------------------------------------------------------------

It matters because of the two lines below.

::: mail/test/mozmill/folder-display/test-columns.js
@@ +486,5 @@
> +
> +  // permute!
> +  let conExtra = [...INBOX_DEFAULTS];
> +  conExtra[5] = "senderCol";
> +  hide_column("correspondentCol");

This line.

@@ +522,5 @@
> +
> +  // permute!
> +  let conExtra = [...INBOX_DEFAULTS];
> +  conExtra[5] = "senderCol";
> +  hide_column("correspondentCol");

And this.

Comment 51

[Kent James (:rkent)]

As long as "the test has detected a real functional failure" is false and we believe that the issue is just a failure of the test code, I have no problem starting 45.4.0 and leaving the test broken. I've already done the backout, but we can reland with the test fixed. But that does not need to block 45.4.0

Comment 52

[Kent James (:rkent)]

Sorry for the indecision here, but I think it will be cleaner to unbackout the backout, and build with the known testing issue. It would still be good to get the test fixed for esr45

http://hg.mozilla.org/releases/comm-esr45/rev/e4e46c3b225e

Comment 53

[Jorg K (CEST = GMT+2)]

Jim, are you really sure this works in ESR45 when the correspondents column is not enabled?

On current trunk I set pref("mail.threadpane.use_correspondents", false); and I'm trying to beat the test into shape so it passes, I tried:

  // permute!
  let conExtra = [...INBOX_DEFAULTS];
  if (useCorrespondent) {
    conExtra[5] = "senderCol";
    hide_column("correspondentCol");
    show_column("senderCol");
  } else {
    conExtra[5] = "correspondentCol";
    hide_column("senderCol");
    show_column("correspondentCol");
  }
  assert_visible_columns(conExtra);

  // Apply to the one dude.
  _apply_to_folder_common(false, folderParent);

  // Make sure it copied to the parent.
  let conExtraSwapped = [...INBOX_DEFAULTS];
  conExtraSwapped[5] = useCorrespondent ? "recipientCol" : "correspondentCol";
  be_in_folder(folderParent);
  assert_visible_columns(conExtraSwapped); <<<=== fails here.

It still fails where indicated with:
EXCEPTION: Found visible column 'recipientCol' but was expecting 'senderCol'!
which makes me believe that the child columns were changed although they shouldn't have been changed.

So given this situation, I'm not convinced that the function actually works and I wouldn't be comfortable with shipping this in a state where I don't know whether it's a test failure or a functional failure.

Comment 54

[Jim Porter (:squib)]

(In reply to Jorg K (GMT+2) from comment #53)
> Jim, are you really sure this works in ESR45 when the correspondents column
> is not enabled?

Completely unsure. But there's probably a bug somewhere around the correspondents column.

I don't have any time to look at this in the near future, unfortunately.

Comment 55

[Jorg K (CEST = GMT+2)]

Attachment: Fix test.

The test was basically wrong.

Children of an outgoing folder have "sent" columns not "inbox" columns. Only if you use correspondents, both are the same so you don't notice the wrongness.

I also changed the test to it does something useful if no correspondents are enabled.

Landed on ESR45:
https://hg.mozilla.org/releases/comm-esr45/rev/fffc59d887b8

So I'm ready for a positive post-landing review ;-)

Comment 56

[Jim Porter (:squib)]

Comment on attachment 8795482 [details] [diff] [review]
Fix test.

rs=me

Comment 57

[Jorg K (CEST = GMT+2)]

ESR45 push came out green.

Landed on other branches as well:
C-C: https://hg.mozilla.org/comm-central/rev/d92fbeb3cf5b
C-A: https://hg.mozilla.org/releases/comm-aurora/rev/158aa9c1b839
C-B: https://hg.mozilla.org/releases/comm-beta/rev/eb97031be855