411690 - Avoid unifinder startup load

Status: VERIFIED FIXED

(Calendar :: General, enhancement)

Description

[Daniel Boelzle [:dbo]]

If I hide the unifinder it nevertheless generates unnecessary load on startup, getting items (w.r.t. the chosen filter).

Comment 1

[Philipp Kewisch [:Fallen][📱📆]]

Attachment: Inhibit load when hidden

This patch takes care of not refreshing anything as long as the unifinder is hidden. All events are removed from the unifinder, as soon as the unifinder is shown, the event view is refreshed.

Comment 2

[Michiel van Leeuwen (email: mvl+moz@)]

Why are you removing the items when the unifinders gets hidden? Once it has been visible, you can use the observers to track changes, so you don't need to re-fetch all the items when the unifinder is shown later.

Comment 3

[Philipp Kewisch [:Fallen][📱📆]]

When the unifinder is hidden, there is no need to refresh events. This is true not only for on first load but any time the unifinder is hidden. If I would leave the events in there, then the data might be quite old and will be refreshed anyway.

I had a different solution, where the unifinder load was postponed until it was first shown, but I thought it might be better to avoid load any time its hidden.

Comment 4

[Michiel van Leeuwen (email: mvl+moz@)]

There is indeed no need to call GetItems while hidden. But you can still process onAddEvent and friends. There is almost no performance hit in that, and will speed up showing the unifinder in case there was no full reload in-between. (for example, when only using storage calendars)

Comment 5

[Philipp Kewisch [:Fallen][📱📆]]

Yes, this works fine as long as no auto-refresh is done. Lets say a refresh while the unifinder is hidden happens. A new event is added in the views, due to a remote edit. Now this item is deleted by the user. The onDeleteItem call will have an item that does not exist in the unifinder. Of course I could just ignore this event if the item does not exist.

Also, if events are removed during a refresh, the unifinder will not notice. When the unifinder is shown, the user will initially be presented with old data, until the refresh is complete.

I'd be ok with keeping events though.

Comment 6

[Philipp Kewisch [:Fallen][📱📆]]

Attachment: Inhibit load when hidden - v2

Better solution as suggested by mvl.

If the unifinder is initially hidden, any add/modify/delete operations are ignored until the unifinder is shown. If the unifinder is hidden afterwards, the add/modify/delete operations are continued until an onLoad event happens.

When the unifinder is shown, a refresh only happens if an onLoad event happened while it was hidden, or of course if the unifinder wasn't initially loaded.

Comment 7

[Philipp Kewisch [:Fallen][📱📆]]

Ah too fast on submiting. Pretend in the onLoad event the first if() condition changes as follows:

---        if (isUnifinderHidden()) {
+++        if (isUnifinderHidden() && !gUnifinderNeedsRefresh) {

Comment 8

[Michael Büttner (no reviews TFN)]

Comment on attachment 296813 [details] [diff] [review]
Inhibit load when hidden - v2

>     onLoad: function uO_onLoad() {
>+        if (isUnifinderHidden()) {
>+            // If the unifinder is hidden, all further item operations might
>+            // produce invalid entries in the unifinder. From now on, ignore
>+            // those operations and refresh as soon as the unifinder is shown
>+            // again.
>+            var unifinderTree = document.getElementById("unifinder-search-results-tree");
>+
>+            gUnifinderNeedsRefresh = true;
>+            gEventArray = [];
>+            unifinderTree.view = unifinderTreeView;
>+            unifinderTree.eventView = new calendarEventView(gEventArray);
>+        }
>         if (!this.mInBatch) {
>             refreshEventTree();
>         }
>     },
I suggest to move the 'unifinderTree'-initialization to the beginning of this function:
>     onLoad: function uO_onLoad() {
>         var unifinderTree = document.getElementById("unifinder-search-results-tree");
>         gEventArray = [];
>         unifinderTree.view = unifinderTreeView;
>         unifinderTree.eventView = new calendarEventView(gEventArray);
...and remove the respective (duplicate) counterpart from refreshEventTree().

Apart from that, the patch looks just fine. r=mickey.

Comment 9

[Philipp Kewisch [:Fallen][📱📆]]

I don't quite understand. What initialization? The unifinder should only be emptied when an onload event happens and the unifinder is hidden. If it is showing, the event view should be initialized with the new set of items right away.

Comment 10

[Michael Büttner (no reviews TFN)]

>         unifinderTree.view = unifinderTreeView;
>         unifinderTree.eventView = new calendarEventView(gEventArray);
These lines belong to the unifinder initialization and shouldn't be issued more than once, am I wrong? But anyway, it's just a minor nit, feel free to go ahead with the patch as it currently stands if you disagree...

Comment 11

[Philipp Kewisch [:Fallen][📱📆]]

Not quite, a bit confusing maybe and I'm sure there is a better way, but as you see the "initialization" was always done in the refreshEventTree(Internal) functions, to fill the unifinder with a new set of elements.

Of course I could go on and change the event array and call the invalidate() function, but I'm going to go ahead and check it in as is for now.

Checked in on HEAD and MOZILLA_1_8_BRANCH

-> FIXED

Comment 12

[Omar Bajraszewski]

Verified with Lightning 2008012900 and Mozilla/5.0 (Windows; U; Windows NT 5.1;
pl; rv:1.8.1.12pre) Gecko/20080127 Calendar/0.8pre