1192838 - Fix broken Grouped By view sort direction change, enable custom column grouping

Status: RESOLVED FIXED

(Thunderbird :: Folder and Message Lists, defect)

Comment 1

[alta88]

The forthcoming patch should resolve all the Blocks: bugs.

Comment 2

[Kent James (:rkent)]

To test these, I need to have custom column sort and group view both enabled. The UI won't let me do that. What are you doing that allows you to set this up for test? If I select sort by a custom column (by clicking on the column) it disables group view.

Comment 3

[alta88]

Attachment: enableGroupedByCustomCollapse.patch

This wip will enable the groupby menuitem and enable collapseAll/ExpandAll on groupby.  Column sorts for secondary cust cols should already work.  But cust col column click sort in groupby is not yet done.

Comment 4

[alta88]

Attachment: enableGroupedByCustomCollapse.patch

Better wip, fixes threading regression, fixes remaining issue #1 in bug 1195480 comment 3.

Comment 5

[alta88]

Attachment: enableGroupedByCustomCollapse.patch

Part 1.

Comment 6

[alta88]

Attachment: enableGroupedByColumnClickSort.patch

Part 2, column click sorting.

These patches should all be applied with the ones in the blocking bugs, and finish the scope of this bug. Automatically listing custom columns in the View-Sort By menulist, rather than having the extension do it, is a separate bug.

Comment 7

[alta88]

Attachment: enableGroupedByCustomCollapse.patch

unbitrot.

Comment 8

[alta88]

Attachment: enableGroupedByColumnClickSort.patch

tweak to ensure non virt collapseAll folder sorts.

Comment 9

[Kent James (:rkent)]

These patches are all in the /mail directory code, which I'll review for simple things but would prefer to leave to magnus. I hate to do this to you after you;ve been waiting, but I really think we should ask Magnus for the reviews. The blocking bugs are all approved now and should land soon.

I'll be mostly AFK until next Tuesday in any case so would not get to them for another week.

Comment 10

[alta88]

Attachment: enableGroupedByCustomCollapse.patch

Comment 11

[alta88]

Attachment: enableGroupedByColumnClickSort.patch

unbitrot.

Comment 12

[alta88]

Attachment: enableGroupedByCustomCollapse.patch

the patch in bug 544489 must be done first.

Comment 13

[alta88]

Attachment: enableGroupedColumnsMap.patch

clean up the clickable columns map.

Comment 14

[Magnus Melin [:mkmelin]]

These are some pretty complex changes. Could you add some tests?

Comment 15

[alta88]

Attachment: enableGroupedTests.patch

Not that complex, imo.  By volume of change, it's mostly refactoring for early return and making dense style more readable.  The logic changes are documented in the code.

Anyway, there are some grouped view tests in xpcshell.  This adds several more and tests that custom column grouping works.

Comment 16

[Magnus Melin [:mkmelin]]

Comment on attachment 8664008 [details] [diff] [review]
enableGroupedByCustomCollapse.patch

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

::: mail/base/content/mail3PaneWindowCommands.js
@@ +468,5 @@
>            return false;
>        case "cmd_expandAllThreads":
>        case "cmd_collapseAllThreads":
> +        return gFolderDisplay.view.showThreaded ||
> +               gFolderDisplay.view.showGroupedBySort;;

nit: double semicolon

Comment 17

[Magnus Melin [:mkmelin]]

Comment on attachment 8670824 [details] [diff] [review]
enableGroupedTests.patch

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

::: mailnews/base/test/unit/test_nsMsgDBView.js
@@ +303,5 @@
> +  var outCount = {};
> +  gDBView.open(aTestFolder, aSortType, aSortOrder, viewFlags, outCount);
> +  // outCount is 0 if byCustom; view is built by addColumnHandler()
> +  dump("  View Out Count: " + outCount.value + "\n");
> +

please remove the dump

Comment 18

[alta88]

Attachment: enableGroupedByCustomCollapse.patch

Comment 19

[alta88]

Attachment: enableGroupedTests.patch

Comment 20

[alta88]

Attachment: enableCustomColSortAPIs.patch

the original dbviewWrapper created _getSortDetails() to handle the problem of different custom sorts with the same sortType, and even implemented it in _createView(). but until the backend supported secondary/multiple custom columns it wasn't necessary in the sort() and magicSort() apis.  fix that now and enable asuth's incredible prescience to be fulfilled..

Comment 21

[Magnus Melin [:mkmelin]]

Comment on attachment 8674354 [details] [diff] [review]
enableCustomColSortAPIs.patch

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

Looks reasonable to me.

Comment 22

[alta88]

the checkin order, if it matters, should be:

enableGroupedByCustomCollapse.patch
enableGroupedByColumnClickSort.patch
enableGroupedColumnsMap.patch
enableCustomColSortAPIs.patch
enableGroupedTests.patch

thanks.

Comment 23

[aleth [:aleth]]

https://hg.mozilla.org/comm-central/rev/e536c76c22e7e7311bedc8b7fa53fd9d6af4346e
Bug 1192838 - Fix broken Grouped By view sort direction change, enable custom column grouping menuitem and expandAll/collapseAll on groups, Part 1. r=mkmelin

https://hg.mozilla.org/comm-central/rev/0ace2ac366214487cfe6c7ecb18cb57f9f25c5f4
Bug 1192838 - Fix broken Grouped By view sort direction change, enable custom column grouping, Part 2 column click sorting. r=mkmelin

https://hg.mozilla.org/comm-central/rev/f84ffb9ecdd2d340b44769d25a40c36a456b9fb5
Bug 1192838 - Fix broken Grouped By view sort direction change, enable custom column grouping menuitem and expandAll/collapseAll on groups, Part 3 - clean up columns map. r=mkmelin

https://hg.mozilla.org/comm-central/rev/60912148758030fd23c39662bd8301f64dad0ada
Bug 1192838 - Fix broken Grouped By view sort direction change, enable custom column grouping menuitem and expandAll/collapseAll on groups, Part 4 - make sort() and magicSort() support multiple custom columns. r=mkmelin

https://hg.mozilla.org/comm-central/rev/f03f355dcdd100da3d056adf81a576465116d952
Bug 1192838 - Fix broken Grouped By view sort direction change: Part 5 - grouped tests. r=mkmelin a=aleth

Comment 24

[Jonathan Kamens]

Yay! Thank you!