Closed Bug 1544914 Opened 5 months ago Closed 4 months ago

[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

Categories

(Calendar :: Calendar Views, task)

task
Not set

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: mkmelin, Assigned: pmorris)

References

(Blocks 1 open bug)

Details

Attachments

(5 files, 8 obsolete files)

1.46 KB, patch
Details | Diff | Splinter Review
3.56 KB, patch
Details | Diff | Splinter Review
20.60 KB, patch
Details | Diff | Splinter Review
21.30 KB, patch
Details | Diff | Splinter Review
470.42 KB, patch
Details | Diff | Splinter Review

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...

Type: defect → task
Attached patch mozmill-multiple-classes-2.patch (obsolete) — Splinter Review

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

Attachment #9061893 - Flags: review?(mkmelin+mozilla)

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

Attachment #9061894 - Flags: review?(mkmelin+mozilla)

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.)

Attachment #9061898 - Flags: review?(mkmelin+mozilla)

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?)

Attachment #9061927 - Flags: review?(mkmelin+mozilla)

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.

Attachment #9061928 - Flags: review?(mkmelin+mozilla)
Attachment #9061893 - Flags: review?(mkmelin+mozilla) → review+
Attachment #9061894 - Flags: review?(mkmelin+mozilla) → review+
Attachment #9061898 - Flags: review?(mkmelin+mozilla) → review+
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

(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.

Flags: needinfo?(mkmelin+mozilla)

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.

Flags: needinfo?(mkmelin+mozilla)
Attachment #9061928 - Flags: review?(mkmelin+mozilla) → review+
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 :
Attachment #9061927 - Flags: review?(mkmelin+mozilla) → feedback+

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.

Attachment #9061928 - Attachment is obsolete: true

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.)

Attachment #9061927 - Attachment is obsolete: true
Attachment #9065233 - Flags: review?(philipp)
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!
Attachment #9065233 - Flags: review?(philipp) → review+

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.

Attachment #9061893 - Attachment is obsolete: true

Part 1 of 4.

Attachment #9061894 - Attachment is obsolete: true

Part 2 of 4.

Attachment #9061898 - Attachment is obsolete: true

Part 3 of 4.

Attachment #9065231 - Attachment is obsolete: true

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.)

Keywords: checkin-needed

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).

Attachment #9065233 - Attachment is obsolete: true
Attachment #9065941 - Attachment is obsolete: true

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.

Pushed by mozilla@jorgk.com:
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

Status: NEW → RESOLVED
Closed: 4 months ago
Keywords: checkin-needed
Resolution: --- → FIXED

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

Target Milestone: --- → 7.0

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.

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