Closed Bug 771888 Opened 9 years ago Closed 9 years ago

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

Categories

(Firefox :: Page Info Window, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 18

People

(Reporter: atte, Assigned: atte)

Details

Attachments

(1 file, 2 obsolete files)

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.
Attached patch patch (obsolete) — Splinter Review
Patch removes all uses of .currentIndex. I left getSelectedImage() in place even though it's not used anymore.
Attachment #640045 - Flags: review?(db48x)
Assignee: nobody → atte.kemppila
Attached patch patch v2 (obsolete) — Splinter Review
On second thought, it's better to remove the unused getSelectedImage().
Attachment #640045 - Attachment is obsolete: true
Attachment #640045 - Flags: review?(db48x)
Comment on attachment 651443 [details] [diff] [review]
patch v2

Daniel, will you have time to review this patch soon?
Attachment #651443 - Flags: review?(db48x)
Attachment #651443 - Flags: review?(db48x) → review?(gavin.sharp)
Attachment #651443 - Flags: review?(gavin.sharp) → review?(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.
Attachment #651443 - Flags: review?(adw) → review+
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Attached patch patch v3Splinter Review
Patch updated according comment 4.
Attachment #651443 - Attachment is obsolete: true
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/12b61cd59f69
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 18
You need to log in before you can comment on or make changes to this bug.