Closed Bug 460942 Opened 14 years ago Closed 13 years ago

richlistitem label property inconsistencies

Categories

(Toolkit :: XUL Widgets, defect)

x86
Linux
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla1.9.3a1

People

(Reporter: arno, Assigned: arno)

Details

Attachments

(1 file, 7 obsolete files)

Hi,
richlistitem label is supposed to concatenate label texts contained by item. but it does not work fine. See testcase
Attached patch patch v1 (obsolete) — Splinter Review
Assignee: nobody → arenevier
Attachment #344065 - Flags: review?(neil)
I think that the current binding takes care of all the common cases, and you can easily provide your own getter if your binding needs it. Enn, thoughts?
That was my thought too when I saw this bug. But item1 in the testcase seems like it should return a value, no?
> I think that the current binding takes care of all the common cases, and you
> can easily provide your own getter if your binding needs it.

Good point. But as neil deakin pointed out, current behaviour is still messy in common case: label are only counted if they are descendant of first node. So, I rewrote my patch by only managing common case.
Attachment #344065 - Attachment is obsolete: true
Attachment #344688 - Flags: review?(neil)
Attachment #344065 - Flags: review?(neil)
Comment on attachment 344688 [details] [diff] [review]
do not count childs of anonymous content

Punting the review to Enn because I don't have tests enabled.

>+
>+            var labels = this.getElementsByTagNameNS(XULNS, "label");
>+            for (var count = 0; count < labels.length; count++) {
>+                labelText += labels[count].value + " ";
>+            }
Why the extra blank line and {}s?

>+            if (labelText.length) // removes trailing space
>+                labelText = labelText.slice(0, -1)
You sliced off the trailing ; by mistake!
Attachment #344688 - Flags: review?(neil) → review?(enndeakin)
Attached patch fixes typo (obsolete) — Splinter Review
sorry for typos
Attachment #344688 - Attachment is obsolete: true
Attachment #344742 - Flags: review?(enndeakin)
Attachment #344688 - Flags: review?(enndeakin)
Attached patch also removes extra blank line (obsolete) — Splinter Review
Attachment #344052 - Attachment is obsolete: true
Attachment #344743 - Flags: review?(enndeakin)
Attachment #344742 - Flags: review?(enndeakin)
Attachment #344742 - Attachment is obsolete: true
Comment on attachment 344743 [details] [diff] [review]
also removes extra blank line

>+            if (labelText.length) // removes trailing space
>+              labelText = labelText.slice(0, -1);

Is there a reason to do this?

If so, there's a recently added (but not yet documented) trim() function that trims spaces which you can use instead.
Attachment #344743 - Flags: review?(enndeakin) → review+
Comment on attachment 344743 [details] [diff] [review]
also removes extra blank line

>+            if (labelText.length) // removes trailing space
>+              labelText = labelText.slice(0, -1);

OK, I see that this is just removing the space that was added at the end. It may be easier to just not add the space in that case, but no big deal.
Keywords: checkin-needed
(In reply to comment #9)
> It may be easier to just not add the space in that case, but no big deal.

return Array.map(labels, function(aLabel) aLabel.value).join(" ");
Whiteboard: [c-n: wait for comment 10, or not ?]
Version: unspecified → Trunk
Is a new patch necessary ?
It's an easy win in simpler code, so yes please. The only thing I'm not quite sure of is whether we can suppose that JavaScript 1.8 will always be available for this binding, so you might want to use the more conservative

function(aLabel) { return aLabel.value; }

instead of the new shorthand.
Attached patch patch v3 (uses Array.map) (obsolete) — Splinter Review
Attachment #344743 - Attachment is obsolete: true
Attachment #346346 - Flags: review?(neil)
Comment on attachment 346346 [details] [diff] [review]
patch v3 (uses Array.map)

>+            var labels = this.getElementsByTagNameNS(XULNS, "label");
>+            return Array.map(labels, 
>+                            function(aLabel) { return aLabel.value }
>+                            ).join(" ");
That's some weird wrapping. I'd wrap it like this:
            return Array.map(labels, function(aLabel) { return aLabel.value })
                        .join(" ");
OK, so I lied, I'd actually wrap it like this ;-)
            return Array.map(this.getElementsByTagNameNS(XULNS, "label"),
                             function(aLabel) { return aLabel.value })
                        .join(" ");
Attachment #346346 - Flags: review?(neil) → review+
Attached patch better indentation (obsolete) — Splinter Review
same patch with indenting as proposed by   neil
Attachment #346346 - Attachment is obsolete: true
Attachment #346548 - Flags: review+
Not quite. Please align dots (before map and join) and arguments (this... and function...).
Attachment #346548 - Flags: review+
Attachment #346548 - Attachment is obsolete: true
Attachment #347401 - Flags: review?
Keywords: checkin-needed
Attachment #347401 - Flags: review? → review?(neil)
Attachment #347401 - Flags: review?(neil) → review+
http://hg.mozilla.org/mozilla-central/rev/bb94e11d66a3
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Whiteboard: [c-n: wait for comment 10, or not ?]
Target Milestone: --- → mozilla1.9.3a1
You need to log in before you can comment on or make changes to this bug.