1271108 - Fix test-columns.js to respect "mail.threadpane.use_correspondents", also fix gloda to respect "mail.threadpane.use_correspondents" (oversight from bug 1268325)

Status: RESOLVED FIXED

(Thunderbird :: Folder and Message Lists, defect)

Description

[Jorg K (CEST = GMT+2)]

Various tests which are part of test-columns.js fail if
  pref("mail.threadpane.use_correspondents", false);
in all-thunderbird.js

Seen first on ESR45:
https://treeherder.mozilla.org/#/jobs?repo=comm-esr45&revision=0d1274dffb75cadc88320ebbac7fedcd3492fa04

The problem is that the correspondents column is hard-coded in the test:
Search for "correspondentCol" in test-columns.js.

With the preference set to false, the expected columns don't show up any more as default and the tests fail.

That could be fixed in two ways:
1) set the set of expected columns depending on the preference.
2) set the preference to true so the tests pass.

Comment 1

[Jorg K (CEST = GMT+2)]

The test changed from sender/recipient to correspondent here:
https://hg.mozilla.org/comm-central/diff/8567e72b848b/mail/test/mozmill/folder-display/test-columns.js
as part of bug 36489 comment #152.

So for option
  1) set the set of expected columns depending on the preference
this changeset tells us what the column set needs to be with the preference set to "false".

Of course ESR45 could also just be fixed by reverting the change made in the test.

There are many ways to skin a cat ;-)

Comment 2

[Jorg K (CEST = GMT+2)]

Attachment: Patch to revert columns to sender/recipient in test for ESR45

Comment 3

[Jorg K (CEST = GMT+2)]

Re. comment #1: The test wasn't fixed completely in bug 36489. Neil shipped another fix in bug 1150073.
My suggestion in attachment 8750065 [details] [diff] [review] already is the comprehensive return to sender/recipient and covers both.

Comment 4

[aleth [:aleth]]

Thanks for looking into this. Did you mean to request review?

Comment 5

[Jorg K (CEST = GMT+2)]

Attachment: Proposed solution (v1).

OK, I decided to fix the test properly so it is future-proof and the patch can be applied to all branches instead of just having a quick-and-dirty fix for ESR45 alone.

Sadly, while doing so, I noticed that a bit was missed in bug 1268325. Gloda insisted on the correspondents column since that was hard-coded into mailnews/db/gloda/modules/dbview.js.

In order to avoid yet another bug that would have to revisit the test, I'm including the fix here as well.

I've tested it locally with "mail.threadpane.use_correspondents" set to true and false in all-thunderbird.js. Setting it locally in the test doesn't work, since the dbview.js apparently reads the preference at initialisation time, so changes in the test have no effect.

I recommend uplift to ESR45 as quickly as possible since with bug 1268325 that landed alone, the UI is confusing since the correspondent still shows up in gloda. Also, the test failures are worrying and might cover-up other issues.

Comment 6

[Jorg K (CEST = GMT+2)]

Comment on attachment 8750190 [details] [diff] [review]
Proposed solution (v1).

Kent said once that he's not a Mozmill type of guy, so perhaps Aceman can review this quicker.

Comment 7

[:aceman]

Comment on attachment 8750190 [details] [diff] [review]
Proposed solution (v1).

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

Thanks for the patch. But it will conflict with bug 562266 that is already done.
But I am inclined to land this one first as 562266 is not yet decided if it is destined for TB45.

::: mail/test/mozmill/folder-display/test-columns.js
@@ +464,5 @@
>    assert_visible_columns(conExtra);
>    be_in_folder(folderChild2);
>    assert_visible_columns(conExtra);
>  }
>  test_apply_to_folder_and_children.EXCLUDED_PLATFORMS = ["linux"];

Yes, I'm great choice for a test that does not run on Linux :) Please push this to try so I can see it :)

Comment 8

[Jorg K (CEST = GMT+2)]

(In reply to :aceman from comment #7)
> Thanks for the patch. But it will conflict with bug 562266 that is already
> done.
That will need to be rebased then ;-)

> Yes, I'm great choice for a test that does not run on Linux :) Please push
> this to try so I can see it :)
Well, which version do you want to see? The patch or another patch with the preference set to false like we need for ESR45.

I thought you can just look at the code. Try doesn't look very promising these days, but I'll do you an Windows XP debug Mozmill with the option set to false. That has a chance to come out green.

To the patch submitted to try will have one additional hunk in all-thunderbird.js.

Comment 9

[Jorg K (CEST = GMT+2)]

OK, here you go:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=86eb344247fc
Switching the preference is in the "try" patch.

Comment 10

[Jorg K (CEST = GMT+2)]

No luck, didn't build. Sigh. Some bustage:

c:/builds/moz2_slave/tb-try-c-cen-w32-d-00000000000/build/mailnews/extensions/smime/src/nsMsgComposeSecure.cpp(901): error C2065: 'ScopedCERTCertList': undeclared identifier
c:/builds/moz2_slave/tb-try-c-cen-w32-d-00000000000/build/mailnews/extensions/smime/src/nsMsgComposeSecure.cpp(901): error C2146: syntax error: missing ';' before identifier 'builtChain'
c:/builds/moz2_slave/tb-try-c-cen-w32-d-00000000000/build/mailnews/extensions/smime/src/nsMsgComposeSecure.cpp(901): error C2065: 'builtChain': undeclared identifier
c:/builds/moz2_slave/tb-try-c-cen-w32-d-00000000000/build/mailnews/extensions/smime/src/nsMsgComposeSecure.cpp(910): error C2065: 'builtChain': undeclared identifier
c:/builds/moz2_slave/tb-try-c-cen-w32-d-00000000000/build/mailnews/extensions/smime/src/nsMsgComposeSecure.cpp(932): error C2065: 'builtChain': undeclared identifier
c:/builds/moz2_slave/tb-try-c-cen-w32-d-00000000000/build/mozilla/config/rules.mk:934: recipe for target 'nsMsgComposeSecure.obj' failed

Comment 11

[:aceman]

Didn't work out (missing some of the latest patches for build breakages). Are you pushing from an old trunk?

Comment 12

[Jorg K (CEST = GMT+2)]

Yes, pushing from an old trunk. That pushes based on my parent??
Anyway, I've done it again:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=e625b36e26dd

Comment 13

[Kent James (:rkent)]

Although I am not a big fan of the correspondents column in general, and therefore am a big fan of Jorg's work to make it less forced, the one place that it really shines is in cross-folder views, such as a gloda synthetic view.

I could see an argument for never forcing a change to correspondent column in normal view, but keeping it as the default in gloda views. That would argue against the changes proposed in this bug to synthetic views default columns.

But I'm not a big users of gloda anyway, so I don't have a strong opinion here. But I do have to decide real soon now whether to delay 45.1.0 waiting for a resolution of this issue one way or another.

Comment 14

[Jorg K (CEST = GMT+2)]

I'd ship it (together with ..., I won't repeat it again) to make it consistent.
Option off: You don't see it. Option on: You do see it ;-)
Hopefully the try will finish soon.

Comment 15

[Jorg K (CEST = GMT+2)]

Well, we agree that the test changes don't matter to the user. So the only thing visible is the change in dbview.js for gloda.

IIRC, the VIRTUAL_DEFAULTS (not sure what they're used for) already don't use correspondents when the option is switched off.

Comment 16

[:aceman]

Comment on attachment 8750190 [details] [diff] [review]
Proposed solution (v1).

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

OK, the tests pass for me locally (also the one Linux-disabled test), with both variants of the pref value (true/false).

Comment 17

[Jorg K (CEST = GMT+2)]

Aceman, thanks, so you were the right guy after all ;-)
Kent, land it like this at least on C-C?

Comment 18

[Jorg K (CEST = GMT+2)]

I've looked at search folders and gloda message lists. I think for consistency sake we should land this as I suggested.

So speak now or forever hold your peace ;-)

Comment 19

[Kent James (:rkent)]

(In reply to Jorg K (PTO during summer, NI me) from comment #17)
> Aceman, thanks, so you were the right guy after all ;-)
> Kent, land it like this at least on C-C?

I spoke my piece in comment 13, but since it has passed review, you are free to land it. If I (or someone else) feels strongly we can file a followup bug. But I probably will not.

Comment 20

[Wayne Mery (:wsmwk)]

(In reply to Jorg K (PTO during summer, NI me) from comment #18)
> I've looked at search folders and gloda message lists. I think for
> consistency sake we should land this as I suggested.
> 
> So speak now or forever hold your peace ;-)

Kent's idea has some merit I think. And I'm very certain I've not heard complaints about correspondent column appearing for global search results. Matt, have you seen any?

But we can enable it in the future, jorg, correct?  (perhaps via some wizard prompt previously discussed)

Comment 21

[Jorg K (CEST = GMT+2)]

https://hg.mozilla.org/comm-central/rev/f5a466d560c4 --> FIXED

===

This patch makes the overall behaviour consistent. With mail.threadpane.use_correspondents set to false, the correspondent column will not show up as default column anywhere any more. Of course users can choose the columns they want individually. Once chosen for a gloda view, the choice will stick for the next gloda view.

With mail.threadpane.use_correspondents set to true, the correspondent column will show up as default column anywhere.

I believe that is consistent behaviour.

As for Kent's comment on virtual folders:
Search folders *already* followed the normal folders even without the patch since the default folders were set in bug 1268325 and the code makes not difference between normal and virtual. There was a puzzling discrepancy between search folders and gloda message lists.

Note that I have raised bug 1269196 to add a tick box somewhere so users can switch off the option which will be set by default from version TB 47.

Comment 22

[Jorg K (CEST = GMT+2)]

Comment on attachment 8750190 [details] [diff] [review]
Proposed solution (v1).

Approval Request Comment

This needs uplift to join its sister bug 1268325. It particularly needs uplift to ESR45 so fix the failing test. There is practically no risk since it mostly fixes a test. There is one hunk to correct the gloda default columns.

Comment 23

[Jorg K (CEST = GMT+2)]

Aurora (TB 48):
https://hg.mozilla.org/releases/comm-aurora/rev/db7310351bb4

Comment 24

[Kent James (:rkent)]

Comment on attachment 8750190 [details] [diff] [review]
Proposed solution (v1).

http://hg.mozilla.org/releases/comm-beta/rev/e0c70b4d92bc
http://hg.mozilla.org/releases/comm-esr45/rev/cc1e319d0228