865170 - Only one top site removed when more than one were selected

Status: RESOLVED FIXED

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

Description

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

If you select (cross-slide) more than one tile from the Top Sites grid and then click the "delete/remove" button, they should all be removed from the grid. Currently, only the last one selected is removed.

Comment 1

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

Attachment: TopSites methods modified to handle multiple selected sites

So apparently I simply punted on this originally. As we can select multiple tiles, the doActionOnSelectedTiles method needs to be able to call TopSite.* methods with one or more sites, and have the right thing happen. 

Currently, TopSites.removeSite, (pin/unpinSite, restoreSite etc.) sets the flag via NewTabUtils, and calls update to trigger repopulation of the grid. If we call removeSite in quick succession unknown things happen. 

ISTM there's a couple of solutions. What I've implemented here is having removeSite loop over its arguments, then call update() once. This works great. For symmetry though, the other methods should behave the same. That looks great for all but pinSite, which takes aSite, aPinIndex. In the patch I modified it so it treats its arguments as a strided list, so it loops over by 2 and can be called like pinSite(site1, index1, site2, index2, ..etc) That's possibly a bit weird. Naming-wise, I'm open to renaming the TopSites methods to plural forms if we decide to go that route. 

The other solution is to have some kind batch function that probably temporarily overrides TopSites.update with a no-op and then restores it and calls it once at the end. The existing methods would remain as-is in this solution. I can look into that if you prefer.

Comment 2

[Matt Brubeck (:mbrubeck)]

Comment on attachment 741284 [details] [diff] [review]
TopSites methods modified to handle multiple selected sites

Review of attachment 741284 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/metro/base/content/TopSites.js
@@ +81,5 @@
> +   */
> +  pinSite: function(aSite, aSlotIndex /*, aSite, aSlotIndex, ... */) {
> +    for(let i=0; i<arguments.length; i+=2){
> +      let aSite = arguments[i],
> +          aSlotIndex = arguments[1+i];

I'm basically okay with this, but I'm thinking it might be better to make these take array (or iterable) arguments, just to make things more explicit and avoid having to write "TopSites.*.apply(Topsites, ...)" everywhere.

Also, you could pass the sites and indices in two separate arrays here.

Comment 3

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

Attachment: Reviste TopSites.* methods to handle one or more sites as input

I went ahead and renamed the methods to be plural. It looks like only the tests needed to pass a single object in, we always have an array when calling these methods in doActionOnSelectedTiles. 
I've left the option of passing in a single object, but could be persuaded to ditch that and make it the callers responsibility to always pass an array. 

Tests updated and all pass for me, and seems to do the right thing when selecting multiple tiles and pin/unpinnig, block/unblocking.

Comment 4

[Matt Brubeck (:mbrubeck)]

Comment on attachment 741930 [details] [diff] [review]
Reviste TopSites.* methods to handle one or more sites as input

Review of attachment 741930 [details] [diff] [review]:
-----------------------------------------------------------------

(In reply to Sam Foster [:sfoster] from comment #3)
> I've left the option of passing in a single object, but could be persuaded
> to ditch that and make it the callers responsibility to always pass an
> array.

Yes, let's eliminate that.  It's currently unused (and untested) code, which I see as a good place for latent bugs to hide. :)

::: browser/metro/base/content/TopSites.js
@@ +83,5 @@
> +   */
> +  pinSites: function(aSites, aSlotIndices) {
> +    let sites = Array.isArray(aSites) ? aSites : [aSites];
> +    let indices = Array.isArray(aSlotIndices) ? aSlotIndices : [aSlotIndices];
> +    for (let i=0; i<sites.length && i<indices.length; i++){

Maybe throw an exception if (aSites.length != aSlotIndices.length)?

@@ +102,5 @@
> +   * @param aSites site or array of sites to be unpinned
> +   */
> +  unpinSites: function(aSites) {
> +    let sites = Array.isArray(aSites) ? aSites : [aSites];
> +    sites.forEach(function(aSite){

Just a reminder, you could use "for (let site of aSites)" or "aSites.forEach(site => {" if you want a slightly lighter syntax here.  Either one will save you from passing a "this" argument after the loop body.

Comment 5

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

Ok, I yanked the object param support and added the throw when those arrays don't match. On inbound: https://hg.mozilla.org/integration/mozilla-inbound/rev/089bfd3486f5

Comment 6

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/089bfd3486f5