Closed Bug 1554647 Opened 1 year ago Closed 1 year ago

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

Categories

(Calendar :: General, task)

task
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 69.0

People

(Reporter: mkmelin, Assigned: khushil324)

References

Details

Attachments

(2 files, 4 obsolete files)

Assignee: nobody → khushil324
Attachment #9071287 - Flags: review?(philipp)
Attachment #9071287 - Flags: feedback?(mkmelin+mozilla)
Status: NEW → ASSIGNED
Attachment #9071287 - Attachment is obsolete: true
Attachment #9071287 - Flags: review?(philipp)
Attachment #9071287 - Flags: feedback?(mkmelin+mozilla)
Attachment #9071523 - Flags: review?(philipp)
Attachment #9071523 - Flags: feedback?(mkmelin+mozilla)
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
Attachment #9071523 - Flags: review?(philipp)
Attachment #9071523 - Flags: feedback?(mkmelin+mozilla)
Attached image 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.

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

Attachment #9071523 - Attachment is obsolete: true
Attachment #9074112 - Flags: feedback?(mkmelin+mozilla)
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
Attachment #9074112 - Flags: feedback?(mkmelin+mozilla) → feedback+
Attachment #9074112 - Attachment is obsolete: true
Attachment #9074948 - Flags: review?(philipp)
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.
Attachment #9074948 - Flags: review?(philipp) → review+
Attachment #9074948 - Attachment is obsolete: true
Attachment #9075143 - Flags: review+
Keywords: checkin-needed

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

Status: ASSIGNED → RESOLVED
Closed: 1 year ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → 7.1
You need to log in before you can comment on or make changes to this bug.