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

Status: VERIFIED FIXED

(Firefox :: Page Info Window, enhancement)

Description

[Jesse Ruderman]

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

[Sunny [:darkowlzz]]

Attachment: Added a |Select All| button.

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

Thanks!

Comment 2

[Philip Chee]

> +  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 3

[:Gavin Sharp [email: [e-mail]]]

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.

Comment 4

[Jared Wein [:jaws] (please needinfo? me)]

Anush, please use the patch that Sunny posted in comment #1 and address the feedback that Philip provided in comment #2.

Comment 5

[Philip Chee]

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

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

Comment 6

[Anush [:anushbmx]]

Attachment: 418517.patch

Comment 7

[Jared Wein [:jaws] (please needinfo? me)]

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).

Comment 8

[Jared Wein [:jaws] (please needinfo? me)]

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.

Comment 9

[Anush [:anushbmx]]

Attachment: 418517.patch

Added the changes as mentioned in Comment #8

Comment 10

[Jared Wein [:jaws] (please needinfo? me)]

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"

Comment 11

[Anush [:anushbmx]]

Attachment: 418517.patch

Yiks

Comment 12

[Jared Wein [:jaws] (please needinfo? me)]

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?

Comment 13

[Anush [:anushbmx]]

Attachment: 418517.patch

Comment 14

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/integration/fx-team/rev/1e1ec290a384

Comment 15

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/1e1ec290a384

Comment 16

[Saheda Reza [:Antora]]

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

Comment 17

[Jared Wein [:jaws] (please needinfo? me)]

Thanks!