Closed Bug 749096 Opened 9 years ago Closed 9 years ago

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

Categories

(MailNews Core :: Filters, defect)

defect
Not set
trivial

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 18.0

People

(Reporter: aceman, Assigned: aceman)

References

Details

Attachments

(1 file, 1 obsolete file)

7.91 KB, patch
aceman
: review+
Details | Diff | Splinter Review
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.
There is also list.currentIndex (for the currently selected row).
Depends on: 103684
Actually, list.selectedIndex is better for some usage.
Status: NEW → ASSIGNED
Component: Search → Filters
Product: Thunderbird → MailNews Core
Attached patch patch (obsolete) — Splinter Review
Attachment #657393 - Flags: review?(mkmelin+mozilla)
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.
(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.
(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?
(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 on attachment 657393 [details] [diff] [review]
patch

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

r=mkmelin with nit addressed
Attachment #657393 - Flags: review?(mkmelin+mozilla) → review+
Attached patch patch v2Splinter Review
Thanks.
Attachment #657393 - Attachment is obsolete: true
Attachment #660211 - Flags: review+
Keywords: checkin-needed
https://hg.mozilla.org/comm-central/rev/dfd106c0f965
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Flags: in-testsuite-
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 18.0
You need to log in before you can comment on or make changes to this bug.