Closed Bug 1545199 Opened 6 years ago Closed 5 years ago

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

Categories

(Calendar :: Tasks, defect)

Product:
Calendar
Component:
Tasks
Version:
Lightning 68
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED FIXED

People

(Reporter: pmorris, Assigned: pmorris)

References

Details

Attachments

(2 files, 2 obsolete files)

part1-persist-task-tree-columns-1.patch
(deleted), patch
Details | Diff | Splinter Review
part2-refactor-restore-task-tree-columns-1.patch
(deleted), patch
Details | Diff | Splinter Review

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.

Attached patch part1-persist-task-tree-columns-0.patch (obsolete) (deleted) — — Splinter Review

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

Assignee: nobody → paul
Status: NEW → ASSIGNED
Attachment #9075490 - Flags: review?(mkmelin+mozilla)

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.

Attachment #9075491 - Flags: review?(mkmelin+mozilla)
Comment on attachment 9075490 [details] [diff] [review] part1-persist-task-tree-columns-0.patch Magnus isn't here and I need this next week.
Attachment #9075490 - Flags: review?(philipp)
Attachment #9075490 - Flags: review?(geoff)
Attachment #9075490 - Flags: approval-calendar-beta?(philipp)
Attachment #9075491 - Flags: review?(philipp)
Attachment #9075491 - Flags: review?(geoff)
Attachment #9075491 - Flags: approval-calendar-beta?(philipp)
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.
Attachment #9075490 - Flags: review?(philipp)
Attachment #9075490 - Flags: review?(mkmelin+mozilla)
Attachment #9075490 - Flags: review?(geoff)
Attachment #9075490 - Flags: review+
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)
Attachment #9075491 - Flags: review?(philipp)
Attachment #9075491 - Flags: review?(mkmelin+mozilla)
Attachment #9075491 - Flags: review?(geoff)
Attachment #9075491 - Flags: review+
Attachment #9075490 - Flags: approval-calendar-beta?(philipp) → approval-calendar-beta+
Attachment #9075491 - Flags: approval-calendar-beta?(philipp) → approval-calendar-beta+
Target Milestone: --- → 7.1

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

Attachment #9075490 - Attachment is obsolete: true

2 of 2. Revised based on review.

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

Attachment #9075491 - Attachment is obsolete: true

Pushed by geoff@darktrojan.net:
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

Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: