1544914 - [de-xbl] convert calendar view bindings: calendar-day-view, calendar-week-view, calendar-multiweek-view, calendar-month-view, calendar-multiday-view, calendar-multiday-view, calendar-month-base-view

Status: RESOLVED FIXED

(Calendar :: Calendar Frontend, task)

Description

[Magnus Melin [:mkmelin]]

Convert the calendar-views.xml bindings to custom element.
https://searchfox.org/comm-central/rev/b2c998eb41d696d31a87384487514e9b386f201c/calendar/base/content/calendar-views.xml#14

Inheritance is from calendar-multiday-view, so that also needs to be included in this conversion.
https://searchfox.org/comm-central/rev/b2c998eb41d696d31a87384487514e9b386f201c/calendar/base/content/calendar-multiday-view.xml#2443

But that inherits from
https://searchfox.org/comm-central/rev/b2c998eb41d696d31a87384487514e9b386f201c/calendar/base/content/calendar-base-view.xml#14

... and that is used in calendar-month-base-view
https://searchfox.org/comm-central/source/calendar/base/content/calendar-month-view.xml#341

Unfortunately they will all probably need to be converted in one shot due to the inheritance...

Comment 1

[Paul Morris [:pmorris]]

Attachment: mozmill-multiple-classes-2.patch

1 of 5 patches. This one improves the mozmill test infrastructure so you can use single class selectors. (Trying this again, see bug 1545142.)

Comment 2

[Paul Morris [:pmorris]]

Attachment: calendar-views-rename-part1-0.patch

2 of 5 patches. Part 1 of 4 for this bug. Rename a file we can better preserve history.

Comment 3

[Paul Morris [:pmorris]]

Attachment: calendar-views-rename-part2-0.patch

3 of 5 patches. Part 2 of 4 for this bug. Rename a file for more consistent file naming. ("calendar-views.js" will be the custom element file and "calendar-views-utils.js" will be for related code.)

Comment 4

[Paul Morris [:pmorris]]

Attachment: calendar-views-part3-de-xbl-0.patch

4 of 5 patches. Part 3 of 4 for this bug.

This is the main patch with the de-xbl work for the calendar views. Apologies for its size.

I ran into some issues with XBL <-> custom elements interoperability with the <calendar-time-bar> element. It seemed better to just go ahead and de-xbl it rather than put time into figuring out the issues. So I went ahead and did that as part of this patch.

Then there were some XUL layout issues with <calendar-time-bar> and it seemed easier to just use HTML instead of XUL for it. (And I just remembered that its class is still extending MozXULELement. Seems to work fine but maybe that should change?)

Comment 5

[Paul Morris [:pmorris]]

Attachment: calendar-views-part4-more-aArgs-0.patch

5 of 5 patches. Part 4 of 4 for this bug.

While I was removing the aArgs from the other files this calendar-views-utils.js file was open in my editor, and I removed the aArgs from it before realizing that wasn't strictly needed for this patch. So I'm including it as a separate patch so the work doesn't go to waste.

Comment 6

[Paul Morris [:pmorris]]

Try server run looks good, only known failures:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=e2d1eefb6a15d00afece31fa3f9b716e954a8f87

Comment 7

[Magnus Melin [:mkmelin]]

Comment on attachment 9061927 [details] [diff] [review]
calendar-views-part3-de-xbl-0.patch

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

Some comments, didn't try it out yet

::: calendar/base/content/calendar-base-view.js
@@ +51,3 @@
>  
> +            const occs = item.getOccurrencesBetween(this.calView.startDate,
> +                this.calView.queryEndDate, {});

re const/let - I think there was some discussion in another bug. 
I don't have a very strong opinion, but might go with let for non-constants (even if they do not change, they can in future code)

@@ +69,3 @@
>  
> +            if (!cal.item.isToDo(oldItem) || oldItem.entryDate || oldItem.dueDate) {
> +                occs = oldItem.getOccurrencesBetween(this.calView.startDate,

could just delcaare let occs here

@@ +86,5 @@
> +                    return;
> +                }
> +            }
> +
> +            occs = newItem.getOccurrencesBetween(this.calView.startDate,

and again here in the other scope

@@ +299,5 @@
>  
> +    /**
> +     * Abstract base class for calendar view elements (day, week, multiweek, month).
> +     *
> +     * @implements {calICalendarView}

please add
@abstract

@@ +512,5 @@
> +        }
> +
> +        get viewBroadcaster() {
> +            return document.getElementById("calendarviewBroadcaster");
> +        }

yuck.

Maybe just use document.getElementById directly for the broadcaster, since it's in no way part of this view

@@ +937,5 @@
>  
> +        disconnectedCallback() {
> +            if (this.mCalendar) {
> +                this.mCalendar.removeObserver(this.mObserver);
> +            }

looks like this would not necessarily work correctly. There must be symmetry with connectedCallback and disconnectedCallback so that they can run mulitple times without losing some part (like this.mObserver)

::: calendar/base/content/calendar-multiday-base-view.js
@@ +160,3 @@
>                  }
>  
> +                const label = document.createElement("label");

here and elsewhere you might want to check you're creating the right kind of element (html or xul, whichever is used in each case)

@@ +213,5 @@
>  
> +    /**
> +     * Abstract class used for the day and week calendar view elements. (Not month or multiweek.)
> +     *
> +     * @implements {calICalendarView}

also add
@extends MozElements.CalendarBaseView

That should make nicer links in generated documentation.

@@ +244,5 @@
> +                         flex="1"
> +                         equalsize="always"/>
> +                    <box class="headerscrollbarspacer multiday-headerscrollbarspacer"/>
> +                  </box>
> +                  <scrollbox class="scrollbox" 

trailing space

@@ +1002,5 @@
> +                return [item];
> +            } else {
> +                // Undated todo.
> +                return [];
> +            }

while here please fix this to not have return after if. so in stead

if (1)
  return
if (2)
  return
return;

::: calendar/base/content/calendar-views.js
@@ +16,1 @@
>  

great!" :)

@@ +17,4 @@
>  
> +// Wrap in a block to prevent leaking to window scope.
> +{
> +    class CalendarDayView extends MozElements.CalendarMultidayBaseView {

pleased add documentation to this class

@@ +65,3 @@
>  
> +
> +    class CalendarWeekView extends MozElements.CalendarMultidayBaseView {

and this

@@ +133,2 @@
>  
> +    class CalendarMultiweekView extends MozElements.CalendarMonthBaseView {

and this

@@ +233,2 @@
>  
> +    class CalendarMonthView extends MozElements.CalendarMonthBaseView {

doc

Comment 8

[Paul Morris [:pmorris]]

(In reply to Magnus Melin [:mkmelin] from comment #7)

re const/let - I think there was some discussion in another bug.
I don't have a very strong opinion, but might go with let for non-constants
(even if they do not change, they can in future code)

Yeah, this came up very briefly here. Philipp asked why I was using const, and I explained the reasoning (for example: https://eslint.org/docs/rules/prefer-const). I still think it's better to prefer const over let. If something needs to change in the future you can always switch to let.

Even if the decision is to prefer let over const overall, then I'd say it's not worth the effort to change the consts back to lets in these patches.

@@ +937,5 @@

•    disconnectedCallback() {
  

•        if (this.mCalendar) {
  

•            this.mCalendar.removeObserver(this.mObserver);
  

•        }
  

looks like this would not necessarily work correctly. There must be symmetry
with connectedCallback and disconnectedCallback so that they can run
mulitple times without losing some part (like this.mObserver)

Hmm, okay, I see the issue. (If only JS classes had a destructor method...) Seems like the solution would be to:

1. Move all the code in the connectedCallback into a constructor method.
2. Put the code that is undone in disconnectedCallback back into the connectedCallback and make connectedCallback able to fire more than once.
3. In child classes, similarly divide code into constructor and connectedCallback, and make the connectedCallbacks able to fire more than once.

Does that sound right? Am I missing something?

On the other hand, I don't think these elements are ever disconnected, so is disconnecedCallback ever called? Is it even needed? Even if you disable/uninstall Lightning, that (currently) requires a restart.

I've made the other changes locally. Would be interested to know your thoughts (mkmelin), on this before heading down one path or the other.

Comment 9

[Magnus Melin [:mkmelin]]

In the constructor you're not allowed to add children or set attributes. Adding event handling is ok.
It's possible some of the tear down needed really should be like we did in bug 1549257, using an onload listener that's called just once.

disconnectedCallback can be fired e.g. if the node gets moved I think. connectedCallback also seems to run more than once for some situations.

Comment 10

[Magnus Melin [:mkmelin]]

Comment on attachment 9061927 [details] [diff] [review]
calendar-views-part3-de-xbl-0.patch

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

Played with it now, and everything seems to work, so f+! Please have Philipp look at it once you did the changes. 
Maybe we want to check in the 3 first patches that do not have "real" changes now?

::: calendar/base/content/calendar-base-view.js
@@ +470,5 @@
> +            };
> +
> +            this.mObserver = new CalendarViewObserver(this);
> +
> +            const { Log4Moz } = ChromeUtils.import("resource:///modules/gloda/log4moz.js");

please move this down to when it's first called

@@ +937,5 @@
>  
> +        disconnectedCallback() {
> +            if (this.mCalendar) {
> +                this.mCalendar.removeObserver(this.mObserver);
> +            }

I think removing the observers in a window unload should do it (and not doing the resetup in connectedCallback)

::: calendar/base/content/calendar-multiday-base-view.js
@@ +1192,5 @@
>                  headerDayBox.removeAttribute("todaylastinview");
>                  dayEventsBox.timeIndicatorBox.setAttribute("hidden", "true");
>                  switch (date.compare(today)) {
> +                    case -1:
> +                        {

the earlier brace style is nicer: put { after the :

Comment 11

[Paul Morris [:pmorris]]

Attachment: calendar-views-part3-more-aArgs-1.patch

Okay, I've addressed all the comments from mkmelin's review.

Also, I've re-ordered the last two patches.

This "more-aArgs" patch is now "part3" of 4 in the series (for this bug).

Next I'll upload the main "de-xbl" patch which is now "part4" of 4 in the series (for this bug).

This way we can go ahead and land all of the patches except for the main de-xbl one, which needs review from Fallen before landing.

Comment 12

[Paul Morris [:pmorris]]

Attachment: calendar-views-part4-de-xbl-1.patch

This is the main patch, part 4 of 4 for this bug. It needs Fallen's review.

(The other patches are ready to go ahead and land.)

Comment 13

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

Comment on attachment 9065233 [details] [diff] [review]
calendar-views-part4-de-xbl-1.patch

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

Wow, superb amount of work here! I thought these would be most difficult. Given the size of the diff and likely use of the xbl converter I'm going to rubberstamp this. I did a spot check and what I saw looks great!

Comment 14

[Paul Morris [:pmorris]]

Attachment: mozmill-multiple-classes-3.patch

Thanks for the review. I did use the XBL converter. I've rebased on current trunk and am uploading the rebased patches.

This is the first one, with the mozmill change.

Comment 15

[Paul Morris [:pmorris]]

Attachment: calendar-views-rename-part1-1.patch

Part 1 of 4.

Comment 16

[Paul Morris [:pmorris]]

Attachment: calendar-views-rename-part2-1.patch

Part 2 of 4.

Comment 17

[Paul Morris [:pmorris]]

Attachment: calendar-views-part3-more-aArgs-2.patch

Part 3 of 4.

Comment 18

[Paul Morris [:pmorris]]

Attachment: calendar-views-part4-de-xbl-2.patch

Part 4 of 4.

I ran the calendar mozmill tests locally with the rebased patches and they all passed. So these are ready for checkin. (Let me know if another try server run is needed.)

Comment 19

[Paul Morris [:pmorris]]

Attachment: calendar-views-part4-de-xbl-3.patch

Sigh... In that previous "Part 4 of 4" patch (calendar-views-part4-de-xbl-2.patch) somehow in the rebasing process Mercurial lost the trail that the new js files were copied/renamed from the deleted xbl files.

This new "Part 4 of 4" patch restores the linkage between the deleted xbl files and new js files (for history's sake).

Comment 20

[Jorg K (CEST = GMT+2)]

https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=06248c61e32429ee1c587154e9ebb976d6266378

Thes patches are in conflict with bug 1504416, but applying bug 1504416 last gave worse errors:

applying calendar-views-rename-part2-1.patch
unable to find 'calendar/base/content/widgets/calendar-list-tree.xml' for patching
(use '--prefix' to apply patch relative to the current directory)
1 out of 1 hunks FAILED -- saving rejects to file calendar/base/content/widgets/calendar-list-tree.xml.rej
patching file calendar/lightning/content/messenger-overlay-sidebar.xul
Hunk #1 FAILED at 42
1 out of 1 hunks FAILED -- saving rejects to file calendar/lightning/content/messenger-overlay-sidebar.xul.rej

applying calendar-views-part4-de-xbl-3.patch
patching file calendar/base/jar.mn
Hunk #1 succeeded at 12 with fuzz 2 (offset -1 lines).
patching file calendar/lightning/content/messenger-overlay-sidebar.xul
Hunk #1 FAILED at 42
1 out of 1 hunks FAILED -- saving rejects to file calendar/lightning/content/messenger-overlay-sidebar.xul.rej

I did the best I could, obviously for calendar-list-tree.xml there was nothing to fix.

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/7adb743c979a
part 1: Rename calendar-base-view.js to calendar-day-label.js. r=mkmelin
https://hg.mozilla.org/comm-central/rev/13cd5bcce746
part 2: Rename calendar-views.js to calendar-views-utils.js. r=mkmelin
https://hg.mozilla.org/comm-central/rev/8c9c30927876
part 3: Remove aArgs from calendar-views-utils.js. r=mkmelin
https://hg.mozilla.org/comm-central/rev/c3195cbed417
part 4: [de-xbl] Convert calendar view bindings. r=philipp

Comment 22

[Jorg K (CEST = GMT+2)]

Also landed with the wrong bug number:
https://hg.mozilla.org/comm-central/rev/92b567487475
Let mozmill tests work with multiple classes. r=mkmelin

Comment 23

[Paul Morris [:pmorris]]

Ah... sorry I didn't think about this mid-air collision, and thanks for resolving it. Also sorry for the bug number mix up. That issue first came up in the minimonth bug, and I forgot to update it. I'll be more careful with similar cases in the future.