Remove use of nsISupportsArray in Calendar's treeview implementation [Error: TypeError: aProps is undefined]

RESOLVED FIXED in 2.4

Status

defect
--
blocker
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: Fallen, Assigned: mmecca)

Tracking

Lightning 2.2
Dependency tree / graph

Details

Attachments

(1 attachment)

Lightning still uses nsISupportsArray in various places. Support for this is going away soon after the merge, see bug 407956 for the nsITreeView part. Getting rid of it is fairly straightforward for the tree view.
Assignee

Comment 1

6 years ago
Bug 407956 has landed, the task list is currently broken on trunk with many instances of:

Error: TypeError: aProps is undefined
Source file: chrome://calendar/content/calendar-task-tree.xml
Line: 517
Assignee: nobody → matthew.mecca
Severity: normal → blocker
Assignee

Comment 2

6 years ago
Posted patch Fix v1Splinter Review
Attachment #721656 - Flags: review?(philipp)
Comment on attachment 721656 [details] [diff] [review]
Fix v1

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

Thanks for beating me to this. I was actually working on a such patch myself just a minute ago, but I had strange errors that aProps is undefined that were driving me crazy.

Did you test the patch? I had some missing icons in the calendar list on my own patch and I remember seeing a similar bug for chatzilla.


r=philipp with the following comments:

::: calendar/base/content/calendar-task-tree.xml
@@ +508,2 @@
>                }
> +              return "";

This can be shortened as:

return aCol.element.getAttribute("anonid") || "";

@@ +536,5 @@
>                // Task categories
>                var categories = item.getCategories({});
>                categories.map(formatStringForCSSRule)
>                          .map(getAtomFromService)
> +                        .forEach(properties.push, properties);

The getAtomFromService line is no longer needed, otherwise you are adding atoms.

I suggest:

properties = properties.concat(item.getCategories({}));

Then in the return line:

return properties.map(formatStringForCSSRule).join(" ");

and remove the formatStringForCSSRule in any other lines above.


If you don't agree (i.e perf reasons, the static strings don't need formatStringForCSSRule) I'd at least use concat() here and drop the getAtomFromService()

::: calendar/base/content/calendar-unifinder.js
@@ +704,5 @@
>          }
>  
>          // Task categories
>          item.getCategories({}).map(formatStringForCSSRule)
>                                .map(getAtomFromService)

Same getAtomFromService issue

::: calendar/base/content/widgets/calendar-list-tree.xml
@@ +647,2 @@
>            } else {
> +              properties.push("unchecked");

Since the lines are now shorter, you can condense this to

properties.push(composite.getCalendarById(calendar.id) ? "checked" : "unchecked");

@@ +655,5 @@
>            let color = (calendar.getProperty("color") || "").substr(1);
>  
>            // Set up the calendar color (background)
>            let bgColorProp = "color-" + (color || "default");
> +          properties.push(bgColorProp);

Since the lines are now shorter, you can also drop the intermediate bgColorProp variable and push directly.

Same for a few of the other props.
Attachment #721656 - Flags: review?(philipp) → review+
(In reply to Philipp Kewisch [:Fallen] from comment #3)
> Thanks for beating me to this. I was actually working on a such patch myself
> just a minute ago, but I had strange errors that aProps is undefined that
> were driving me crazy.
> 
> Did you test the patch? I had some missing icons in the calendar list on my
> own patch and I remember seeing a similar bug for chatzilla.

Argh, the error I was getting was actually an error with one of my extensions that hooked into the calendar list tree. Your patch was actually working quite fine. Remaining review comments still stand of course.
Summary: Remove use of nsISupportsArray in Calendar's treeview implementation → Remove use of nsISupportsArray in Calendar's treeview implementation [Error: TypeError: aProps is undefined]
Assignee

Comment 5

6 years ago
Pushed to comm-central - https://hg.mozilla.org/comm-central/rev/324320ba7679
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Whiteboard: [good first bug][mentor=Fallen][lang=js]
Target Milestone: --- → 2.4
You need to log in before you can comment on or make changes to this bug.