Last Comment Bug 749096 - use proper accessor functions for getting list rows instead of open-coding them in mail/base/content/FilterListDialog.js
: use proper accessor functions for getting list rows instead of open-coding th...
Status: RESOLVED FIXED
:
Product: MailNews Core
Classification: Components
Component: Filters (show other bugs)
: Trunk
: All All
: -- trivial (vote)
: Thunderbird 18.0
Assigned To: :aceman
:
Mentors:
Depends on: 103684 450302
Blocks:
  Show dependency treegraph
 
Reported: 2012-04-26 01:14 PDT by :aceman
Modified: 2012-09-11 17:22 PDT (History)
4 users (show)
ryanvm: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
patch (7.91 KB, patch)
2012-08-31 12:17 PDT, :aceman
mkmelin+mozilla: review+
Details | Diff | Splinter Review
patch v2 (7.91 KB, patch)
2012-09-11 13:30 PDT, :aceman
acelists: review+
Details | Diff | Splinter Review

Description :aceman 2012-04-26 01:14:14 PDT
Spin off from bug 450302 comment 48.

I have implemented new functionality in mail/base/content/FilterListDialog.js in already landed bugs but I have directly manipulated the (html/xul) elements of the filter list. It seems there are proper accessor functions for these operations. This bug is to use these more robust accessors throughout the whole file:
list.getItemAtIndex
list.selectItem
list.itemCount

Thanks to Axelg whose patch brought me to this idea.
Comment 1 :aceman 2012-04-26 13:15:48 PDT
There is also list.currentIndex (for the currently selected row).
Comment 2 :aceman 2012-08-31 12:17:00 PDT
Actually, list.selectedIndex is better for some usage.
Comment 3 :aceman 2012-08-31 12:17:36 PDT
Created attachment 657393 [details] [diff] [review]
patch
Comment 4 Magnus Melin 2012-09-05 13:12:17 PDT
Comment on attachment 657393 [details] [diff] [review]
patch

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

::: mail/base/content/FilterListDialog.js
@@ +195,5 @@
>      }
>      aFilter.enabled = !aFilter.enabled;
>  
>      // Now update the appropriate row
> +    let row = document.getElementById("filterList").getItemAtIndex(aIndex);

aIndex + 1, no?

@@ +310,5 @@
>    }
>    updateCountBox();
>  
>    // Select filter above previously selected if one existed, otherwise the first one.
> +  if (newSelectionIndex == -1 && list.itemCount)

please test against itemCount == 0. falsy tests are good in general but when testing for zero it's harder to read, imo.
Comment 5 :aceman 2012-09-05 13:25:24 PDT
(In reply to Magnus Melin from comment #4)
> ::: mail/base/content/FilterListDialog.js
> @@ +195,5 @@
> >      }
> >      aFilter.enabled = !aFilter.enabled;
> >  
> >      // Now update the appropriate row
> > +    let row = document.getElementById("filterList").getItemAtIndex(aIndex);
> 
> aIndex + 1, no?

No. That is the difference between .children and .getItemAtIndex. GetItemAtIndex ignores any headers and always returns the proper row elements. .children is just a hack and assumes there is exactly one element at the beginning representing the header that must be skipped. But that may change any time, that's why this bug. That is how I understand it.
 
> please test against itemCount == 0. falsy tests are good in general but when
> testing for zero it's harder to read, imo.

OK.
Comment 6 :aceman 2012-09-05 13:27:13 PDT
(In reply to Magnus Melin from comment #4)
> > +  if (newSelectionIndex == -1 && list.itemCount)
> 
> please test against itemCount == 0. falsy tests are good in general but when
> testing for zero it's harder to read, imo.

Actually this is testing for > 0. So should I change it?
Comment 7 Magnus Melin 2012-09-11 12:22:23 PDT
(In reply to :aceman from comment #6)
> (In reply to Magnus Melin from comment #4)
> > > +  if (newSelectionIndex == -1 && list.itemCount)
> > 
> > please test against itemCount == 0. falsy tests are good in general but when
> > testing for zero it's harder to read, imo.
> 
> Actually this is testing for > 0. So should I change it?

Or rather != 0 - but yes please :)
Comment 8 Magnus Melin 2012-09-11 12:23:07 PDT
Comment on attachment 657393 [details] [diff] [review]
patch

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

r=mkmelin with nit addressed
Comment 9 :aceman 2012-09-11 13:30:16 PDT
Created attachment 660211 [details] [diff] [review]
patch v2

Thanks.
Comment 10 Ryan VanderMeulen [:RyanVM] 2012-09-11 17:22:32 PDT
https://hg.mozilla.org/comm-central/rev/dfd106c0f965

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