865478 - Refactor richgrid to return items instead of indices

Status: RESOLVED FIXED

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

Description

[Allison Naaktgeboren :ally]

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.

Comment 1

[Allison Naaktgeboren :ally]

Attachment: 2 methods needed for 789363

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.

Comment 2

[Sam Foster [:sfoster] (he/him)]

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.

Comment 3

[Sam Foster [:sfoster] (he/him)]

Attachment: Add getItemsByUrl and removeItemByUrl methods to richgrid + tests

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.

Comment 4

[Matt Brubeck (: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))
    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);

Comment 5

[Matt Brubeck (:mbrubeck)]

(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

[Sam Foster [:sfoster] (he/him)]

Attachment: Add getItemsByUrl and removeItem methods to richgrid + tests

Agreed on previous comments. This time we have a removeItem method, which is used internally also by removeItemAtIndex. Tests updated and pass.

Comment 7

[Sam Foster [:sfoster] (he/him)]

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

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/34413bab9ad5