Closed Bug 508320 Opened 13 years ago Closed 12 years ago

Removing calendar incorrectly updates the unifinder, wrong events displayed until reload

Categories

(Calendar :: Calendar Frontend, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: ssitter, Assigned: Fallen)

References

Details

(Keywords: regression, Whiteboard: [needed beta][no l10n impact])

Attachments

(1 file)

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1.3pre) Gecko/20090803 Sunbird/1.0pre

Found during testing of Bug 508041. Test works in Sunbird 0.9 -> regression.

Steps to Reproduce, Actual Results, Expected Results:
=====================================================

1) startup Sunbird with clean profile:
    console: CalendarList addCalendar() calls rowCountChanged(0, 1)
    console: Unifinder setItems() calls rowCountChanged(0, 0)

--> unifinder is empty --> OK

2) add 3 events (1A, 1B, 1C) to default calendar:
    console: Unifinder addItems() calls rowCountChanged(-1, 1)
    console: Unifinder addItems() calls rowCountChanged(0, 1)
    console: Unifinder addItems() calls rowCountChanged(1, 1)

--> unifinder content is 1A, 1B, 1C --> OK

3) add a second calendar and select it

    console: CalendarList addCalendar() calls rowCountChanged(1, 1)
    console: Unifinder setItems() calls rowCountChanged(0, 0)

--> unifinder content is 1A, 1B, 1C --> OK

4) add 3 events (2A, 2B, 2C) to second calendar:

    console: Unifinder addItems() calls rowCountChanged(2, 1)
    console: Unifinder addItems() calls rowCountChanged(3, 1)
    console: Unifinder addItems() calls rowCountChanged(4, 1)

--> unifinder content is 1A, 1B, 1C, 2A, 2B, 2C --> OK

5) remove the first calendar:

    console: CalendarList removeCalendar() calls rowCountChanged(0, -1)
    console: Unifinder removeItems() calls rowCountChanged(0, -1)
    console: Unifinder removeItems() calls rowCountChanged(1, -1)
    console: Unifinder removeItems() calls rowCountChanged(2, -1)

--> unifinder content is 1B, 2A, 2C --> NOT OK, expected is 2A, 2B, 2C
Flags: blocking-calendar1.0?
Flags: blocking-calendar1.0? → blocking-calendar1.0+
Whiteboard: [not needed beta][no l10n impact]
I have a patch to fix this locally, will upload as soon as I have network again on my laptop.
Assignee: nobody → philipp
Status: NEW → ASSIGNED
OS: Windows XP → All
Hardware: x86 → All
Attached patch Fix - v1 β€” β€” Splinter Review
The promised patch
Attachment #393869 - Flags: review?(ssitter)
This patch seems rather long. Would it be possible to just remove the items from the end of the array instead of the beginning, i.e. from last to first item? This way the index positions for the items to be removed won't change on the fly. If I remember correctly something like this was done in the past.
(In reply to comment #3)
> This patch seems rather long. Would it be possible to just remove the items
> from the end of the array instead of the beginning, i.e. from last to first
> item? This way the index positions for the items to be removed won't change on
> the fly. If I remember correctly something like this was done in the past.

I don't think this will help. The items in aItemArray are not necessarily sorted, we would need to first sort this array using the same criteria as the unifinder, then remove the items from the end. I don't think this would be more performant than this solution?
The only thing that removing from the end could improve here would be to make the delta in the second loop unneeded.
Whiteboard: [not needed beta][no l10n impact] → [not needed beta][no l10n impact][needs review]
Stefan, whats the status on this one? Do you think anything should change or do you agree to my comments?
Duplicate of this bug: 423320
Duplicate of this bug: 455552
I just wanted to take over the review and I noticed that I could not reproduce this behaviour on windows (latest available Sunbird & Lightning builds)

Can anyone else confirm that this bug has been solved somehow in the meantime?
I can reproduce it using Lightning 1.0b2pre (20100504) with Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US; rv:1.9.2.5pre) Gecko/20100505 Lanikai/3.1pre.
Duplicate of this bug: 563896
Whiteboard: [not needed beta][no l10n impact][needs review] → [needed beta][no l10n impact][needs review]
So, whats the status? Stefan, do you think you could review this soon? Otherwise, maybe you have a new/different set of STR's for Markus?
Meanwhile I could reprocduce the behaviour myself and I can confirm that the patch works.
As for the code review, I'm not sure how much time I can spare for the next week, but I try.
Stefan, if you start the review, let me know, wo we don't do double work.
>+++ calendar-unifinder.js
>@@ -461,10 +469,9 @@
>      * Sets the currently selected column in the unifinder (used for sorting).
>      */
>     set selectedColumn uTV_setSelectedColumn(aCol) {
>-        var tree = document.getElementById("unifinder-search-results-tree");
>-        var treecols = tree.getElementsByTagName("treecol");
>-        for (var i = 0; i < treecols.length; i++) {
>-            var col = treecols[i];
>+        let tree = document.getElementById("unifinder-search-results-tree");
>+        let treecols = tree.getElementsByTagName("treecol");
>+        for each (let col in Array.slice(treecols); i++) {
>             if (col.getAttribute("sortActive")) {
>                   col.removeAttribute("sortActive");
>                   col.removeAttribute("sortDirection");
in the for each statement the (...; i++) is too much.

else only one typo:
+        // Then we go through the indexes to remove, and remove then from the
then in stead of them.

For the method of deletion I thought it through several times and can't come up with a simpler or more performant solution - the reasons philipp stated make sense.

One more question at the end:
By what criteria did you select the spots where you updated "var" with "let"?
If you intended to update the whole file, you missed several ocurrences. ;-)

with the above corrections
r=markus
Attachment #393869 - Flags: review?(ssitter) → review+
Whiteboard: [needed beta][no l10n impact][needs review] → [needed beta][no l10n impact]
Pushed to comm-central <http://hg.mozilla.org/comm-central/rev/265623017726>
-> FIXED
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → 1.0b2
You need to log in before you can comment on or make changes to this bug.