Bug 1504416 (calendar-list-tree-bindings)

[de-xbl] Migrate calendar-list-tree, full-calendar-list-tree, and gdata-list-tree to custom element.

RESOLVED FIXED in 7.0

Status

task
RESOLVED FIXED
9 months ago
Last month

People

(Reporter: arshad, Assigned: pmorris)

Tracking

(Blocks 1 bug)

Trunk
Dependency tree / graph

Details

Attachments

(2 attachments, 7 obsolete attachments)

No description provided.
Summary: [de-xbl] Migrate calendar-list-tree to custom element. → [de-xbl] Migrate calendar-list-tree and full-calendar-list-tree to custom element.
Summary: [de-xbl] Migrate calendar-list-tree and full-calendar-list-tree to custom element. → [de-xbl] Migrate calendar-list-tree, full-calendar-list-tree, and gdata-list-tree to custom element.
Alias: calendar-list-tree-bindings
Comment on attachment 9022403 [details] [diff] [review]
calendar-list-tree-bindings-WIP.patch

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

::: calendar/base/content/widgets/calendar-list-tree.js
@@ +9,5 @@
> +        return ["hidecolumnpicker", "hideheader", "cycler", "childtooltip, childcontext"];
> +    }
> +
> +    _getChildrenString(elemArr) {
> +        if (Array.isArray(elemArr) && elemArr.length > 0) {

looks like you could do

if (!Array.isArray(elemArr)) {
  return this.innerHTML;
}

... and then go on with the array case

@@ +32,5 @@
> +                    <treecol anonid="checkbox-treecol" inheritAttr="true" cycler="true" hideheader="true" width="17"></treecol>
> +                    <treecol anonid="color-treecol" inheritAttr="true" hideheader="true" width="16"></treecol>
> +                    <treecol anonid="calendarname-treecol" inheritAttr="true" hideheader="true" label="&calendar.unifinder.tree.calendarname.label;" flex="1"></treecol>
> +                    <treecol anonid="status-treecol" inheritAttr="true" hideheader="true" width="18"></treecol>
> +                    ${this._getChildrenString(["treecol"])}

$?

@@ +79,5 @@
> +            onModifyItem: (aNewItem, aOldItem) => { },
> +            onDeleteItem: (aDeletedItem) => { },
> +            onError: (aCalendar, aErrNo, aMessage) => { },
> +
> +            onPropertyChanged: (aCalendar, aName, aValue, aOldValue) => {

I'd remove the a-namings while converting.

@@ +421,5 @@
> +        } else if (document.popupNode && document.popupNode.contextCalendar) {
> +            // Otherwise, we can try to get the context calendar from the popupNode.
> +            return document.popupNode.contextCalendar;
> +        }
> +        return aRow && aRow.value > -1 && this.mCalendarList[aRow.value];

would add parenthesis around (aRow.value > -1) for readability

::: common/content/customElements.js
@@ +9,5 @@
>  for (let script of [
>    "chrome://messenger/content/mailWidgets.js",
>    "chrome://messenger/content/generalBindings.js",
>    "chrome://messenger/content/statuspanel.js",
> +  "chrome://calendar/content/widgets/calendar-list-tree.js",

shouldn't be loaded here but by lightning
(In reply to Magnus Melin [:mkmelin] from comment #3)

> ... and then go on with the array case
> 
> @@ +32,5 @@
> > +                    <treecol anonid="checkbox-treecol" inheritAttr="true" cycler="true" hideheader="true" width="17"></treecol>
> > +                    <treecol anonid="color-treecol" inheritAttr="true" hideheader="true" width="16"></treecol>
> > +                    <treecol anonid="calendarname-treecol" inheritAttr="true" hideheader="true" label="&calendar.unifinder.tree.calendarname.label;" flex="1"></treecol>
> > +                    <treecol anonid="status-treecol" inheritAttr="true" hideheader="true" width="18"></treecol>
> > +                    ${this._getChildrenString(["treecol"])}
> 
> $?
$ is used to interpolate variables in template string.. ex - `asd${pi}` = `asd3.14`
> 
> @@ +79,5 @@
> > +            onModifyItem: (aNewItem, aOldItem) => { },
> > +            onDeleteItem: (aDeletedItem) => { },
> > +            onError: (aCalendar, aErrNo, aMessage) => { },
> > +
> > +            onPropertyChanged: (aCalendar, aName, aValue, aOldValue) => {
> 
> I'd remove the a-namings while converting.
> 
I haven't removed anonid yet, is this naming convention because of anonid?

> ::: common/content/customElements.js
> @@ +9,5 @@
> >  for (let script of [
> >    "chrome://messenger/content/mailWidgets.js",
> >    "chrome://messenger/content/generalBindings.js",
> >    "chrome://messenger/content/statuspanel.js",
> > +  "chrome://calendar/content/widgets/calendar-list-tree.js",
> 
> shouldn't be loaded here but by lightning

how do i do that?
(In reply to Arshad Khan [:arshad] from comment #4)
> > > +            onPropertyChanged: (aCalendar, aName, aValue, aOldValue) => {
> > 
> > I'd remove the a-namings while converting.
> > 
> I haven't removed anonid yet, is this naming convention because of anonid?

Hungarian notation denoting a for argument. Doesn't make much sense in JavaScript really since you're not supposed to modify input to be "out parameters".

> > ::: common/content/customElements.js
> > @@ +9,5 @@
> > >  for (let script of [
> > >    "chrome://messenger/content/mailWidgets.js",
> > >    "chrome://messenger/content/generalBindings.js",
> > >    "chrome://messenger/content/statuspanel.js",
> > > +  "chrome://calendar/content/widgets/calendar-list-tree.js",
> > 
> > shouldn't be loaded here but by lightning
> 
> how do i do that?

You add it as a script at the place from the relevant xul window/overlay.
Pausing the work on this bug as of now because of the intermittent QueryInterface errors and no tree showing error.
Attachment #9022403 - Attachment is obsolete: true
Attachment #9026725 - Attachment is obsolete: true
Depends on: 1446341
Attachment #9027088 - Attachment is obsolete: true
Attachment #9027088 - Attachment is patch: false
Attachment #9027088 - Attachment is patch: true
Comment on attachment 9027088 [details] [diff] [review]
calendar-list-tree-bindings.patch

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

A few drive-by comments:

I guess removing the tree is part of a different bug? I'm concerned though that if we make a lot of adaptions in this bug to migrate to a CE with a tree, we'll have to throw everything overboard again when creating a CE without a xul tree. There probably won't be much matching code.

I'd almost suggest to re-write the calendar list as a CE from scratch, exposing relevant features.


> > +            onPropertyChanged: (aCalendar, aName, aValue, aOldValue) => {
> 
> I'd remove the a-namings while converting.

a-namings are pretty common in calendar/ actually, to differentiate between arguments and local variables.

::: calendar/base/content/widgets/calendar-list-tree.js
@@ +5,5 @@
> +/* global MozXULElement */
> +
> +class MozCalendarListTree extends MozXULElement {
> +    static get observedAttributes() {
> +        return ["hidecolumnpicker", "hideheader", "cycler", "childtooltip, childcontext"];

The hideheader attribute et al are specific to the tree usage, if we can get rid of the tree then we likely don't need to expose the hideheader attribute. The only ones that might be relevant outside of the element are childtooltip and childcontext.

@@ +10,5 @@
> +    }
> +
> +    _getChildrenString(elemArr) {
> +        if (!Array.isArray(elemArr) || elemArr.length === 0) {
> +            return this.innerHTML;

Use of innerHTML in general is a performance issue, if we can avoid using this it would be preferable.

@@ +31,5 @@
> +                <treecols anonid="treecols" inheritAttr='true' hideheader="true">
> +                    <treecol anonid="checkbox-treecol" inheritAttr="true" cycler="true" hideheader="true" width="17"></treecol>
> +                    <treecol anonid="color-treecol" inheritAttr="true" hideheader="true" width="16"></treecol>
> +                    <treecol anonid="calendarname-treecol" inheritAttr="true" hideheader="true" label="&calendar.unifinder.tree.calendarname.label;" flex="1"></treecol>
> +                    <treecol anonid="status-treecol" inheritAttr="true" ${isGdataListTree? "primary='true'": ""} hideheader="true" width="18"></treecol>

gdata specific code shouldn't be in the main binding. Then you should rather have a custom element in the gdata provider that extends this.
Component: General → Calendar Views
Product: Thunderbird → Calendar

Over to Paul for this one.

Assignee: arshdkhn1 → paul
Status: ASSIGNED → NEW
No longer depends on: 1446341

This is ready for review. There's a lot here. I considered doing the gdata tree separately but it uses the same CSS, which I'd already changed, so I went ahead and did it with the rest.

Tests are passing locally (linux), and I manually tested:

  • the main (full) tree (the main calendar list in the sidebar in calendar/tasks tabs)
  • gdata tree (in the dialog for creating a new google calendar)

I wasn't able to manually test the 3rd usage of this tree, which (I think) should appear when you uninstall the google (gdata) provider add-on. For some reason there is no uninstall button.

Note that I'm handling attribute inheritance manually for now (see bug 1545824).

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

A patch to remove (de-overlay) the calendar-calendars-list.xul overlay file by merging it into messenger-overlay-sidebar.xul. This overlay was only used in this one place so removing it simplifies things. This patch should be applied after the de-xbl patch.

Attachment #9059936 - Flags: review?(mkmelin+mozilla)
Status: NEW → ASSIGNED
Comment on attachment 9059935 [details] [diff] [review]
calendar-list-tree-de-xbl-0.patch

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

It all seems too work fine, someone from calendar/ can do the final review. Maybe you could add more documentation on the classes of what these widgets "typically" is in our usage.

::: calendar/base/content/widgets/calendar-list-tree.js
@@ +45,2 @@
>  
> +            // The first class of each treecol is used to identify it, add additional classes after.

let's not rely on the first class being the "id" (and then remove this comment)

@@ +186,3 @@
>  
> +            /**
> +             * Observer of changes to calendars.  Each registered calendar uses this observer.

please remove the double space

@@ +402,5 @@
>  
> +        /**
> +         * Add the passed calendar to the composite calendar to ensure that it is visible.
> +         *
> +         * @param calendar

please add type and comment

@@ +414,5 @@
> +         *
> +         * @param {string} className    The class to query by.
> +         * @return                      The column.
> +         */
> +        getColumn(className) {

I suggest to inline this method, it's a oneliner in the new situation, and used only twice

@@ +549,5 @@
> +        /**
> +         * Update a calendar's tree row (to refresh the color and such).
> +         *
> +         * @param calendar     The calendar to update.
> +         */

could add type here too (and in a few other places)

@@ +604,3 @@
>  
> +        /**
> +         * Get a calendar asoociated with an event.  If a row or column is passed as an argument,

double space

@@ +835,5 @@
> +        /**
> +         * Called to link the tree to the tree view.  A null argument un-sets/un-links the tree.
> +         * Performs initial load of calendars if needed and if possible.
> +         *
> +         * @param {Object | null} tree

{?Object}

or well, probably something more specific?

@@ +967,5 @@
> +             *
> +             * @implements calICalendarManagerObserver
> +             */
> +            this.calMgrObserver = {
> +                listTree: this,

Remove this, and in stead use array as I'll write below. Then this will be correct. same for the others below

@@ +972,5 @@
> +                QueryInterface: ChromeUtils.generateQI([Ci.calICalendarManagerObserver]),
> +
> +                // calICalendarManagerObserver Methods
> +
> +                onCalendarRegistered: function(calendar) {

onCalendarRegistered: (calendar) => {
Attachment #9059935 - Flags: review?(mkmelin+mozilla) → feedback+
Type: enhancement → task
Attachment #9059936 - Flags: review?(mkmelin+mozilla) → review+

Thanks for the review. I'll make those changes. One question below...

(In reply to Magnus Melin [:mkmelin] from comment #13)

@@ +967,5 @@

  •         *
    
  •         * @implements calICalendarManagerObserver
    
  •         */
    
  •        this.calMgrObserver = {
    
  •            listTree: this,
    

Remove this, and in stead use array as I'll write below. Then this will be
correct. same for the others below

Can you say more here? I'm not sure what you'd like me to remove (the @implements part?) and what you mean about using array instead.

I meant the part about listTree: this

When you use onCalendarRegistered: (calendar) => {

.... then the this will be properly defined inside and you don't need to set listTree: this just to access the proper this

Ah, I see, using arrow functions for different 'this' behavior. Thanks for clarifying. I made that change all through the file, but then I got JS errors in the console like:

JavaScript error: chrome://calendar/content/widgets/calendar-list-tree.js, line 234: TypeError: this.onPropertyChanged is not a function

Not sure why... So I changed it back to using the listTree property. Although that's less concise, on the plus side, it has the advantage of being more explicit.

I've made the other changes and also switched to using the shorthand method syntax without "function" keyword. Rebased on current trunk. Mozmill tests all pass locally.

::: calendar/base/content/widgets/calendar-list-tree.js
@@ +45,2 @@

  •        // The first class of each treecol is used to identify it, add additional classes after.
    

let's not rely on the first class being the "id" (and then remove this
comment)

Ok, makes sense. I've used ids instead. Often in this code we have a reference to the element and need to find out what kind of element it is, and classes don't work well for that (since there might be more than one).

@@ +414,5 @@

  •     *
    
  •     * @param {string} className    The class to query by.
    
  •     * @return                      The column.
    
  •     */
    
  •    getColumn(className) {
    

I suggest to inline this method, it's a oneliner in the new situation, and
used only twice

I can see pros and cons to either way of doing this. I've inlined it, as suggested.

Attachment #9059935 - Attachment is obsolete: true
Attachment #9061462 - Flags: review?(philipp)

Regarding using ids: you can't do that, since the document might contain many widgets and these "internal" ids shine through. The emphasis was on first class.

(In reply to Magnus Melin [:mkmelin] from comment #17)

Regarding using ids: you can't do that, since the document might contain many widgets and these "internal" ids shine through. The emphasis was on first class.

Ok, I see. I'm aware of the duplicate id problem, and I've been using classes to avoid it. In this case I think there's (currently...) only one instance of the widget per document (it's used in dialog windows), so that's why I thought it would be okay (though not ideal...) to use ids.

But it would be better to avoid them. To use classes, in any order, I'll just have to do something like this:

function getElementType(element) {
  const classesUsedAsIds = ['one', 'two', 'three'];

  for (const className of element.classList) {
    if (classesUsedAsIds.contains(className) {
      return className;
    }
  }
  return "";
}

I'll do that and upload another patch.

If you really need to know the type, you can always store it as a type attribute.
I don't like using custom attributes when it should be css, but there are of course usecases for it too.

Ah, that's a good solution. I'll do that.

Ok, this time using a custom 'type' attribute in place of 'id' for identifying columns. Mozmill tests pass locally.

Attachment #9061462 - Attachment is obsolete: true
Attachment #9061462 - Flags: review?(philipp)
Attachment #9061527 - Flags: review?(philipp)
Comment on attachment 9061527 [details] [diff] [review]
calendar-list-tree-de-xbl-2.patch

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

For future patches maybe you could give me a diff between after the XBL converter ran and the final state. I saw a few things I would normally have flagged, but these were likely at the same as before. For example, there are a few places where template strings would make the code more readable.
Attachment #9061527 - Flags: review?(philipp) → review+

Good idea about a diff to capture what changes are the XBL converter and which are after the converter run. I'll plan on doing that.

Here's the first of the two patches, rebased on current trunk. Mozmill calendar tests pass locally (linux). Ready for checkin. (Let me know if another try server run is needed.)

Attachment #9061527 - Attachment is obsolete: true

And here's the second of the two patches, rebased on trunk.

Attachment #9059936 - Attachment is obsolete: true
Keywords: checkin-needed

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

Thes patches are in conflict with bug 1544914, see comment there. It would be nice to indicate which bug goes first.

Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/ebe4366fb0a3
[de-xbl] Migrate calendar-list-tree bindings to custom elements. r=philipp
https://hg.mozilla.org/comm-central/rev/844558b1ed2e
De-overlay calendar-calendars-list.xul. r=mkmelin

Status: ASSIGNED → RESOLVED
Closed: 2 months ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → 7.0
Regressions: 1558084
You need to log in before you can comment on or make changes to this bug.