1545199 - Changes to currently visible columns in task lists are not persisted after restart

Status: RESOLVED FIXED

(Calendar :: Tasks, defect)

Description

[Paul Morris [:pmorris]]

This is a follow-up to bug 1531096. On current development builds, changes to the currently visible columns (which ones are visible and their order) in the task trees (task lists in the tasks tab and todaypane) are not persisted after closing and re-opening Thunderbird.

Comment 1

[Paul Morris [:pmorris]]

Attachment: part1-persist-task-tree-columns-0.patch

1 of 2 patches. Fixes persisting column state across app restarts.

Comment 2

[Paul Morris [:pmorris]]

Attachment: part2-refactor-restore-task-tree-columns-0.patch

2 of 2 patches. Refactor out a "restore state" function to make it easier to see how this is working by putting the persist and restore logic into adjacent functions.

Comment 3

[Jorg K (CEST = GMT+2)]

Comment on attachment 9075490 [details] [diff] [review]
part1-persist-task-tree-columns-0.patch

Magnus isn't here and I need this next week.

Comment 4

[Geoff Lankow (:darktrojan)]

Comment on attachment 9075490 [details] [diff] [review]
part1-persist-task-tree-columns-0.patch

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

It's disappointing this isn't built in to the tree custom element, but not surprising, unfortunately. This isn't even the only way we persist this data, the unifinder results tree does it differently. Oh well.

::: calendar/base/content/calendar-task-tree.js
@@ +17,5 @@
> +     * Then on startup that data can be set on the attributes of the columns.
> +     *
> +     * @param {CalendarTaskTree} tree  The task tree for which we want to persist column state.
> +     */
> +    function persistTaskTreeColumnState(tree) {

You should make this a member of CalendarTaskTree.

@@ +30,5 @@
> +                visible += visible.length > 0 ? " " + content : content;
> +            }
> +            if (ordinals.length > 0) {
> +                ordinals += " ";
> +            }

Store all these values in arrays, then do e.g. ordinals.join(" ") when setting the attribute.

Comment 5

[Geoff Lankow (:darktrojan)]

Comment on attachment 9075491 [details] [diff] [review]
part2-refactor-restore-task-tree-columns-0.patch

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

Nice. A quick optimisation while you're changing things.

::: calendar/base/content/calendar-task-tree.js
@@ +59,5 @@
> +     * state of the columns across restarts. Used with the `persistTaskTreeColumnState` function.
> +     *
> +     * @param {CalendarTaskTree} tree  The task tree for which we want to restore column state.
> +     */
> +    function restoreTaskTreeColumnState(tree) {

Again, I think this should be a member of CalendarTaskTree.

@@ +68,5 @@
> +        let sortDirection = tree.getAttribute("sortDirection") || "ascending";
> +
> +        tree.querySelectorAll("treecol").forEach((col) => {
> +            const itemProperty = col.getAttribute("itemproperty");
> +            if (visibleColumns.some(visCol => visCol == itemProperty)) {

visibleColumns.includes(itemProperty)

Comment 6

[Paul Morris [:pmorris]]

Attachment: part1-persist-task-tree-columns-1.patch

1 of 2. Revised based on review. I did a little more refactoring while I was at it.

Comment 7

[Paul Morris [:pmorris]]

Attachment: part2-refactor-restore-task-tree-columns-1.patch

2 of 2. Revised based on review.

Thanks for the review. Try server run coming up next.

Comment 8

[Paul Morris [:pmorris]]

Try run is underway (I'm off to bed):
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=a59625f8344d301c1b9999397ad14a4cadcc4bb6

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/091a75e4e8cc
Persist calendar-task-tree column state across restarts. r=darktrojan
https://hg.mozilla.org/comm-central/rev/63616633fa86
Refactor restoring calendar-task-tree column state after restart. r=darktrojan

Comment 10

[Jorg K (CEST = GMT+2)]

TB 68 beta 4 / Cal 7.0 (well, just aligned that version with TB):
https://hg.mozilla.org/releases/comm-beta/rev/ebb463916466d22f7739f4c4e081b0ad07bc82bc
https://hg.mozilla.org/releases/comm-beta/rev/8e975ea6b3bee88e21fe94f4339ec2e527d6566b