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