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)
Firefox
Page Info Window
Tracking
()
RESOLVED
FIXED
Firefox 18
People
(Reporter: atte, Assigned: atte)
Details
Attachments
(1 file, 2 obsolete files)
4.56 KB,
patch
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•9 years ago
|
||
Patch removes all uses of .currentIndex. I left getSelectedImage() in place even though it's not used anymore.
Attachment #640045 -
Flags: review?(db48x)
Updated•9 years ago
|
Assignee: nobody → atte.kemppila
Assignee | ||
Comment 2•9 years ago
|
||
On second thought, it's better to remove the unused getSelectedImage().
Attachment #640045 -
Attachment is obsolete: true
Attachment #640045 -
Flags: review?(db48x)
Comment 3•9 years ago
|
||
Comment on attachment 651443 [details] [diff] [review] patch v2 Daniel, will you have time to review this patch soon?
Attachment #651443 -
Flags: review?(db48x)
Assignee | ||
Updated•9 years ago
|
Attachment #651443 -
Flags: review?(db48x) → review?(gavin.sharp)
Updated•9 years ago
|
Attachment #651443 -
Flags: review?(gavin.sharp) → review?(adw)
Comment 4•9 years ago
|
||
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+
Updated•9 years ago
|
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Assignee | ||
Comment 5•9 years ago
|
||
Patch updated according comment 4.
Attachment #651443 -
Attachment is obsolete: true
Assignee | ||
Updated•9 years ago
|
Keywords: checkin-needed
Comment 6•9 years ago
|
||
(Remarkably) Green on Try. https://tbpl.mozilla.org/?tree=Try&rev=cf7f719d7f47 https://hg.mozilla.org/integration/mozilla-inbound/rev/12b61cd59f69
Flags: in-testsuite-
Keywords: checkin-needed
Comment 7•9 years ago
|
||
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.
Description
•