The default bug view has changed. See this FAQ.

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

RESOLVED FIXED in Thunderbird 49.0

Status

Thunderbird
Folder and Message Lists
RESOLVED FIXED
11 months ago
10 months ago

People

(Reporter: Jorg K (GMT+1), Assigned: Jorg K (GMT+1))

Tracking

Trunk
Thunderbird 49.0

Thunderbird Tracking Flags

(thunderbird46 wontfix, thunderbird47 fixed, thunderbird48 fixed, thunderbird49 fixed, thunderbird_esr4546+ fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

11 months ago
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.
(Assignee)

Comment 1

11 months ago
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 ;-)
(Assignee)

Comment 2

11 months ago
Created attachment 8750065 [details] [diff] [review]
Patch to revert columns to sender/recipient in test for ESR45
Blocks: 1268325
(Assignee)

Comment 3

11 months ago
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

11 months ago
Thanks for looking into this. Did you mean to request review?
(Assignee)

Updated

11 months ago
status-thunderbird46: --- → wontfix
status-thunderbird47: --- → affected
status-thunderbird48: --- → affected
status-thunderbird49: --- → affected
status-thunderbird_esr45: --- → affected
tracking-thunderbird_esr45: --- → ?
(Assignee)

Updated

11 months ago
Summary: TEST-UNEXPECTED-FAIL | C:\slave\test\build\tests\mozmill\folder-display\test-columns.js → Fix test-columns.js to respect "mail.threadpane.use_correspondents", also fix gloda to respect "mail.threadpane.use_correspondents" (oversight from bug 1268325)
(Assignee)

Comment 5

11 months ago
Created attachment 8750190 [details] [diff] [review]
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.
Assignee: nobody → mozilla
Attachment #8750065 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #8750190 - Flags: review?(rkent)
Attachment #8750190 - Flags: approval-comm-esr45?
Attachment #8750190 - Flags: approval-comm-beta?
Attachment #8750190 - Flags: approval-comm-aurora?
(Assignee)

Comment 6

11 months ago
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.
Attachment #8750190 - Flags: review?(acelists)

Comment 7

11 months ago
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 :)
(Assignee)

Comment 8

11 months ago
(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.
(Assignee)

Comment 9

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

Comment 10

11 months ago
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

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

Comment 12

11 months ago
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

11 months ago
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.
(Assignee)

Comment 14

11 months ago
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.
(Assignee)

Comment 15

11 months ago
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

11 months ago
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).
Attachment #8750190 - Flags: review?(acelists) → review+
(Assignee)

Comment 17

11 months ago
Aceman, thanks, so you were the right guy after all ;-)
Kent, land it like this at least on C-C?
Flags: needinfo?(rkent)
(Assignee)

Comment 18

11 months ago
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

11 months ago
(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.
Flags: needinfo?(rkent)
(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)
Flags: needinfo?(unicorn.consulting)
(Assignee)

Comment 21

11 months ago
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.
Status: ASSIGNED → RESOLVED
Last Resolved: 11 months ago
status-thunderbird49: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 49.0
(Assignee)

Comment 22

11 months ago
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.
Attachment #8750190 - Flags: review?(rkent)
Attachment #8750190 - Flags: approval-comm-aurora?
Attachment #8750190 - Flags: approval-comm-aurora+
(Assignee)

Comment 23

11 months ago
Aurora (TB 48):
https://hg.mozilla.org/releases/comm-aurora/rev/db7310351bb4
status-thunderbird48: affected → fixed
Flags: needinfo?(unicorn.consulting)

Comment 24

10 months ago
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
Attachment #8750190 - Flags: approval-comm-esr45?
Attachment #8750190 - Flags: approval-comm-esr45+
Attachment #8750190 - Flags: approval-comm-beta?
Attachment #8750190 - Flags: approval-comm-beta+

Updated

10 months ago
status-thunderbird47: affected → fixed
status-thunderbird_esr45: affected → fixed
tracking-thunderbird_esr45: ? → 46+
You need to log in before you can comment on or make changes to this bug.