All users were logged out of Bugzilla on October 13th, 2018

Add "Select All" button to Page Info "Media" tab

VERIFIED FIXED in Firefox 39

Status

()

--
enhancement
VERIFIED FIXED
11 years ago
3 years ago

People

(Reporter: jruderman, Assigned: anushbmx, Mentored)

Tracking

(Depends on: 1 bug)

Trunk
Firefox 39
Points:
---

Firefox Tracking Flags

(firefox39 fixed)

Details

(Whiteboard: [lang=xul][lang=js])

Attachments

(1 attachment, 4 obsolete attachments)

(Reporter)

Description

11 years ago
The saving-multiple-images feature in Page Info is really cool but hard to discover.  (I think part of the reason is that the preview pane and small listbox makes it hard to think of ctrl/shift-clicking items.)

It might be good to add a "Select All" button next to the "Save As..." button.  This would make it clear that it's possible to select multiple images and save them, and it would make it take fewer clicks to save a bunch of images.

Comment 1

5 years ago
Created attachment 8341206 [details] [diff] [review]
Added a |Select All| button.

Submitting 1st patch. It seems to work.
Please test and suggest improvements.

Thanks!
Attachment #8341206 - Flags: feedback?(gavin.sharp)

Comment 2

5 years ago
> +  if (tree && "treeBoxObject" in tree)
> +    tree.view.selection.selectAll();
When do you not have an imagetree and when does the tree not have a treeBoxObject?

> +      <hbox align="right">
> +        <button label="&selectall.label;" accesskey="&selectall.accesskey;"
> +                id="imageselectallbutton" oncommand="selectAllImages();"/>
> +      </hbox>

for hbox-es, align should be one of start center end baseline stretch.
Perhaps you meant to use the pack attribute (start center end)?

https://developer.mozilla.org/en-US/docs/XUL/Attribute/align
https://developer.mozilla.org/en-US/docs/XUL/Attribute/pack
Comment on attachment 8341206 [details] [diff] [review]
Added a |Select All| button.

Sorry I never got to this request in a timely fashion :(

Philip's comments are worth addressing, this looks good otherwise though.
Attachment #8341206 - Flags: feedback?(gavin.sharp) → feedback+
Mentor: jaws
OS: Windows Vista → All
Hardware: x86 → All
Whiteboard: [lang=xul][lang=js]
Anush, please use the patch that Sunny posted in comment #1 and address the feedback that Philip provided in comment #2.
Assignee: nobody → anush
Status: NEW → ASSIGNED

Comment 5

4 years ago
> +  if (tree && "treeBoxObject" in tree)
> +    tree.view.selection.selectAll();

Note after Bug 979835, treeBoxObject is not a property of tree any more.
(Assignee)

Comment 6

4 years ago
Created attachment 8572504 [details] [diff] [review]
418517.patch
Attachment #8572504 - Flags: review?(jaws)
Comment on attachment 8572504 [details] [diff] [review]
418517.patch

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

::: browser/base/content/pageinfo/pageInfo.js
@@ +1244,5 @@
> +{
> +  var tree = document.getElementById("imagetree");
> +
> +  if (!tree)
> +  	return unknown;

`unknown` is not a keyword in JavaScript. I think you meant to use `undefined` here, in which case just `return;` will return `undefined`.

@@ +1253,3 @@
>  function doSelectAll()
>  {
>    var elem = document.commandDispatcher.focusedElement;

After this line, add:
if (elem.id == "saveallbutton")
  elem = document.getElementById("imagetree");

::: browser/base/content/pageinfo/pageInfo.xul
@@ +236,5 @@
>            </vbox>
>            <spacer id="imageSaveBoxSpacer" flex="1"/>
> +          <button label="&selectall.label;" accesskey="&selectall.accesskey;"
> +                  icon="save" id="imagesaveallbutton"
> +                  oncommand="doSelectAllImages();"/>

We can re-use doSelectAll() here, and rename the id from imagesaveallbutton to saveallbutton (since this will also save non-image media such as video and audio files).
Attachment #8572504 - Flags: review?(jaws) → feedback+
Comment on attachment 8572504 [details] [diff] [review]
418517.patch

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

Please disregard my previous review comments.

::: browser/base/content/pageinfo/pageInfo.js
@@ +1239,5 @@
>      gClipboardHelper.copyString(text.join("\n"), document);
>    }
>  }
>  
> +function doSelectAllImages()

Actually, let's keep this a separate function instead of merging it with doSelectAll, but please rename this function to doSelectAllMedia()

@@ +1243,5 @@
> +function doSelectAllImages()
> +{
> +  var tree = document.getElementById("imagetree");
> +
> +  if (!tree)

Here you should just do:
if (tree)
  tree.view.selection.selectAll();

instead of returning early, since there is no other code that needs to be run. I don't see a way that tree would be null, but I also don't see the negative cost of having a simple null-check here.
(Assignee)

Comment 9

4 years ago
Created attachment 8574042 [details] [diff] [review]
418517.patch

Added the changes as mentioned in Comment #8
Attachment #8572504 - Attachment is obsolete: true
Attachment #8574042 - Flags: review?(jaws)
Comment on attachment 8574042 [details] [diff] [review]
418517.patch

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

::: browser/base/content/pageinfo/pageInfo.js
@@ +1244,5 @@
> +{
> +  var tree = document.getElementById("imagetree");
> +
> +  if (tree)
> +  	tree.view.selection.selectAll();

nit, please remove this tab character and replace it with two spaces (we don't use tab characters in our source code). You may want to change settings in your editor to always use spaces.

::: browser/base/content/pageinfo/pageInfo.xul
@@ +235,5 @@
>              <label control="thepreviewimage" value="&mediaPreview;" class="header"/>
>            </vbox>
>            <spacer id="imageSaveBoxSpacer" flex="1"/>
> +          <button label="&selectall.label;" accesskey="&selectall.accesskey;"
> +                  icon="save" id="imagesaveallbutton"

This id needs to be renamed to "selectallbutton"
Attachment #8574042 - Flags: review?(jaws) → feedback+
(Assignee)

Comment 11

4 years ago
Created attachment 8574063 [details] [diff] [review]
418517.patch

Yiks
Attachment #8574042 - Attachment is obsolete: true
Attachment #8574063 - Flags: review?(jaws)
Comment on attachment 8574063 [details] [diff] [review]
418517.patch

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

::: browser/base/content/pageinfo/pageInfo.xul
@@ +235,5 @@
>              <label control="thepreviewimage" value="&mediaPreview;" class="header"/>
>            </vbox>
>            <spacer id="imageSaveBoxSpacer" flex="1"/>
> +          <button label="&selectall.label;" accesskey="&selectall.accesskey;"
> +                  icon="save" id="selectallbutton"

I just noticed this, but why is icon="save" here?
Attachment #8574063 - Flags: review?(jaws) → feedback+
Attachment #8341206 - Attachment is obsolete: true
(Assignee)

Comment 13

4 years ago
Created attachment 8574088 [details] [diff] [review]
418517.patch
Attachment #8574063 - Attachment is obsolete: true
Attachment #8574088 - Flags: review?(jaws)
Attachment #8574088 - Flags: review?(jaws) → review+
Keywords: checkin-needed
https://hg.mozilla.org/integration/fx-team/rev/1e1ec290a384
Keywords: checkin-needed
Whiteboard: [lang=xul][lang=js] → [lang=xul][lang=js][fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/1e1ec290a384
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
status-firefox39: --- → fixed
Resolution: --- → FIXED
Whiteboard: [lang=xul][lang=js][fixed-in-fx-team] → [lang=xul][lang=js]
Target Milestone: --- → Firefox 39
I looked for this button on :
version: BonEcho/2.0.0.13pre user agent : Mozilla/5.0 (Windows; U; Windows NT 6.2; en-US; rv:1.8.1.13pre) Gecko/2008021903
And the button is now found on :
Firefox version : 39.0b7 User Agent : Mozilla/5.0 (Windows NT 6.3; rv:39.0) Gecko/20100101 Firefox/39.0
suggesting Status -> verified fixed
Thanks!
Status: RESOLVED → VERIFIED

Updated

3 years ago
Depends on: 1224960
You need to log in before you can comment on or make changes to this bug.