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

Status: RESOLVED FIXED

(Calendar :: Calendar Frontend, defect)

Description

[Stefan Sitter [:ssitter]]

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

Comment 1

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

I have a patch to fix this locally, will upload as soon as I have network again on my laptop.

Comment 2

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Attachment: Fix - v1

The promised patch

Comment 3

[Stefan Sitter [: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.

Comment 4

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

(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?

Comment 5

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

The only thing that removing from the end could improve here would be to make the delta in the second loop unneeded.

Comment 6

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Stefan, whats the status on this one? Do you think anything should change or do you agree to my comments?

Comment 9

[Markus Adrario [:Taraman]]

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?

Comment 10

[Stefan Sitter [:ssitter]]

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.

Comment 12

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

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?

Comment 13

[Markus Adrario [:Taraman]]

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.

Comment 14

[Markus Adrario [:Taraman]]

>+++ 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

Comment 15

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Pushed to comm-central <http://hg.mozilla.org/comm-central/rev/265623017726>
-> FIXED

Comment 16

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Pushed to comm-1.9.2 <http://hg.mozilla.org/releases/comm-1.9.2/rev/55be43628c1e>
-> FIXED