423320 - Calendar Unifinder: Setting filter updates tree incorrectly

Status: RESOLVED DUPLICATE of bug 508320

(Calendar :: Calendar Frontend, defect)

Description

[Berend Cornelius]

Steps to reproduce:
- Set the filter to the "Next 7 days"
- Create a Series of 5 events from today.
- Set The filter to today -> one event is correctly displayed
- Set the filter back to "Next 7 days"
- delete the event from tomorrow from the series
- Set the filter back to today -> no event is displayed which is wrong the event of today is still there.

I had a look at it and it seems to me that this is a problem of the rowcountChange in "setItems" in calendar-Unifinder.js in line 420.

Comment 1

[Simon Paquet [:sipaq]]

Not a blocker, but we'll take a patch if one materializes.

Comment 2

[Berend Cornelius]

Attachment: patch v. #1

Comment 3

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

Comment on attachment 311759 [details] [diff] [review]
patch v. #1

>     removeItems: function uTV_removeItems(aItemArray) {
>         for each (var item in aItemArray) {
>             var row = this.getItemRow(item);
>             if (row > -1) {
>                 this.eventArray.splice(row, 1);
>                 if (this.tree) {
>                     this.tree.rowCountChanged(row, -1);
>                 }
>+                this.calculateIndexMap();
>             }
>         }
>-        this.calculateIndexMap();
>     },
This change will call calculateIndexMap each time an event is removed. Its better to do so after all items have been removed, otherwise runtime complexity is quite a bit higher.

What you can do is to only run the function if items have actually been removed, but I don't think its often the case that removeItems is being called with items that do not exist.

r=philipp for the first hunk.

Comment 4

[Berend Cornelius]

As we discussed it seems necessary to call "calculateIndexMap" after each removing action because each removal action potentially also modifies the index map, which of course has a negative impact on the performance. Looking at the code in general I don't understand why all the action within and linked with "prepareCalendarUnifinder()" has to be taken place on load. Instead this could be called when th Unifinder is explicitly demanded either by a menu command or - when its visibility is persistent - on the modechange. We have cared so much about the performance of the unifinder. Therefor I think here it is worth while changing the onload behaviour towards a "onDemand" behaviour too.

Comment 5

[Sven Giermann]

Any updates on this?

r+ patch available, but no check-in - are you going to address comment #4 in this one, or do you want to file another bug on it?

Comment 6

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

This will be fixed by bug: