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

RESOLVED FIXED in Firefox 53

Status

()

Firefox
Bookmarks & History
RESOLVED FIXED
6 months ago
4 months ago

People

(Reporter: matrixisreal, Assigned: matrixisreal, Mentored)

Tracking

unspecified
Firefox 53
Points:
---

Firefox Tracking Flags

(firefox53 fixed)

Details

(Whiteboard: [good first bug][lang=js])

Attachments

(1 attachment, 4 obsolete attachments)

(Assignee)

Description

6 months ago
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.
(Assignee)

Updated

6 months ago
Blocks: 1324564

Comment 1

6 months ago
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.
Flags: needinfo?(paolo.mozmail)

Comment 2

6 months ago
Morpheus, Bryant, do you have an opinion on this?
Flags: needinfo?(paolo.mozmail)
Flags: needinfo?(mochen)
Flags: needinfo?(bmao)

Comment 3

6 months ago
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.
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.
Flags: needinfo?(mochen)
Flags: needinfo?(bmao)

Comment 5

6 months ago
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.
Whiteboard: [good first bug][lang=js]
(Assignee)

Comment 6

6 months ago
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 :)
(Assignee)

Updated

6 months ago
Assignee: nobody → pavankarthikboddeda

Comment 7

6 months ago
(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.
(Assignee)

Comment 8

6 months ago
Created attachment 8823240 [details] [diff] [review]
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
Attachment #8823240 - Flags: review?(mak77)
(Assignee)

Updated

6 months ago
Status: NEW → ASSIGNED

Comment 9

5 months ago
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.
Attachment #8823240 - Flags: review?(mak77)
(Assignee)

Comment 10

5 months ago
Created attachment 8827211 [details] [diff] [review]
v.2.0.patch

Hi,
As you said, the commands indeed pass through isCommandEnabled()
But Im not sure which implementation is more elegant.
Attachment #8827211 - Flags: review?(mak77)
(Assignee)

Comment 11

5 months ago
Created attachment 8827213 [details] [diff] [review]
v.2.0.patch
Attachment #8827211 - Attachment is obsolete: true
Attachment #8827211 - Flags: review?(mak77)
Attachment #8827213 - Flags: review?(mak77)
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.
Attachment #8827213 - Flags: review?(mak77) → feedback+
(Assignee)

Comment 13

5 months ago
Created attachment 8828257 [details] [diff] [review]
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.
Attachment #8827213 - Attachment is obsolete: true
Attachment #8828257 - Flags: review?(mak77)
(Assignee)

Comment 14

5 months ago
Created attachment 8828258 [details] [diff] [review]
V2.patch

Oops, forgot the commit message.
Attachment #8828257 - Attachment is obsolete: true
Attachment #8828257 - Flags: review?(mak77)
Attachment #8828258 - Flags: review?(mak77)
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.
Attachment #8828258 - Flags: review?(mak77) → review+

Updated

5 months ago
Attachment #8823240 - Attachment is obsolete: true
https://treeherder.mozilla.org/#/jobs?repo=try&revision=204eb5e337418054537f8b47b6ef927672fab909
The failures are random intermittents, so we are good to go.
Keywords: checkin-needed

Comment 18

5 months ago
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c8b01174d713
Use cases of some multi-selection commands in Downloads Library not clear. r=mak
Keywords: checkin-needed

Comment 19

5 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/c8b01174d713
Status: ASSIGNED → RESOLVED
Last Resolved: 5 months ago
status-firefox53: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 53
QA Whiteboard: [good first verify]
You need to log in before you can comment on or make changes to this bug.