Closed Bug 865478 Opened 8 years ago Closed 8 years ago

Refactor richgrid to return items instead of indices


(Firefox for Metro Graveyard :: Firefox Start, defect)

Windows 8
Not set


(Not tracked)

Firefox 23


(Reporter: ally, Unassigned)




(1 file, 2 obsolete files)

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.
Attached patch 2 methods needed for 789363 (obsolete) — Splinter Review
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.
Blocks: 789363
Priority: -- → P2
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
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 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))

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, elem => elem.url == aUrl);
Attachment #741880 - Flags: review?(mbrubeck) → review-
(In reply to Matt Brubeck (:mbrubeck) from comment #4)
>   return, elem => elem.url ==
> aUrl);

I forgot that we can now write this even shorter, as:

  return Array.filter(this.childen, elem => elem.url == aUrl);
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)
Attachment #741997 - Attachment is patch: true
Attachment #741997 - Attachment mime type: message/rfc822 → text/plain
Attachment #741997 - Flags: review?(mbrubeck) → review+
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:
Closed: 8 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → Firefox 23
Assignee: sfoster → nobody
Priority: P2 → --
QA Contact: sfoster
You need to log in before you can comment on or make changes to this bug.