The default bug view has changed. See this FAQ.

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

RESOLVED FIXED in Thunderbird 18.0

Status

MailNews Core
Filters
--
trivial
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: aceman, Assigned: aceman)

Tracking

Trunk
Thunderbird 18.0
Dependency tree / graph
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

7.91 KB, patch
aceman
: review+
Details | Diff | Splinter Review
(Assignee)

Description

5 years ago
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.
(Assignee)

Comment 1

5 years ago
There is also list.currentIndex (for the currently selected row).
(Assignee)

Updated

5 years ago
Depends on: 103684
(Assignee)

Comment 2

5 years ago
Actually, list.selectedIndex is better for some usage.
Status: NEW → ASSIGNED
Component: Search → Filters
Product: Thunderbird → MailNews Core
(Assignee)

Comment 3

5 years ago
Created attachment 657393 [details] [diff] [review]
patch
Attachment #657393 - Flags: review?(mkmelin+mozilla)

Comment 4

5 years ago
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.
(Assignee)

Comment 5

5 years ago
(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.
(Assignee)

Comment 6

5 years ago
(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

5 years ago
(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

5 years ago
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+
(Assignee)

Comment 9

5 years ago
Created attachment 660211 [details] [diff] [review]
patch v2

Thanks.
Attachment #657393 - Attachment is obsolete: true
Attachment #660211 - Flags: review+
(Assignee)

Updated

5 years ago
Keywords: checkin-needed
https://hg.mozilla.org/comm-central/rev/dfd106c0f965
Status: ASSIGNED → RESOLVED
Last Resolved: 5 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.