Closed
Bug 865478
Opened 8 years ago
Closed 8 years ago
Refactor richgrid to return items instead of indices
Categories
(Firefox for Metro Graveyard :: Firefox Start, defect)
Tracking
(Not tracked)
RESOLVED
FIXED
Firefox 23
People
(Reporter: ally, Unassigned)
References
Details
Attachments
(1 file, 2 obsolete files)
5.65 KB,
patch
|
mbrubeck
:
review+
|
Details | Diff | Splinter Review |
From a #windev discussion. Richgrid currently returns & uses indexes for most operations of consumers on grids, when what the consumer really wants is the item, and does not care where in the grid it is. Since there is a bunch on pin/unpin/update etc based on grids, I propose we do this in two patches to minimize rebase/rot/removal pain all around. 1) Add the new methods that return items not indices. Leave existing ones alone. 2) let the pin/unpin/hide/restore/update work land rsilveira has said he might need a couple of the index based functions for that, so let's not rip that those out until he knows which ones he needs. 2a) Decide as a group which index based methods should stay (probably based on above) 3) Remove any unneeded index based functions, refactor call sites to use the new item based methods.
Reporter | ||
Comment 1•8 years ago
|
||
iirc, sam said he wanted this bug. This is the bit that I need to move on bug 789363, which can act as a starting point.
Updated•8 years ago
|
Priority: -- → P2
Comment 2•8 years ago
|
||
IMO any richgrid methods that run queries on the children should return a NodeList, so I lean towards a querySelectorAll implementation for the getItemsByUrl. I'm on the fence about whether that should be getItemsByValue or getItemsByUrl. The richgriditem does have a url property so this value attr == url association is already exposed so I guess its ok.
QA Contact: sfoster
Comment 3•8 years ago
|
||
I've implemented getItemsByUrl and removeItemByUrl. getItemsByUrl uses querySelectorAll and returns a nodelist. removeItemByUrl only removes the first match. You would need to do something like while (item = grid.removeItemByUrl(url, true)) { ... } to remove all matches. Note that like the other mutatation methods it takes a 2nd 'skip rearrange' param. As ally notes, this is part 1 to add the functionality we need. Part 2 (in a seperate ticket) will be to adjust existing code to use these hopefully-more-efficient methods, and remove the indices-based ones if they turn out to be not useful.
Attachment #741590 -
Attachment is obsolete: true
Attachment #741880 -
Flags: review?(mbrubeck)
Comment 4•8 years ago
|
||
Comment on attachment 741880 [details] [diff] [review] Add getItemsByUrl and removeItemByUrl methods to richgrid + tests >+ <method name="removeItemByUrl"> Sorry, "removeItemByUrl" seems a little too specialized to me. As I suggested in bug 789363, I'd prefer to add a more generic "removeItem" method. Instead of this: while (let item = grid.removeItemByUrl(url, true)) ; We would write this: for (let item of grid.getItemsByUrl(url)) grid.removeItem(item); I think this will end up being slightly less code overall (especially since removeItemAt can be replaced by removeItem, or implemented in terms of it), and the "removeItem" method should be more generically useful. >+ return this.querySelectorAll('richgriditem[value="'+aUrl+'"]'); I do like this implementation of getItemsByUrl. Or if we want to return an Array, we can do: return Array.prototype.filter.call(this.children, elem => elem.url == aUrl);
Attachment #741880 -
Flags: review?(mbrubeck) → review-
Comment 5•8 years ago
|
||
(In reply to Matt Brubeck (:mbrubeck) from comment #4) > return Array.prototype.filter.call(this.children, elem => elem.url == > aUrl); I forgot that we can now write this even shorter, as: return Array.filter(this.childen, elem => elem.url == aUrl);
Comment 6•8 years ago
|
||
Agreed on previous comments. This time we have a removeItem method, which is used internally also by removeItemAtIndex. Tests updated and pass.
Assignee: nobody → sfoster
Attachment #741880 -
Attachment is obsolete: true
Attachment #741997 -
Flags: review?(mbrubeck)
Updated•8 years ago
|
Attachment #741997 -
Attachment is patch: true
Attachment #741997 -
Attachment mime type: message/rfc822 → text/plain
Updated•8 years ago
|
Attachment #741997 -
Flags: review?(mbrubeck) → review+
Comment 7•8 years ago
|
||
I prefer getItemsByUrl to return a NodeList. Calling code can always turn it into an array with Array.slice if that's what you need. Landed on inbound: https://hg.mozilla.org/integration/mozilla-inbound/rev/34413bab9ad5
Comment 8•8 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/34413bab9ad5
Status: NEW → RESOLVED
Closed: 8 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → Firefox 23
Updated•7 years ago
|
Assignee: sfoster → nobody
Updated•7 years ago
|
Priority: P2 → --
QA Contact: sfoster
You need to log in
before you can comment on or make changes to this bug.
Description
•