771888 - Page Info > Media handles tree selection wrong (uses .currentIndex)

Status: RESOLVED FIXED

(Firefox :: Page Info Window, defect)

Description

[Atte Kemppilä [:atte]]

STR:
1. Go to http://www.mozilla.org/
2. Open: Page Info > Media
3. Select two images from the list
4. De-select one of the two selected images

Actual Results:
Preview shows the image that was de-selected.

Expected Results:
Preview shows the image that remained selected.

Basically the problem is that the code uses tree.selection.currentIndex, which gives the cursor row, not the selected row.

Comment 1

[Atte Kemppilä [:atte]]

Attachment: patch

Patch removes all uses of .currentIndex. I left getSelectedImage() in place even though it's not used anymore.

Comment 2

[Atte Kemppilä [:atte]]

Attachment: patch v2

On second thought, it's better to remove the unused getSelectedImage().

Comment 3

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 651443 [details] [diff] [review]
patch v2

Daniel, will you have time to review this patch soon?

Comment 4

[Drew Willcoxon :adw]

Comment on attachment 651443 [details] [diff] [review]
patch v2

Thanks for the patch.

> function onImageSelect()
> {
>   var previewBox   = document.getElementById("mediaPreviewBox");
>   var mediaSaveBox = document.getElementById("mediaSaveBox");
>   var splitter     = document.getElementById("mediaSplitter");
>   var tree = document.getElementById("imagetree");
>-  var count = tree.view.selection.count;
>+  var rows = getSelectedRows(tree);
>+  var count = rows.length;
>   if (count == 0) {
>     previewBox.collapsed   = true;
>     mediaSaveBox.collapsed = true;
>     splitter.collapsed     = true;
>     tree.flex = 1;
>   }
>   else if (count > 1) {
>     splitter.collapsed     = true;
>@@ -865,24 +865,24 @@ function onImageSelect()
>     mediaSaveBox.collapsed = false;
>     tree.flex = 1;
>   }
>   else {
>     mediaSaveBox.collapsed = true;
>     splitter.collapsed     = false;
>     previewBox.collapsed   = false;
>     tree.flex = 0;
>-    makePreview(tree.view.selection.currentIndex);
>+    makePreview(rows[0]);
>   }
> }

It looks like the only case that actually needs a selected row index is the else branch, when count == 1.  getSelectedRows is O(n), n = number of rows in the tree, which means you're always doing O(n) work when at most you only need O(1) work, in the else branch.  So I think you should keep the var count definition from the original code and call getSelectedRow (or use getSelectedRows()[0]) for the makePreview argument.

(Now, maybe it's the case that tree.view.selection.count is O(n) too, I don't know offhand, but that's no reason for unnecessary O(n) work here.)

Please make that change before landing, or let me know if I'm not seeing some problem with what I've said.

Comment 5

[Atte Kemppilä [:atte]]

Attachment: patch v3

Patch updated according comment 4.

Comment 6

[Ryan VanderMeulen [:RyanVM]]

(Remarkably) Green on Try.
https://tbpl.mozilla.org/?tree=Try&rev=cf7f719d7f47

https://hg.mozilla.org/integration/mozilla-inbound/rev/12b61cd59f69

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/12b61cd59f69