1554647 - [de-xbl] convert the calendar-event-column binding to custom element

Status: RESOLVED FIXED

(Calendar :: General, task)

Description

[Magnus Melin [:mkmelin]]

Convert the calendar-event-column binding to custom element.

https://searchfox.org/comm-central/rev/5a670c59f9004ef9be4874cfbfe57ec2ef3b260f/calendar/base/content/calendar-multiday-view.xml#23

Comment 1

[Khushil Mistry [:khushil324]]

Attachment: Bug-1554647_de-xbl_calendar-event-column.patch

Comment 2

[Khushil Mistry [:khushil324]]

Attachment: Bug-1554647_de-xbl_calendar-event-column.patch

Comment 3

[Magnus Melin [:mkmelin]]

Comment on attachment 9071523 [details] [diff] [review]
Bug-1554647_de-xbl_calendar-event-column.patch

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

Needs unbitrotting/adjustment from bug 1556797
Seems to work, I think. I'm not sure exactly what functionality needs more thorough testing

Trying to grab an event with the mouse I get this (possibly not due to this patch, but maybe):
TypeError: element is null 82 calendar-multiday-base-view.js:1439:38
    findColumnForClientPoint chrome://calendar/content/calendar-multiday-base-view.js:1439
    onEventSweepMouseMove chrome://calendar/content/calendar-event-column.js:1313

::: calendar/base/content/calendar-event-column.js
@@ +5,5 @@
> +"use strict";
> +
> +/* global MozXULElement, getSelectedCalendar, cal, getOtherOrientation, invokeEventDragSession */
> +
> +// This is loaded into all XUL windows. Wrap in a block to prevent

Not all XUL windows, just the once we include it to. Just keep the latter sentence

@@ +30,5 @@
> +                <stack flex="1" class="multiday-column-box-stack" style="min-width: 1px; min-height: 1px">
> +                    <box flex="1" class="multiday-column-bg-box" style="min-width: 1px; min-height: 1px"/>
> +                    <box class="multiday-column-top-box" flex="1" style="min-width: 1px; min-height: 1px" equalsize="always" mousethrough="always"/>
> +                    <box class="timeIndicator" mousethrough="always" hidden="true"/>
> +                    <box flex="1" class="fgdragcontainer" style="min-width: 1px; min-height: 1px; overflow:hidden;">

Off topic, but I don't understand when the drag indicators are supposed to show up. They sometimes do, sometimes it's impossible to get them to do so (same on trunk).

@@ +138,5 @@
> +            this.mDayStartMin = 8 * 60;
> +
> +            this.mDayEndMin = 17 * 60;
> +
> +            // an array of objects that contain information about the events that are to be

Capitalize An

@@ +160,5 @@
> +            this.mDragState = null;
> +
> +            this.mLayoutBatchCount = 0;
> +
> +

double empty line here. did you run the linter? I think it doesn't like that

@@ +189,5 @@
> +
> +            // mEventInfos.
> +            this.mSelectedChunks = [];
> +
> +            const { cal } = ChromeUtils.import("resource://calendar/modules/calUtils.jsm");

move this up to top level and remove the eslint global for it

@@ +357,5 @@
> +        internalDeleteEvent(aOccurrence) {
> +            let itemIndex = -1;
> +            let occ;
> +            for (let i in this.mEventInfos) {
> +                occ = this.mEventInfos[i].event;

declare it inside the loop since you're not using it elsewhere

@@ +366,5 @@
> +            }
> +
> +            if (itemIndex == -1) {
> +                return false;
> +            } else {

no else after return please

@@ +426,5 @@
> +            // 'durend' is always positive, instead 'durstart' might be negative
> +            // if the event starts one or more days before the date of this column
> +            let realStart_ = (durstart.days * 24 + durstart.hours) * 60 + durstart.minutes;
> +            realStart_ = durstart.isNegative ? -1 * realStart_ : realStart_;
> +            let realEnd_ = (durend.days * 24 + durend.hours) * 60 + durend.minutes;

let's not use the trailing _ named variable names....

@@ +527,5 @@
> +                if (theMin < this.mDayStartMin || theMin >= this.mDayEndMin) {
> +                    box.setAttribute("off-time", "true");
> +                }
> +
> +                // Carry forth the day relation

.

@@ +551,5 @@
> +            // fgbox is used for dragging events
> +            this.fgboxes.box.setAttribute("orient", orient);
> +            this.querySelector(".fgdragspacer").setAttribute("orient", orient);
> +
> +            // this one is set to otherorient, since it will contain

capitailize This and add an ending .

@@ +729,5 @@
> +                if (startComparison == 0) {
> +                    // If the items start at the same time, return the longer one
> +                    // first
> +                    return bEventInfo.layoutEnd.compare(aEventInfo.layoutEnd);
> +                } else {

no else after return please

@@ +1217,5 @@
> +            // scrollbar.
> +            let diffStart, diffEnd;
> +            let orient = event.target.getAttribute("orient");
> +            let scrollbox = document.getAnonymousElementByAttribute(
> +                event.target, "anonid", "scrollbox");

this one needed to change for the unbitrottin

@@ +1357,5 @@
> +            } else {
> +                mousePos = event.screenX - col.parentNode.screenX;
> +                sizeattr = "width";
> +            }
> +            // don't let mouse position go outside the window edges

Capitalize + .

@@ +1413,5 @@
> +                if (dragState.startMin >= dragState.limitEndMin) {
> +                    dragState.startMin = Math.ceil((dragState.limitEndMin - snapIntMin) / snapIntMin) * snapIntMin;
> +                }
> +            } else if (dragState.dragType == "modify-end") {
> +                // if we're modifying the end, the start time is fixed.

If

@@ +1427,5 @@
> +            }
> +            let currentOffset = shadowElements.offset;
> +            let currentShadows = shadowElements.shadows;
> +
> +            // now we can update the shadow boxes position and size

Capitalize + .

@@ +1466,5 @@
> +            window.removeEventListener("keypress", col.onEventSweepKeypress);
> +
> +            document.calendarEventColumnDragging = null;
> +
> +            // if the user didn't sweep out at least a few pixels, ignore

If

@@ +1729,5 @@
> +            window.addEventListener("mouseup", this.onEventSweepMouseUp);
> +            window.addEventListener("keypress", this.onEventSweepKeypress);
> +        }
> +
> +        // called by sibling columns to tell us to take over the sweeping

C

Comment 4

[Magnus Melin [:mkmelin]]

Attachment: eventcols.png

Restarting I notice there's something wrong, I think from this patch. There's some kind of gathering of events I added while testing, on top of the day.

The columns are also not lined up with the days, but that seem to be the case on trunk too.

Comment 5

[Magnus Melin [:mkmelin]]

Hmm, or not. Those events may have been there from some earlier testing...

Comment 6

[Khushil Mistry [:khushil324]]

Attachment: Bug-1554647_de-xbl_calendar-event-column.patch

Comment 7

[Magnus Melin [:mkmelin]]

Comment on attachment 9074112 [details] [diff] [review]
Bug-1554647_de-xbl_calendar-event-column.patch

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

Looks good, just some minor things. Please have Philipp do the final review.

::: calendar/base/content/calendar-event-column.js
@@ +7,5 @@
> +/* global MozXULElement, getSelectedCalendar, getOtherOrientation, invokeEventDragSession, currentView */
> +
> +// Wrap in a block to prevent leaking to window scope.
> +{
> +    const { cal } = ChromeUtils.import("resource://calendar/modules/calUtils.jsm");

please add a blank line after this

@@ +30,5 @@
> +                <stack flex="1" class="multiday-column-box-stack" style="min-width: 1px; min-height: 1px">
> +                    <box flex="1" class="multiday-column-bg-box" style="min-width: 1px; min-height: 1px"/>
> +                    <box class="multiday-column-top-box" flex="1" style="min-width: 1px; min-height: 1px" equalsize="always" mousethrough="always"/>
> +                    <box class="timeIndicator" mousethrough="always" hidden="true"/>
> +                    <box flex="1" class="fgdragcontainer" style="min-width: 1px; min-height: 1px; overflow:hidden;">

I know this is from the existing code, but can we move the inline style stuff into calendar-views.css

@@ +64,5 @@
> +                    this.focus();
> +                }
> +            });
> +
> +            this.addEventListener("click", (event) => {

please move this into the click handler above (let's keep it one handler per type)

@@ +85,5 @@
> +                    calendar.getProperty("capabilities.events.supported") === false) {
> +                    return;
> +                }
> +
> +                // Only start sweeping out an event if the left button was clicked

nit: add .

@@ +142,5 @@
> +            // An array of objects that contain information about the events that are to be
> +            // displayed. The contained fields are:
> +            // event:        The event that is to be displayed in a 'calendar-event-box'
> +            // layoutStart:  The 'start'-datetime object of the event in the timezone of the view
> +            // layoutEnd:    The 'end'-datetime object of the event in the timezone of the view.

can you add "- " for each of these list items

@@ +385,5 @@
> +
> +        // This function returns the start and end minutes of the occurrence
> +        // part in the day of this column, moreover, the real start and end
> +        // minutes of the whole occurrence (which could span multiple days)
> +        // relative to the time 0:00 of the day in this column.

please change to jsdoc style /** */

@@ +660,5 @@
> +                            }
> +                        } else {
> +                            let chunkBox = document.createXULElement("spacer");
> +                            chunkBox.setAttribute("context", this.getAttribute("context"));
> +                            chunkBox.setAttribute("style", "min-width: 1px; min-height: 1px;");

this too move to css file

Comment 8

[Khushil Mistry [:khushil324]]

Attachment: Bug-1554647_de-xbl_calendar-event-column.patch

Comment 9

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

Comment on attachment 9074948 [details] [diff] [review]
Bug-1554647_de-xbl_calendar-event-column.patch

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

I'm wondering if use of `hg copy` would allow preserving the history of the code moved from calendar-multiday-view to calendar-event-column. Could you check?

::: calendar/base/content/calendar-event-column.js
@@ +336,5 @@
> +        }
> +
> +        setAttribute(attr, val) {
> +            // this should be done using lookupMethod(), see bug 286629
> +            let ret = XULElement.prototype.setAttribute.call(this, attr, val);

I'm wondering if this can be done using `super.setAttribute(attr, val)` or if that would cause an infinite loop as well.

Comment 10

[Khushil Mistry [:khushil324]]

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

Comment 11

[Khushil Mistry [:khushil324]]

Attachment: Bug-1554647_de-xbl_calendar-event-column.patch

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/c1649b46c7a3
[de-xbl] convert the calendar-event-column binding to custom element. r=philipp