Closed
Bug 460942
Opened 16 years ago
Closed 15 years ago
richlistitem label property inconsistencies
Categories
(Toolkit :: UI Widgets, defect)
Tracking
()
RESOLVED
FIXED
mozilla1.9.3a1
People
(Reporter: arno, Assigned: arno)
Details
Attachments
(1 file, 7 obsolete files)
3.54 KB,
patch
|
neil
:
review+
|
Details | Diff | Splinter Review |
Hi, richlistitem label is supposed to concatenate label texts contained by item. but it does not work fine. See testcase
Assignee | ||
Comment 1•16 years ago
|
||
Assignee: nobody → arenevier
Attachment #344065 -
Flags: review?(neil)
Comment 2•16 years ago
|
||
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?
Comment 3•16 years ago
|
||
That was my thought too when I saw this bug. But item1 in the testcase seems like it should return a value, no?
Assignee | ||
Comment 4•16 years ago
|
||
> 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 5•16 years ago
|
||
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)
Assignee | ||
Comment 6•16 years ago
|
||
sorry for typos
Attachment #344688 -
Attachment is obsolete: true
Attachment #344742 -
Flags: review?(enndeakin)
Attachment #344688 -
Flags: review?(enndeakin)
Assignee | ||
Comment 7•16 years ago
|
||
Attachment #344052 -
Attachment is obsolete: true
Attachment #344743 -
Flags: review?(enndeakin)
Assignee | ||
Updated•16 years ago
|
Attachment #344742 -
Flags: review?(enndeakin)
Assignee | ||
Updated•16 years ago
|
Attachment #344742 -
Attachment is obsolete: true
Comment 8•16 years ago
|
||
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.
Updated•16 years ago
|
Attachment #344743 -
Flags: review?(enndeakin) → review+
Comment 9•16 years ago
|
||
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.
Assignee | ||
Updated•16 years ago
|
Keywords: checkin-needed
Comment 10•16 years ago
|
||
(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(" ");
Updated•16 years ago
|
Whiteboard: [c-n: wait for comment 10, or not ?]
Version: unspecified → Trunk
Assignee | ||
Comment 11•16 years ago
|
||
Is a new patch necessary ?
Comment 12•16 years ago
|
||
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.
Assignee | ||
Comment 13•16 years ago
|
||
Attachment #344743 -
Attachment is obsolete: true
Attachment #346346 -
Flags: review?(neil)
Comment 14•16 years ago
|
||
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+
Assignee | ||
Comment 15•16 years ago
|
||
same patch with indenting as proposed by neil
Attachment #346346 -
Attachment is obsolete: true
Attachment #346548 -
Flags: review+
Comment 16•16 years ago
|
||
Not quite. Please align dots (before map and join) and arguments (this... and function...).
Assignee | ||
Updated•16 years ago
|
Attachment #346548 -
Flags: review+
Assignee | ||
Comment 17•16 years ago
|
||
Attachment #346548 -
Attachment is obsolete: true
Attachment #347401 -
Flags: review?
Updated•16 years ago
|
Keywords: checkin-needed
Updated•15 years ago
|
Attachment #347401 -
Flags: review? → review?(neil)
Updated•15 years ago
|
Attachment #347401 -
Flags: review?(neil) → review+
Comment 18•15 years ago
|
||
http://hg.mozilla.org/mozilla-central/rev/bb94e11d66a3
Status: NEW → RESOLVED
Closed: 15 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.
Description
•