[de-xbl] convert calendar-task-tree to custom element
Categories
(Calendar :: Tasks, enhancement)
Tracking
(Not tracked)
People
(Reporter: pmorris, Assigned: pmorris)
References
Details
Attachments
(2 files, 10 obsolete files)
|
7.21 KB,
patch
|
Details | Diff | Splinter Review | |
|
114.78 KB,
patch
|
Details | Diff | Splinter Review |
|
Assignee | |
Comment 1•5 years ago
|
||
Attaching a WIP patch for some feedback. Setting the view on the tree is not working for some reason. Opening a 'tasks' tab and clicking to focus on it gives:
JavaScript error: chrome://global/content/elements/tree.js, line 614: TypeError: this.view is null
And adding a task leads to this error:
JavaScript error: chrome://calendar/content/calendar-task-tree.js, line 644: TypeError: this.tree.view is null
In calendar-task-tree.js, in load function, lines 1075-1100, is where this is set up, but even right after setting 'tree.view' there, it is still null. I've tried various things, but no luck.
And it looks so easy here.
Also unrelated (I think) but setTree (line 841) does not appear to fire like it should.
|
||
Comment 2•5 years ago
|
||
Comment on attachment 9047209 [details] [diff] [review] wip-calendar-task-tree-0.patch Review of attachment 9047209 [details] [diff] [review]: ----------------------------------------------------------------- Didn't try tracking down the problem (yet), but I'd get rid of the "binding" property which is strange now (at the very least rename, if it's really needed) The take a look at everything that is trying to find anonymous content, that very likely needs to be replaced by something else now. ::: calendar/base/content/calendar-task-tree.js @@ +348,5 @@ > + > + > +// Wrap in a block to prevent leaking to window scope. > +{ > + class MozCalendarTaskTree extends customElements.get("tree") { please add jsdoc style documentation about what this widget is while you're here @@ +393,5 @@ > + connectedCallback() { > + if (this.delayConnectedCallback()) { > + return; > + } > + super.connectedCallback(); maybe this before the this.delayConnectedCallback() not sure it matters in practice @@ +400,5 @@ > + <tree class="calendar-task-tree" > + flex="1" > + enableColumnDrag="false" > + keepcurrentinview="true"> > + <treecols anonid="calendar-task-tree-cols"> please remove the anonids as that is basically an xbl thing. If we need to match something we usually just put a class on it and match that instead @@ +522,5 @@ > + * Attributes > + */ > + > + // back reference to the binding > + binding: this, this should probably be removed now in the process @@ +535,5 @@ > + return this.mSelectedColumn; > + }, > + > + set selectedColumn(aCol) { > + let tree = document.getAnonymousNodes(this.binding)[0]; getAnonymousNodes I think mean xbl internals, but you don't have those anymore @@ +536,5 @@ > + }, > + > + set selectedColumn(aCol) { > + let tree = document.getAnonymousNodes(this.binding)[0]; > + let treecols = tree.getElementsByTagNameNS(tree.namespaceURI, "treecol"); usually nowadays, let treecols = this.querySeletorAll("treecol"); That also works with forEach, so this.querySeletorAll("treecol").forEach((col) => { ....... }); @@ +567,5 @@ > + }, > + > + // Removes an array of old items from the list, and adds an array of new items if > + // they match the currently applied filter. > + modifyItems: function(aNewItems, aOldItems, aDontSort, aSelectNew) { while you're at it, make these methods the new style modifyItems(aNewItems, aOldItems, aDontSort, aSelectNew) { ... or if it's needed to get the correct this modifyItems: (aNewItems, aOldItems, aDontSort, aSelectNew) => { @@ +895,5 @@ > + */ > + onSelect: function(event) { }, > + > + onDoubleClick: function(event) { > + if (event.button == 0) { make an early return here @@ +917,5 @@ > + > + onKeyPress: function(event) { > + switch (event.key) { > + case "Delete": > + { I think usually angel backets go on the previous line, like case "Delete": { @@ +927,5 @@ > + } > + case " ": > + { > + if (this.tree.currentIndex > -1) { > + let col = document.getAnonymousElementByAttribute( more anonymous stuff. probably doesn't work now @@ +996,5 @@ > + > + QueryInterface: cal.generateQI([ > + Ci.calICompositeObserver, > + Ci.calIObserver > + ]), should fit on one line @@ +1074,5 @@ > + } > + > + load() { > + const { Services } = ChromeUtils.import("resource://gre/modules/Services.jsm"); > + // TODO: remove cal, likely unneeded here move services to the top too. it's in practice always already available @@ +1450,5 @@ > + calendarController.onSelectionChanged({ detail: focused ? this.selectedTasks : [] }); > + calendarController.todo_tasktree_focused = focused; > + } > + > + disconnectedCallback() { call super too?
|
|
Assignee | |
Comment 3•5 years ago
•
|
||
Thanks for the review. It was helpful. On a process note, some of those things I knew I needed to do, but just hadn't gotten to them yet because I was focusing on getting it working first (WIP status). But I assume from your review that it's best to go ahead and fix those kinds of things before uploading a patch. (After all, they might be related to the bigger problem, even if they seem unrelated.)
I've made some good progress on this, solving the main issue I had before. Now the tasks are displayed in the main task tab and there are no more JS errors in the console (at least so far as I've poked around).
There are still glitches: no tasks showing in the today pane, you can sort by a column, but then the task labels are not correctly aligned with their tasks (tooltips don't match, opening the task opens the task in the tooltip not the one in the label, etc.). So still WIP, more to do...
Part of what was tripping me up before was that in the XBL version there is a <tree> element that is the direct child of the "calendar-task-tree" element. But I was trying to inherit from the MozTree custom element (following the example of the places tree CE). So we either need to extend MozTree and not have a <tree> child, or have a <tree> child and not extend MozTree.
Since the code was written assuming a <tree> child, I've gone with that approach for now. More things were working that way. (Ideally I think the other way is simpler, but it probably makes sense to do that as a follow-up refactor after a fully working de-xbl pass.)
|
|
Assignee | |
Comment 4•5 years ago
•
|
||
I am not exactly thrilled by the eslint settings for object-shorthand. I found that I couldn't have an object that uses both the new method syntax and the old/standard property syntax (like "myProp: myVal"). ("error Unexpected mix of shorthand and non-shorthand properties.") That's needlessly limiting, IMHO.
(object-shorthand is currently set to "always" via mozilla/tools/lint/eslint/eslint-plugin-mozilla/lib/configs/recommended.js)
|
|
||
Comment 5•5 years ago
|
||
calendar has it's own eslint settings in calendar/.eslintrc.js
"object-shorthand": [2, "consistent"], // found in rules
Would be good to unify that more with the standard mozilla one where needed. Don't know if it makes a difference in this case.
Re the review: yes, it's easier to spot the real problem once the obvious problems are gone.
|
||
Comment 6•5 years ago
|
||
Yeah, I set object-shorthand it to consistent. This isn't one rule I'm too set about, but I do find it confusing to read. It always looks like something is missing:
{
data,
values: "foo",
otherthing
}
MakeMyDay may also have opinions about this.
|
|
Assignee | |
Comment 7•5 years ago
|
||
Yeah, I agree that consistency is good within non-method props, and within method props, but it would be nice to be able to use the new method syntax and old/standard non-method syntax in the same object. At a quick glance I didn't see a setting for that.
|
|
||
Comment 8•5 years ago
|
||
For things like the example in comment 6, personally I don't think that is much of problem. It's only certain cases where you can reasonably control all the parameter names (commonly one of the datas is passed on from an incoming parameter somewhere else). If in the data there is one value that you can or can't use shorthand for then all would need to change, which seems unfortunate.
|
|
Assignee | |
Comment 9•5 years ago
|
||
I've got the task tree basically working now, except for a few minor things I'm keeping notes on.
My next step is to do some refactoring to simplify the code: Move the tree view code into its own file. Extend the 'tree' custom element rather than having it as a child node, and then merge the 'treeWrapper' (formerly 'binding') and the 'tree' references. Also merge 'tree' and 'treebox' references since they seem to be the same thing. Give JS modules a try for this code.
The big glitches I was seeing before were the result of code getting crossed between the two instances of the tree (in the main task view and the today pane). UI events from one instance were being routed to the other one. (Rather than document.querySelector use this.querySelector)
There were also some things to figure out to get the priority and completed images to appear in the cells and column headings.
|
|
Assignee | |
Comment 10•5 years ago
|
||
This is getting much closer. The refactoring worked and the code is simpler. I have a couple of questions and some minor bugs remain.
-
In the setTree function (in calendar-task-tree-view.js) I've added a conditional that disables un-setting the tree (when setTree is passed a null argument (that's confusing, why didn't they just have an unsetTree function?)). Without the conditional the unifinder/todaypane tree does not appear. On launch setTree fires four times and the third one is with a null argument. So that must be disabling the unifinder tree. Firing four times doesn't seem right.
-
Currently I've added the new custom element to the pre-existing calendar-task-tree.js file, which is included in two XUL files. But we don't need to define this CE twice. This may affect the setTree issue. So we should move the existing utility functions to their own file, like 'calendar-task-tree-utils.js', so they can be included wherever needed without defining the CE more than once. But I'm not sure where the CE defining file should be included. (The organization of XUL overlays is still new and a bit mysterious to me.)
-
I'm pretty sure this fixes bug 432582 and so the comment referencing it can be removed.
-
We'll need a decision on the eslint setting.
Some papercut bugs I've noticed when testing:
-
'restore column order' doesn't work
-
'due in' sometimes shows "0 days" when that is not accurate. Seems related to setting the due date? I see this error:
JavaScript error: chrome://lightning/content/lightning-item-iframe.js, line 1159: TypeError: gStartTime is null -
text styling (green, red) for inprocess property is not consistently shown right after changes, but only shows up after restart.
-
right-click on a task, there are duplicate menuitems for open task and new task
-
error appears on mouse drag
JavaScript error: chrome://calendar/content/calendar-dnd-listener.js, line 604: NS_ERROR_XPC_BAD_CONVERT_JS: Could not convert JavaScript argument arg 1 [nsIDragService.invokeDragSession] -
change a property for a task with buttons in the view detail panel and the "delete" button is disabled
-
seems like more columns should be visible by default, per the list in the XUL files, (I only see three shown by default).
|
|
Assignee | |
Comment 11•5 years ago
|
||
Oh and there's also "calendar-task-tree-todaypane". The XUL binding wasn't being used before, as far as I can tell, and I haven't gotten the custom element to work yet. When I try to use it for the unifinder/todaypane tree, the tree does not appear.
|
|
Assignee | |
Comment 12•5 years ago
|
||
Another thing: I had to use <treecol is="treecol-image"> to get the images in the column headers (for priority and completed checkboxes). The old way wasn't working. But, that means there's no <image> element with a class for applying some mac-only css for styling the checkbox column header. Not sure how to solve this yet.
|
|
Assignee | |
Comment 13•5 years ago
|
||
This patch contains the "calendar-task-tree-utils.js" file re-arrangement described above in comment 10. It did not impact the setTree situation at all, but still seems like a good move.
|
|
Assignee | |
Comment 14•5 years ago
|
||
The main issues are solved, and basic functionality is working. By looking at the MozPlacesTree code, I found that setting up the tree view and the observers should happen not only with connectedCallback but whenever the content of the tree changes (when refresh is called). That solved the setTree / this.tree is null problem.
Here are some things that are still not working. I made a first attempt at fixing most of these, but so far the fixes have eluded me. I'll keep working on it, but wanted to post this update.
- toggling "show completed tasks" in today pane has no effect
- right-click on a task, there are duplicate menuitems for open task and new task
- error appears on mouse drag, dragging tasks doesn't work
JavaScript error: chrome://calendar/content/calendar-dnd-listener.js, line 604: NS_ERROR_XPC_BAD_CONVERT_JS: Could not convert JavaScript argument arg 1 [nsIDragService.invokeDragSession] - change a property for a task with buttons in the view detail panel and the "delete" button is disabled
- I think more columns should be visible by default, per the list in the XUL files, (I only see three shown by default).
- do we need the apparently unused "calendar-task-tree-todaypane"
- find a new way to apply mac-only css for styling the checkbox column header
|
|
||
Comment 15•5 years ago
|
||
Comment on attachment 9052669 [details] [diff] [review] wip-calendar-task-tree-5.patch Review of attachment 9052669 [details] [diff] [review]: ----------------------------------------------------------------- Just a few general comments. It appears indeed to be working ::: calendar/base/content/calendar-task-tree-view.js @@ +38,5 @@ > + return (this.mSelectedColumn = aCol); > + } > + > + /** > + * High-level task tree manipulation indention off. But perhaps don't use /** */ style comments except for specific documentation @@ +41,5 @@ > + /** > + * High-level task tree manipulation > + */ > + > + // Adds an array of items to the list if they match the currently applied filter. these should instead use jsdoc style comments though @@ +69,5 @@ > + let delItems = idiff.deletedItems; > + let addItems = idiff.addedItems; > + let modItems = idiff.modifiedItems; > + > + // find the indexes of the old items that need to be removed (we usually try to capitalize and add period for sentences) ::: calendar/base/content/calendar-task-tree.js @@ +138,5 @@ > + connectedCallback() { > + if (this.delayConnectedCallback()) { > + return; > + } > + super.connectedCallback(); maybe super call first? @@ +139,5 @@ > + if (this.delayConnectedCallback()) { > + return; > + } > + super.connectedCallback(); > + this.textContent = ""; or check hasChildNodes()?
|
|
Assignee | |
Comment 16•5 years ago
|
||
A separate patch to rename calendar-task-tree.js to calendar-task-tree-utils.js. To be applied before the main patch. This way calendar-task-tree.xml can be renamed to calendar-task-tree.js and history is preserved.
|
|
Assignee | |
Comment 17•5 years ago
•
|
||
Ready for review. Here are a few things that might make sense to handle in follow-up bugs.
-
Changes to the currently visible columns are not persisted across restarts.
-
Trying to drag a task doesn't work, produces an error.
-
A mac-only CSS / checkbox image issue. The way that I was able to get the checkbox images to appear, there's no longer an image element with the class "calendar-task-tree-col-completed-checkboximg".
Pre-existing bug:
- When start and due dates are the same day n weeks apart, "Due In" displays 0 days.
A few notes:
-
Why is "enableColumnDrag" set to false? It doesn't seem to prevent dragging columns.
-
I've used ids on two treecol elements in order to get the completed/checkbox and priority images to appear. If there's another way to do it, I wasn't able to figure it out.
-
This closes bug 432582. [Edit: nope, on closer inspection, it does not.]
|
|
||
Updated•5 years ago
|
|
|
||
Comment 18•5 years ago
|
||
Comment on attachment 9056791 [details] [diff] [review] calendar-task-tree-6.patch Review of attachment 9056791 [details] [diff] [review]: ----------------------------------------------------------------- Seems to work, and I don't find any new problems. For the patch, there's a bunch of aParams used which would be converted, and for the JSDoc there is some inconsistence in capitalizing string/Number etc. Not sure which is the "right" way. ::: calendar/base/content/calendar-task-tree-view.js @@ +313,5 @@ > + if (task.title) { > + return task.title.replace(/\n/g, " "); > + } else { > + return cal.l10n.getCalString("eventUntitled"); > + } while we're here, no else after return @@ +463,5 @@ > + /** > + * @param {Event} event > + */ > + onDoubleClick(event) { > + if (event.button == 0) { could do an early return here ::: calendar/base/content/calendar-task-tree.js @@ +109,5 @@ > + this.hasConnected = true; > + this.appendChild(MozXULElement.parseXULToFragment(` > + <treecols> > + <treecol is="treecol-image" > + id="calendar-task-tree-col-completed" I would keep is an id on the same row for greppability (and it's kind of part of the element definition) @@ +287,5 @@ > + } > + if (widths && widths.length > 0) { > + col.width = Number(widths.shift()); > + } > + if (sorted && sorted.length > 0) { sorted.length > 0 and sorted are the same @@ +363,1 @@ > if (aTask && aTask.dueDate && aTask.dueDate.isValid) { could do early return @@ +471,5 @@ > > + let composite = cal.view.getCompositeCalendar(window); > + composite.addObserver(this.mTaskTreeObserver); > + > + let branch = Services.prefs.getBranch(""); we don't need this temp variable
|
|
Assignee | |
Comment 19•5 years ago
|
||
Rebased on latest trunk.
|
|
Assignee | |
Comment 20•5 years ago
•
|
||
I've addressed Magnus' comments, rebased on trunk, and made a few more minor improvements while I was at it. Calendar mozmill tests are all passing locally. Magnus wrote:
For the patch, there's a bunch of aParams used which would be converted, and for the JSDoc there is some inconsistence in capitalizing string/Number etc. Not sure which is the "right" way.
I've removed the aParam style. On JSDoc capitalization, I've been following http://usejsdoc.org
I looked into the reason for the differences in capitalization, and it's due to JavaScript's naming primitive types with lowercase (string, boolean, number, null) and object types with uppercase (Object, Array, Event, String). (See section "2.3 A word on types" here: http://2ality.com/2011/08/jsdoc-intro.html ) So it seems the "right way" is uppercase or lowercase depending on the type. I checked and everything looks correct to me.
|
|
||
Updated•5 years ago
|
|
|
||
Comment 21•5 years ago
|
||
Comment on attachment 9057154 [details] [diff] [review] calendar-task-tree-7.patch Review of attachment 9057154 [details] [diff] [review]: ----------------------------------------------------------------- Pretty close, I think you can go ahead and fix these issues without a re-review. r=philipp with comments: ::: calendar/base/content/calendar-task-tree-utils.js @@ +73,5 @@ > handleTaskContextMenuStateChange(aEvent); > > + const treeNodeId = aEvent.target.triggerNode.closest(".calendar-task-tree").id; > + const isTodaypane = treeNodeId == "unifinder-todo-tree"; > + const isMainTaskTree = treeNodeId == "calendar-task-tree"; Any specific reason for const here? We've been using let for local variables elsewhere. ::: calendar/base/content/calendar-task-tree-view.js @@ +10,5 @@ > + * The tree view for a CalendarTaskTree. > + */ > +class CalendarTaskTreeView { > + /** > + * @param {CalendarTaskTree} taskTree Can you add a description for the param and a generic "Creates a new task tree view"? I know these are most annoying, but I'd like to be as complete as possible. @@ +22,5 @@ > + /** > + * @param {nsIIDRef} IID The IID to query for > + * @return {nsQIResult} The object queried for aIID > + */ > + QueryInterface(IID) { Though here is I guess where I break my own rules. QI is such an integral part of the platform that I don't think we need to document it at all. I'm also ok not documenting methods that are from some interface, e.g. nsITreeView. You can remove those. @@ +26,5 @@ > + QueryInterface(IID) { > + return cal.generateClassQI(this, IID, [Ci.nsITreeView]); > + } > + > + /** @type {Object} */ Can you check this file and complete the jsdoc? There are a few more instances of such comments. @@ +133,5 @@ > + if (doNotSort) { > + this.tree.recreateHashTable(); > + } else { > + this.tree.sortItems(); > + } Bug 1502923 makes some changes to sorting and reindexing, I wonder if we could apply this here as well. Separate bug though :) ::: calendar/base/content/calendar-task-tree.js @@ +406,4 @@ > > + refreshFromCalendar(calendar) { > + // TODO: Consider changing eslint defaults. > + // eslint-disable-next-line object-shorthand Let's either change the defaults or adapt :) ::: calendar/base/themes/osx/calendar-task-tree.css @@ +4,5 @@ > > @import url(chrome://calendar-common/skin/calendar-task-tree.css); > > .calendar-task-tree > treechildren::-moz-tree-image(calendar-task-tree-col-completed), > +/* TODO: calendar-task-tree-col-completed-checkboximg no longer exists */ If it no longer exists, can we remove the rule?
|
|
Assignee | |
Comment 22•5 years ago
|
||
Thanks for review. New patches on the way.
On 4/13/19 3:53 PM, Bugzilla@Mozilla wrote:
::: calendar/base/content/calendar-task-tree-utils.js
@@ +73,5 @@handleTaskContextMenuStateChange(aEvent);
- const treeNodeId = aEvent.target.triggerNode.closest(".calendar-task-tree").id;
- const isTodaypane = treeNodeId == "unifinder-todo-tree";
- const isMainTaskTree = treeNodeId == "calendar-task-tree";
Any specific reason for const here? We've been using let for local variables
elsewhere.
Just the standard functional programming reasons for preferring immutability to mutability. (Like how Rust defaults to immutable variables.) Basically, code is easier to understand when names consistently refer to the same thing and don't change on you. Making things const when they won't change and using let for things that do change alerts the reader to what will and won't change. When a function has most or all const then it is simpler to understand.
I've left these as const, let me know if I should change to let.
(Hmm, there's an eslint rule for this: prefer-const:
https://eslint.org/docs/rules/prefer-const#suggest-using-const-prefer-const
Maybe that's something we'd want to consider at some point?)
::: calendar/base/content/calendar-task-tree-view.js
@@ +10,5 @@
- The tree view for a CalendarTaskTree.
- */
+class CalendarTaskTreeView {- /**
* @param {CalendarTaskTree} taskTreeCan you add a description for the param and a generic "Creates a new task tree
view"? I know these are most annoying, but I'd like to be as complete as
possible.
Ok, done.
@@ +22,5 @@
- /**
* @param {nsIIDRef} IID The IID to query for* @return {nsQIResult} The object queried for aIID*/- QueryInterface(IID) {
Though here is I guess where I break my own rules. QI is such an integral part
of the platform that I don't think we need to document it at all.I'm also ok not documenting methods that are from some interface, e.g.
nsITreeView. You can remove those.
Ok, makes sense. I've left the JSDoc for setTree since that is important for understanding how the class connects to the task tree.
@@ +26,5 @@
- QueryInterface(IID) {
return cal.generateClassQI(this, IID, [Ci.nsITreeView]);- }
- /** @type {Object} */
Can you check this file and complete the jsdoc? There are a few more instances
of such comments.
Yep, done.
@@ +133,5 @@
if (doNotSort) {this.tree.recreateHashTable();} else {this.tree.sortItems();}Bug 1502923 makes some changes to sorting and reindexing, I wonder if we could
apply this here as well. Separate bug though :)
Hmm, I'll put it on my list to take a look.
::: calendar/base/content/calendar-task-tree.js
@@ +406,4 @@
refreshFromCalendar(calendar) {// TODO: Consider changing eslint defaults.// eslint-disable-next-line object-shorthandLet's either change the defaults or adapt :)
Ok, I have changed the settings to turn off this rule (nothing required for object-shorthand).
The closest setting to my preference is "methods" which requires the method shorthand, but not for properties. But setting it to "methods" raises 47 linting errors in two other files. So we should do that in a separate step if we do.
::: calendar/base/themes/osx/calendar-task-tree.css
@@ +4,5 @@@import url(chrome://calendar-common/skin/calendar-task-tree.css);
.calendar-task-tree > treechildren::-moz-tree-image(calendar-task-tree-col-completed),
+/* TODO: calendar-task-tree-col-completed-checkboximg no longer exists */If it no longer exists, can we remove the rule?
Ok, removed, and I'll open a new bug for fixing this mac-only checkbox CSS thing.
Also, I saw that "_getItemFromEvent" in the tree view was not actually a 'private' method, and was being called from the tree. So I changed it to "getItemFromEvent" so its name is now aligned with its usage.
|
|
Assignee | |
Comment 23•5 years ago
|
||
|
|
Assignee | |
Comment 24•5 years ago
•
|
||
Rebased on current trunk. Here's the try server run:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=af92475219e9e0612b1ead6fe6fc3d676742d717
|
|
Assignee | |
Comment 25•5 years ago
|
||
The try server run looks ok to me. I think these two patches are ready for checkin.
|
|
||
Comment 26•5 years ago
|
||
You can just add the checkin-needed when you think it's ready. (Try looks good to me too.)
|
||
Comment 27•5 years ago
|
||
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/765a945dbeb3
Rename calendar-task-tree.js to calendar-task-tree-utils.js. r=philipp
https://hg.mozilla.org/comm-central/rev/b6e519322394
[de-xbl] convert calendar-task-tree bindings to custom elements. r=philipp
|
||
Updated•5 years ago
|
|
||
Updated•4 years ago
|
Description
•