Last Comment Bug 747824 - Improve attachmentlist navigation
: Improve attachmentlist navigation
Status: RESOLVED FIXED
:
Product: Thunderbird
Classification: Client Software
Component: Mail Window Front End (show other bugs)
: Trunk
: All All
: -- normal (vote)
: Thunderbird 15.0
Assigned To: Jim Porter (:squib)
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-04-22 21:43 PDT by Jim Porter (:squib)
Modified: 2012-05-21 19:54 PDT (History)
1 user (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
Improve attachmentlist binding (7.60 KB, patch)
2012-04-22 21:43 PDT, Jim Porter (:squib)
bwinton: review+
Details | Diff | Review
Clean things up a bit more (12.15 KB, patch)
2012-05-12 18:15 PDT, Jim Porter (:squib)
bwinton: review+
Details | Diff | Review

Description Jim Porter (:squib) 2012-04-22 21:43:08 PDT
Created attachment 617384 [details] [diff] [review]
Improve attachmentlist binding

I noticed some bugs in the attachmentlist code when working on my Attachment Tab add-on related to margins. Specifically, adding a margin screws up navigating with the keyboard. Here's a patch to fix it, and a couple other little issues I came across.

I suppose I can write tests, but I feel like Mozmill is the wrong way to test XBL bindings... any ideas?
Comment 1 Blake Winton (:bwinton) (:☕️) 2012-04-24 10:49:43 PDT
Comment on attachment 617384 [details] [diff] [review]
Improve attachmentlist binding

>+++ b/mail/base/content/mailWidgets.xml
>@@ -67,60 +67,76 @@
>       <method name="getItemAtIndex">
>         <parameter name="index"/>
>         <body><![CDATA[
>-          return this.children[index] || null;
>+          if (index < this.children.length)

Should we also check for index >= 0?

Aside from that, it looks good.  r=me.
Comment 2 Jim Porter (:squib) 2012-05-12 18:15:20 PDT
Created attachment 623468 [details] [diff] [review]
Clean things up a bit more

Sorry to ask for review again, but looking at the code, I noticed that we were creating superfluous arrays because of the "children" property (whose name also conflicts with the member on DOM elements). I fixed these up, which should improve performance when there are lots of items in the attachment list.
Comment 3 Blake Winton (:bwinton) (:☕️) 2012-05-16 14:05:38 PDT
Comment on attachment 623468 [details] [diff] [review]
Clean things up a bit more

>+++ b/mail/base/content/mailWidgets.xml
>@@ -56,71 +58,90 @@
>       <method name="getItemAtIndex">
>         <parameter name="index"/>
>         <body><![CDATA[
>-          return this.children[index] || null;
>+          if (index >= 0 && index < this._childNodes.length)
>+            return this._childNodes[index];
>+          return null;
>         ]]></body>
>       </method>
>       <method name="getRowCount">
>         <body><![CDATA[
>-          return this.children.length;
>+          return this.itemCount;

Since we use this._childNodes.length up above, why not use it here, too?

Other than that, it seems fine, and passes all the tests, so r=me.

Thanks,
Blake.
Comment 4 Jim Porter (:squib) 2012-05-21 17:37:00 PDT
(In reply to Blake Winton (:bwinton - Thunderbird UX) from comment #3)
> >+++ b/mail/base/content/mailWidgets.xml
> >@@ -56,71 +58,90 @@
> >       <method name="getItemAtIndex">
> >         <parameter name="index"/>
> >         <body><![CDATA[
> >-          return this.children[index] || null;
> >+          if (index >= 0 && index < this._childNodes.length)
> >+            return this._childNodes[index];
> >+          return null;
> >         ]]></body>
> >       </method>
> >       <method name="getRowCount">
> >         <body><![CDATA[
> >-          return this.children.length;
> >+          return this.itemCount;
> 
> Since we use this._childNodes.length up above, why not use it here, too?

Fixed in the patch I'm about to push...
Comment 5 Jim Porter (:squib) 2012-05-21 19:54:51 PDT
Checked in: http://hg.mozilla.org/comm-central/rev/d63ef5d1ea12

Note You need to log in before you can comment on or make changes to this bug.