749096 - use proper accessor functions for getting list rows instead of open-coding them in mail/base/content/FilterListDialog.js

Status: RESOLVED FIXED

(MailNews Core :: Filters, defect)

Description

[:aceman]

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]

There is also list.currentIndex (for the currently selected row).

Comment 2

[:aceman]

Actually, list.selectedIndex is better for some usage.

Comment 3

[:aceman]

Attachment: patch

Comment 4

[Magnus Melin [:mkmelin]]

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]

(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]

(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 [:mkmelin]]

(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 [:mkmelin]]

Comment on attachment 657393 [details] [diff] [review]
patch

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

r=mkmelin with nit addressed

Comment 9

[:aceman]

Attachment: patch v2

Thanks.

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/comm-central/rev/dfd106c0f965