1324571 - Use cases of some multi-selection commands in Downloads Library not clear

Status: RESOLVED FIXED

(Firefox :: Bookmarks & History, defect)

Description

[Pavan Karthik [:matrixisreal]]

Bug filed after the following discussion with marco in the comment
https://bugzilla.mozilla.org/show_bug.cgi?id=1196395#c25

Open Library
goto Downloads 
Select Multiple Downloads and right click

The current available options are Remove From History,Open Containing Folder, Go to Download Page and Copy Download Link and Clear Downloads.

But the use case of command like "Copy Download link" is not clear.
Marco suggests to delete some commands on multiple selection if it has no use case.


Now once this is resolved,
https://bugzilla.mozilla.org/show_bug.cgi?id=1324564
has to be fixed to manage the plural forms, which i think might require some negotiations.

Comment 1

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

This requires ux feedback, maybe the Taipei team UX can evaluate it.
Paolo, could you please forward to the Taipei UX? It's not urgent, but it would be good to have an evaluation so maybe we can make this a mentored bug.

Comment 2

[:Paolo Amadini]

Morpheus, Bryant, do you have an opinion on this?

Comment 3

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

the question is whether multi-selection makes sense for this view, and if it makes sense, which options make more sense for multi-selection?
This could also end up stating that the status quo is fine, and in such case we still need to fix plural forms.

Comment 4

[Morpheus Chen [:morpheus] UX]

As I see it, the available options are only "remove from history", "copy download link" and "clear downloads", and others are disabled. In my opinion, "copy download link" helps users directly copy all links of selected downloads which seems to only serve a specific purpose: users can collect links and paste to a document at once. Besides, the term here is a bit ambiguous too. Taking all these into consideration, I would also recommend deleting "copy download link", "view in Finder" and "go to download page" when multiple-selected, or just making them disabled as others do.

Comment 5

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

IIRC in Places views we have the selectiontype attribute that should allow to define which commands are disabled/visible in case of multi selection. I think that's not supported by the downloads view, so maybe one solution could be to add such thing.

Comment 6

[Pavan Karthik [:matrixisreal]]

Hi Marco,
I am almost done with the path, I have some clarifications to make before I proceed further.

From what Morpheus suggests, I think disabling the items would be a better idea, as a context menu of just 2items doesn't look elegant.

I am a bit uncertain on which way to implement,
As you suggested, If we add a selection type attribute, we need to iterate over all the menu items and set the visibility(will use more resources), For that I couldnt figure out how/where the visiblity of 
"downloadsCmd_pauseResume"
"downloadsCmd_pauseResume"
"downloadsCmd_unblock"
are set. Please let me know.

The other way is to get the items   "copy download link", "view in Finder" and "go to download page" by
 --  adding ids(which are current not there)
 -- or just by commands

Do let me know which way to proceed.
thanks :)

Comment 7

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

(In reply to Pavan Karthik from comment #6)
> I am a bit uncertain on which way to implement,
> As you suggested, If we add a selection type attribute, we need to iterate
> over all the menu items and set the visibility(will use more resources), For
> that I couldnt figure out how/where the visiblity of 
> "downloadsCmd_pauseResume"
> "downloadsCmd_pauseResume"
> "downloadsCmd_unblock"
> are set. Please let me know.

I think these are hidden through CSS:
http://searchfox.org/mozilla-central/rev/39f45e8015056b1fc2de3f85250360666cf2d275/browser/components/downloads/content/downloads.css#129

But that approach is unlikely to work for multi-selection, unless we add an attribute on the richlistbox every time there is multi selection. That's a possibility but it sounds a little bit hackish and error-prone.

> The other way is to get the items   "copy download link", "view in Finder"
> and "go to download page" by
>  --  adding ids(which are current not there)
>  -- or just by commands

I think iterating wouldn't be a big deal from a perf point of view, and you could likely do that only for allDownloadsViewOverlay.xul that IIRC is used both for the Library and about:downloads. In the end opening a contextual menu isn't on the critical perf path.

I agree on disabling items rather than hiding them.

Comment 8

[Pavan Karthik [:matrixisreal]]

Attachment: v1.0.patch

Hi marco,
Sorry for showing up late,I was busy with college stuff.
So, I this patch, as you have mentioned I have added "selection type" 
attribute to set the disabled Attribute accordingly,
Right now my code just can recognize "single" and "multiple" values of selection type.
Let me know what you think about this.
Thanks :D

Comment 9

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Comment on attachment 8823240 [details] [diff] [review]
v1.0.patch

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

Out of curiosity, do these command go through isCommandEnabled in allDownloadsViewOverlay.js?
Cause if so, that would look like a more natural point where to disable commands. It would be worth checking that... That means we may not have to add "selectiontype", since we'd directly disable given commands.

Both solutions are valid, I guess it's just matter of comparing what's simpler.

Regardless, I'm posting some comments here in case in the end this is the only way forward.

::: browser/components/downloads/content/allDownloadsViewOverlay.js
@@ +1321,5 @@
>  
> +    let selectionSize = this._richlistbox.selectedItems.length;
> +    let contextMenu = document.getElementById("downloadsContextMenu");
> +    for (var i = 0; i < contextMenu.childNodes.length; ++i) {
> +      let item = contextMenu.childNodes[i];

I think this should also work:
for (let menuitem of contextMenu.childNodes) {

@@ +1322,5 @@
> +    let selectionSize = this._richlistbox.selectedItems.length;
> +    let contextMenu = document.getElementById("downloadsContextMenu");
> +    for (var i = 0; i < contextMenu.childNodes.length; ++i) {
> +      let item = contextMenu.childNodes[i];
> +      this._setDisability(item,selectionSize);

let's make this more similar to the code in controller.js
if (this._shouldDisableMenuItem(item, selectionSize) {
  // disable the command here.
}
where shouldDisableMenuItem will just return a bool.

Comment 10

[Pavan Karthik [:matrixisreal]]

Attachment: v.2.0.patch

Hi,
As you said, the commands indeed pass through isCommandEnabled()
But Im not sure which implementation is more elegant.

Comment 11

[Pavan Karthik [:matrixisreal]]

Attachment: v.2.0.patch

Comment 12

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Comment on attachment 8827213 [details] [diff] [review]
v.2.0.patch

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

This looks MUCH smaller than the other approach. Did you test it in a local build and does it properly disable the entries in the menu as expected?
I'd go for this, if you can confirm it works as expected.

Please add a commit message with also the reviewer.

::: browser/components/downloads/content/allDownloadsViewOverlay.js
@@ +1160,5 @@
>    // nsIController
>    isCommandEnabled(aCommand) {
>      switch (aCommand) {
>        case "cmd_copy":
> +      case "downloadsCmd_openReferrer":        

trailing spaces here.

Comment 13

[Pavan Karthik [:matrixisreal]]

Attachment: v2.1.patch

hi,
yeah, this approach turned out out much much simpler. 
and yes I have tested the patch locally and it worked fine. :D
Fixed the nits.

Comment 14

[Pavan Karthik [:matrixisreal]]

Attachment: V2.patch

Oops, forgot the commit message.

Comment 15

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Comment on attachment 8828258 [details] [diff] [review]
V2.patch

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

LGTM!
I will push this to try as soon as my current build is done.

PS: in the commit message, after r=, you can also just put the nickname used on irc that is often indicated on bugzilla with the real name. In my case r=mak would be enough.

Comment 16

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=204eb5e337418054537f8b47b6ef927672fab909

Comment 17

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

The failures are random intermittents, so we are good to go.

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c8b01174d713
Use cases of some multi-selection commands in Downloads Library not clear. r=mak

Comment 19

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/c8b01174d713