Closed Bug 1568723 Opened 5 years ago Closed 5 years ago

Mini-month should only load free/busy data when first visible

Categories

(Calendar :: Calendar Frontend, enhancement)

Product:
Calendar
Component:
Calendar Frontend
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED FIXED

People

(Reporter: darktrojan, Assigned: darktrojan)

References

(Blocks 1 open bug)

Details

(Keywords: perf)

Attachments

(5 files, 3 obsolete files)

1568723-minimonth-lazy-3.diff
(deleted), patch
pmorris
: review+
Details | Diff | Splinter Review
esr68-1568723-minimonth-lazy-3.diff
(deleted), patch
darktrojan
: review+
darktrojan
: approval-calendar-esr+
Details | Diff | Splinter Review
1568723-merge-error.patch
(deleted), patch
pmorris
: review+
pmorris
: approval-calendar-esr+
Details | Diff | Splinter Review
1568723-wait-for-xbl-esr1.diff
(deleted), patch
pmorris
: review+
pmorris
: approval-calendar-esr+
Details | Diff | Splinter Review
1568723-wait-for-xbl-esr2.diff
(deleted), patch
pmorris
: review+
pmorris
: approval-calendar-esr+
Details | Diff | Splinter Review

In testing, I've found that loading the free/busy data at start-up wastes a lot of time, even if there is no mini-month visible.

Attached patch 1568723-minimonth-lazy-1.diff (obsolete) (deleted) — — Splinter Review

This gets rid of a lot of back-and-forth between the today pane and the agenda list (which really should be the same thing, since there's a 1:1 relationship and no clear reason for separation).

I've removed the freebusy attribute from the XUL and add it only when a mini-month is shown. That is, when the calendar or tasks tab is opened, or the today pane is shown and with the mini-month view.

Attachment #9080510 - Flags: review?(paul)
Blocks: tb-startupperf
Keywords: perf
Comment on attachment 9080510 [details] [diff] [review]
1568723-minimonth-lazy-1.diff

This needs some more work.
Attachment #9080510 - Flags: review?(paul)
Attached patch 1568723-minimonth-lazy-2.diff (obsolete) (deleted) — — Splinter Review

Oh, this'll do. I have a much bigger fight to pick with the Today Pane but it can wait.

Attachment #9080510 - Attachment is obsolete: true
Attachment #9080814 - Flags: review?(paul)
Comment on attachment 9080814 [details] [diff] [review]
1568723-minimonth-lazy-2.diff

Review of attachment 9080814 [details] [diff] [review]:
-----------------------------------------------------------------

r+ with one question and a couple of nits addressed.  Overall looks good, and the freebusy data in minimonths appears as expected when I took it for a test run.  I'm looking forward to that bigger fight with Today Pane. :-)

::: calendar/base/content/agenda-listbox-utils.js
@@ +695,5 @@
>  
>  /**
>   * Sets up the calendar for the agenda listbox.
>   */
>  agendaListbox.setupCalendar = async function() {

Looks like you don't need the 'async' anymore.

::: calendar/base/content/today-pane.js
@@ +430,5 @@
>      },
>  
>      /**
>       * Update the today-splitter state and today-pane width with saved
>       * mode-dependent values.

Looks like this doc string already needed updating to reflect previous changes, since I don't see anything about setting the today-pane width in the function?

@@ +436,5 @@
>      updateSplitterState: function() {
>          let splitter = document.getElementById("today-splitter");
> +        if (this.isVisible) {
> +            if (this.start === null) {
> +                this.setDay(cal.dtz.now());

It seems a little unintuitive to me to be doing this `setDay` set up step here in this function.  Would it work just as well to do it in `onLoad` instead?  It looks like a one-time set up step, but maybe I'm missing something.
Attachment #9080814 - Flags: review?(paul) → review+
Attached patch 1568723-minimonth-lazy-3.diff (deleted) — — Splinter Review

Shuffled some stuff around. I think initialising things on-demand in setTodayHeader is the right place, as at that point all of the various UI pieces are in the right state so we can accurately determine whether something needs to be done.

Attachment #9080814 - Attachment is obsolete: true
Attachment #9081173 - Flags: review?(paul)
Blocks: 1569513
Comment on attachment 9081173 [details] [diff] [review]
1568723-minimonth-lazy-3.diff

Review of attachment 9081173 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM. r+
Attachment #9081173 - Flags: review?(paul) → review+
Keywords: checkin-needed
Target Milestone: --- → 70

Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/5ecd32f6c2e3
Streamline Today Pane initialisation, and load mini-month free/busy data only when first visible. r=pmorris

Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Keywords: checkin-needed
Resolution: --- → FIXED

I'm porting this patch to esr68 to fix bug 1581411. This also entails porting the patch from bug 1569513 (which depends on this patch).

Attachment #9095667 - Flags: review?(geoff)
Attachment #9095667 - Flags: approval-calendar-esr?(geoff)
Attachment #9095667 - Flags: review?(geoff)
Attachment #9095667 - Flags: review+
Attachment #9095667 - Flags: approval-calendar-esr?(geoff)
Attachment #9095667 - Flags: approval-calendar-esr+
Blocks: 1581411

According to Bug 1560547 Comment 26 the ID on ESR68 branch is "today-Minimonth" not "today-minimonth". Based on how often the ID is used in comm-central https://searchfox.org/comm-central/search?q=today-Minimonth maybe it would be easier to change the ID and usages to "today-minimonth" on ESR branch too?

.\base\content\today-pane.js: document.getElementById("today-minimonth").setAttribute("freebusy", "true");

.\base\content\today-pane.js: document.getElementById("today-Minimonth").value = cal.dtz.dateTimeToJsDate(this.start);
.\base\content\today-pane.js: document.getElementById("today-Minimonth"),
.\base\content\today-pane.xul: <minimonth id="today-Minimonth" onchange="TodayPane.setDaywithjsDate(this.value);"/>

What about
.\base\content\today-pane.xul: <minimonth id="todayMinimonth"

Seems unused and has disappeared from trunk.

Attached patch 1568723-merge-error.patch (obsolete) (deleted) — — Splinter Review

You prefer to remove the unused ID, or does some part of the system insist on having one?

Flags: needinfo?(paul)
Attachment #9097310 - Flags: review?(paul)
Attachment #9097310 - Flags: approval-calendar-esr?(paul)
Attached patch 1568723-merge-error.patch (deleted) — — Splinter Review

Aligned the unused on with trunk:
https://searchfox.org/comm-central/rev/266e9cc242cd0de076e85eb4aa0b8392fcb2ca01/calendar/base/content/today-pane.xul#143
The ID is also unused.

Attachment #9097310 - Attachment is obsolete: true
Attachment #9097310 - Flags: review?(paul)
Attachment #9097310 - Flags: approval-calendar-esr?(paul)
Attachment #9097313 - Flags: review?(paul)
Attachment #9097313 - Flags: approval-calendar-esr?(paul)
Comment on attachment 9097313 [details] [diff] [review]
1568723-merge-error.patch

Review of attachment 9097313 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good.  Thanks for fixing this up.  Aligning ESR with trunk on all of these (including the currently unused one) makes sense.
Attachment #9097313 - Flags: review?(paul)
Attachment #9097313 - Flags: review+
Attachment #9097313 - Flags: approval-calendar-esr?(paul)
Attachment #9097313 - Flags: approval-calendar-esr+
Attached patch 1568723-wait-for-xbl-esr1.diff (deleted) — — Splinter Review

BL :-(

Attachment #9097531 - Flags: review?(paul)
Attachment #9097531 - Flags: approval-calendar-esr?(paul)
Comment on attachment 9097531 [details] [diff] [review]
1568723-wait-for-xbl-esr1.diff

Review of attachment 9097531 [details] [diff] [review]:
-----------------------------------------------------------------

r+ Looks good.  It would be good to add a comment to explain that this code is there to wait for the XBL binding to load, but also fine to leave that out since this is headed for ESR and it's not too hard to understand what the code is doing.
Attachment #9097531 - Flags: review?(paul)
Attachment #9097531 - Flags: review+
Attachment #9097531 - Flags: approval-calendar-esr?(paul)
Attachment #9097531 - Flags: approval-calendar-esr+

As you can see, I build a new version and sadly the error has shifted now:
TypeError: document.getElementById(...).isVisible is not a function today-pane.js:471:56

Yes, I opened the file via the error console to check that the new code had really arrived.

I think with the updateDisplay() call you added, you've also caused this:

date value is not finite in DateTimeFormat.format() minimonth.xml:727
    dateTimeFormatFormatToBind self-hosted:2628
    dateTimeFormatFormatToBind self-hosted:1003
    showMonth chrome://calendar/content/widgets/minimonth.xml:727
    update chrome://calendar/content/widgets/minimonth.xml:902
    set_value chrome://calendar/content/widgets/minimonth.xml:175
    setDay chrome://calendar/content/today-pane.js:395
    updateDisplay chrome://calendar/content/today-pane.js:128
    updateDisplay chrome://calendar/content/today-pane.js:83
    modebox_XBL_Constructor chrome://calendar/content/widgets/calendar-widgets.xml:97
Flags: needinfo?(geoff)
Attached patch 1568723-wait-for-xbl-esr2.diff (deleted) — — Splinter Review

Okay, we'll wait for XBL earlier. What a pain.

(Note to Paul: agendaListbox.init isn't async any more, so let's not await it.)

Flags: needinfo?(geoff)
Attachment #9099119 - Flags: review?(paul)

(In reply to Jorg K (GMT+2) from comment #22)

I think with the updateDisplay() call you added, you've also caused this:

No, I haven't caused that, that's the same problem we're ignoring in bug 1560547.

Attachment #9099119 - Flags: approval-calendar-esr?(paul)
Comment on attachment 9099119 [details] [diff] [review]
1568723-wait-for-xbl-esr2.diff

Review of attachment 9099119 [details] [diff] [review]:
-----------------------------------------------------------------

Changes look fine to me.  r+ assuming Geoff and/or Jorg have confirmed that this prevents the error.  (I'm out sick today, so I haven't confirmed that yet.)
Attachment #9099119 - Flags: review?(paul)
Attachment #9099119 - Flags: review+
Attachment #9099119 - Flags: approval-calendar-esr?(paul)
Attachment #9099119 - Flags: approval-calendar-esr+

r+ assuming Geoff and/or Jorg have confirmed that this prevents the error.

I patched my XPI file to check it. Seems to OK now, more ready for prime time than ever before ;-) - So this will go out in TB 68.1.2 in the next few days.

You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: