Closed Bug 411690 Opened 17 years ago Closed 17 years ago

Avoid unifinder startup load

Categories

(Calendar :: General, enhancement)

enhancement
Not set
normal

Tracking

(Not tracked)

VERIFIED FIXED

People

(Reporter: dbo, Assigned: Fallen)

Details

Attachments

(1 file, 1 obsolete file)

If I hide the unifinder it nevertheless generates unnecessary load on startup, getting items (w.r.t. the chosen filter).
Attached patch Inhibit load when hidden (obsolete) β€” β€” Splinter Review
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.
Assignee: nobody → philipp
Status: NEW → ASSIGNED
Attachment #296737 - Flags: review?
Attachment #296737 - Flags: review? → review?(Berend.Cornelius)
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.
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.
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)
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.
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.
Attachment #296737 - Attachment is obsolete: true
Attachment #296813 - Flags: review?(daniel.boelzle)
Attachment #296737 - Flags: review?(Berend.Cornelius)
Ah too fast on submiting. Pretend in the onLoad event the first if() condition changes as follows:

---        if (isUnifinderHidden()) {
+++        if (isUnifinderHidden() && !gUnifinderNeedsRefresh) {
Attachment #296813 - Flags: review?(daniel.boelzle) → review?(michael.buettner)
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.
Attachment #296813 - Flags: review?(michael.buettner) → review+
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.
>         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...
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
Status: ASSIGNED → RESOLVED
Closed: 17 years ago
Resolution: --- → FIXED
Target Milestone: --- → 0.8
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
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: