Last Comment Bug 626484 - Incorrect order of lines when copying from web console
: Incorrect order of lines when copying from web console
Status: VERIFIED FIXED
[console-1][has-patch][fixed-in-devto...
:
Product: Firefox
Classification: Client Software
Component: Developer Tools (show other bugs)
: Trunk
: All All
: P1 normal (vote)
: Firefox 6
Assigned To: Adam [:hobophobe]
:
Mentors:
Depends on:
Blocks: devtools4
  Show dependency treegraph
 
Reported: 2011-01-17 12:58 PST by :Ehsan Akhgari (away Aug 1-5)
Modified: 2011-05-12 06:21 PDT (History)
10 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
-


Attachments
Create an item array which is then sorted prior to pulling the strings; also skips filtered items, so that they aren't copied. (1.83 KB, patch)
2011-02-02 11:55 PST, Adam [:hobophobe]
mihai.sucan: feedback-
Details | Diff | Splinter Review
Reworked patch based on feedback + added mochitest (4.30 KB, patch)
2011-02-03 14:14 PST, Adam [:hobophobe]
mihai.sucan: review-
rcampbell: feedback+
Details | Diff | Splinter Review
Refined based on feedback, shortens sort function and solves test issues. (5.63 KB, patch)
2011-04-04 14:02 PDT, Adam [:hobophobe]
mihai.sucan: review+
Details | Diff | Splinter Review
Fixes undefined itemsSet in previous version of test. (5.65 KB, patch)
2011-04-05 12:29 PDT, Adam [:hobophobe]
gavin.sharp: review-
Details | Diff | Splinter Review
Gathers the indices and sorts without richlistbox.getIndexOfItem (4.51 KB, patch)
2011-04-18 13:12 PDT, Adam [:hobophobe]
mihai.sucan: feedback-
Details | Diff | Splinter Review
[checked-in][in-devtools]Gathering loop now operates directly on children, avoiding selectedItems altogether. (4.33 KB, patch)
2011-04-19 10:46 PDT, Adam [:hobophobe]
gavin.sharp: review+
mihai.sucan: feedback+
Details | Diff | Splinter Review

Description :Ehsan Akhgari (away Aug 1-5) 2011-01-17 12:58:04 PST
STR:

1. Open web console on a page, and make sure it contains more than one line.
2. Select any lines except for the first one.
3. Cmd+A.
4. Cmd+C.
5. Paste in a text editor.  The line selected in (2) is pasted as the first line, out of order.
Comment 1 Mike Beltzner [:beltzner, not reading bugmail] 2011-01-17 21:21:19 PST
Really ugly, but I don't think it's a blocker. Would accept a patch if there's an obvious fix.
Comment 2 Adam [:hobophobe] 2011-02-02 11:55:12 PST
Created attachment 509182 [details] [diff] [review]
Create an item array which is then sorted prior to pulling the strings; also skips filtered items, so that they aren't copied.

If needed I can file a separate bug/patch for the filtered items bit.  The current Web Console will copy items that were selected prior to filtering.

The main goal of this patch is to fix the ordering issue: the current implementation of richlistbox[multiselect=true] simply adds items to its selectedItems in the order they were selected, which means we must sort them prior to copying.

One alternative to this patch is to refresh the selection by iterating over the richlistbox and adding the selected items in that order, but sorting should be faster.

Whether richlistbox/listbox should be changed to keep the selectedItems sorted, I'm not sure, but it's probably too late to make changes the elements themselves, where hopefully sorting the items here may be minor enough to get in for Firefox 4.
Comment 3 Mihai Sucan [:msucan] 2011-02-03 05:12:09 PST
Assigning the bug to Adam, who has submitted a working patch. Will provide feedback.
Comment 4 Mihai Sucan [:msucan] 2011-02-03 05:29:32 PST
Comment on attachment 509182 [details] [diff] [review]
Create an item array which is then sorted prior to pulling the strings; also skips filtered items, so that they aren't copied.

Thanks for the patch Adam! Good work!

Patch comments:

+      // Do not copy items that are hidden due to filtering options.
+      let isFiltered = item.classList.contains("hud-filtered-by-string") ||
+                       item.classList.contains("hud-filtered-by-type");
+
+      if (isFiltered) {
+        continue;
+      }
+
+      items.push(item);

Why not just do:

if (item.classList.contains("hud-filtered-by-string") ||
    item.classList.contains("hud-filtered-by-type")) {
  items.push(item);
}

+      if (aItemIndex < bItemIndex) {
+        return -1;
+      }
+      if (aItemIndex > bItemIndex) {
+        return 1;
+      }
+      // Should be impossible.
+      return 0;

I think I'd prefer if () { ... } else if () { ... } else { ... }.

Otherwise, your patch is fine and works as intended. Thanks!

I am giving the patch feedback- because you need to write a mochitest. Recommended reading:

https://developer.mozilla.org/en/Browser_chrome_tests

I use the following bash script to run Web Console tests:

https://gist.github.com/809460

> mozhudtests # to run all tests
> mozhudtests test-file-name.js # to run a specific test

Please let me know if you have further questions.
Comment 5 Rob Campbell [:rc] (:robcee) 2011-02-03 10:06:21 PST
I have the filtering fix in bug 626607.
Comment 6 Rob Campbell [:rc] (:robcee) 2011-02-03 10:07:33 PST
which ... landed?
Comment 7 Adam [:hobophobe] 2011-02-03 14:14:17 PST
Created attachment 509580 [details] [diff] [review]
Reworked patch based on feedback + added mochitest

In this version I opt to sort the selectedItems array directly: nothing should depend on the original order of the array, so this should be simpler.

For the mochitest, I created a new test file (browser_webconsole_bug_626484_output_copy_order.js), but if it should be incorporated into an existing test, I can move it.  The test logs three unique lines, and then tries selecting+copying them in the six possible orders for selecting all three, verifying that the clipboard gets the correct contents.
Comment 8 Rob Campbell [:rc] (:robcee) 2011-02-04 05:35:45 PST
Comment on attachment 509580 [details] [diff] [review]
Reworked patch based on feedback + added mochitest

This looks good! Thanks for the patch. :)
Comment 9 Mihai Sucan [:msucan] 2011-04-04 09:47:06 PDT
Comment on attachment 509580 [details] [diff] [review]
Reworked patch based on feedback + added mochitest

Reviewing this patch within the devtools team, as discussed last week in a meeting with Kevin and David.

Review comments:

- the sort function could be smaller:

+    aOutputNode.selectedItems.sort(function(a, b) {
+      return aOutputNode.getIndexOfItem(a) <
+             aOutputNode.getIndexOfItem(b) -1 : 1;
+    });

- nit: the license block in the mochitest should follow the license policy:

http://www.mozilla.org/MPL/license-policy.html

+ * Contributor(s):
+ *  Adam Dane <unusualtears@gmail.com>

Contributor name starts at "n". Also, the PD license has no contributors. Consider using the Mozilla tri-license.


+  make_selection();

No need to wrap the code inside the make_selection() function. Just inline the make_selection() function content in the parent function.

- In the test:
+    expectedClipboardText.push("[" +
+        ConsoleUtils.timestampString(item.timestamp) + "] " +
+        item.clipboardText);

Nit: two indent spaces.


+  let HUD = HUDService.hudReferences[hudId].HUDBox;
+  outputNode = HUD.querySelector(".hud-output-node");

outputNode is available in the HUD object: HUD.outputNode.

Also the HUD variable is unused.

- You seem to make multiple selections for the same items, but in different order - that looks good. Still, I don't see where you clear each selection.

- All the mochitest tests pass even if I remove the call to the items selection sort in HUDService. The test must fail when there's no code change. (user testing confirms that the bug is still reproducible with the sort call removed.)

Giving the patch review- for the moment. Please address the comments, as we would like this to be in the Firefox 5 release.

Thanks for your work - it is very much appreciated! We apologize for the delay in reviewing your code.
Comment 10 Adam [:hobophobe] 2011-04-04 14:02:34 PDT
Created attachment 524011 [details] [diff] [review]
Refined based on feedback, shortens sort function and solves test issues.

Thank you for the feedback Mihai.  This patch:

1. Shortens the sort function.
2. Fixes the test license to use Mozilla tri-license.
3. Clears the selection before each test.
4. Restructures the waitForClipboard calls so that the tests would be seen as discrete tests.  waitForClipboard's callbacks are now to the added nextTest function, allowing all of the tests to run properly.
5. Calls jsterm.clearOutput when done and clears the selection after all tests are complete.
Comment 11 Mihai Sucan [:msucan] 2011-04-05 02:10:19 PDT
Comment on attachment 524011 [details] [diff] [review]
Refined based on feedback, shortens sort function and solves test issues.

(Please ask for re-review when you submit updated patches.)

Thanks for your quick update! I will review the changes ASAP.
Comment 12 Mihai Sucan [:msucan] 2011-04-05 12:11:51 PDT
Comment on attachment 524011 [details] [diff] [review]
Refined based on feedback, shortens sort function and solves test issues.

The patch now looks fine. Thanks!

itemsSet is undefined. You need to add let itemsSet; before the functions.

Beyond that, review+! Asking for additional review from a toolkit peer.
Comment 13 Adam [:hobophobe] 2011-04-05 12:29:31 PDT
Created attachment 524079 [details] [diff] [review]
Fixes undefined itemsSet in previous version of test.

Thanks again, Mihai.
Comment 14 Rob Campbell [:rc] (:robcee) 2011-04-06 11:47:22 PDT
gavin, could you give this an r+? probably doesn't need much actual review as mihai's already given it a work-through.
Comment 15 :Gavin Sharp [email: gavin@gavinsharp.com] 2011-04-12 21:52:07 PDT
Comment on attachment 524079 [details] [diff] [review]
Fixes undefined itemsSet in previous version of test.

Calling getIndexOfItem twice in a sort function is extremely inefficient - it builds an array of children on each invocation (see the "children" property and getIndexOfItem implementation in richlistbox.xml). I'd be surprised if this doesn't result in unresponsiveness when copying many selected items.

You'd be better off getting aOutputNode.children once, and doing your own indexOf calls, even if that isn't quite so concise. Given each child an "order" ordinal once (e.g. aOutputNode.children.forEach(function (c) c._index=index++) but with a loop to garantee ordering) and then just using aItem._index < bItem._index may be even more efficient.
Comment 16 Adam [:hobophobe] 2011-04-18 13:12:03 PDT
Created attachment 526801 [details] [diff] [review]
Gathers the indices and sorts without richlistbox.getIndexOfItem

Follows Gavin's suggestion (Comment 15) to store each index in an _index property.  It only adds the item._index for the selected items, as I did not see a benefit to adding it to the others.

This could also be done with a mapping using each item's id as the key.  That would be slightly slower (would lookup the id and then lookup inside the mapping), but would avoid adding the extra properties.
Comment 17 :Gavin Sharp [email: gavin@gavinsharp.com] 2011-04-18 17:45:10 PDT
The forEach in that patch is still inefficient - no need to call indexOf for each element when you can just iterate through aOutputNode.selectedItems and directly set _index to your loop variable.
Comment 18 Mihai Sucan [:msucan] 2011-04-19 08:33:08 PDT
Comment on attachment 526801 [details] [diff] [review]
Gathers the indices and sorts without richlistbox.getIndexOfItem

It is an improvement, but I agree with Gavin: please update the patch as requested in comment 17.

Thanks!
Comment 19 Adam [:hobophobe] 2011-04-19 10:46:12 PDT
Created attachment 527039 [details] [diff] [review]
[checked-in][in-devtools]Gathering loop now operates directly on children, avoiding selectedItems altogether.
Comment 20 Mihai Sucan [:msucan] 2011-04-20 06:10:24 PDT
Comment on attachment 527039 [details] [diff] [review]
[checked-in][in-devtools]Gathering loop now operates directly on children, avoiding selectedItems altogether.

Patch looks fine now. Feedback+!

Please ask Gavin for review again.
Comment 21 :Gavin Sharp [email: gavin@gavinsharp.com] 2011-04-20 13:36:39 PDT
Comment on attachment 527039 [details] [diff] [review]
[checked-in][in-devtools]Gathering loop now operates directly on children, avoiding selectedItems altogether.

much better, thanks! (I didn't review the test)
Comment 22 :Ehsan Akhgari (away Aug 1-5) 2011-04-20 14:14:59 PDT
(In reply to comment #21)
> (I didn't review the test)

Then please ask for somebody else to review the test before you land this patch.  Thanks! :-)
Comment 23 Adam [:hobophobe] 2011-04-20 16:22:26 PDT
Comment on attachment 527039 [details] [diff] [review]
[checked-in][in-devtools]Gathering loop now operates directly on children, avoiding selectedItems altogether.

Test needs review; Gavin has already reviewed the patch.
Comment 24 :Gavin Sharp [email: gavin@gavinsharp.com] 2011-04-21 01:07:36 PDT
Comment on attachment 527039 [details] [diff] [review]
[checked-in][in-devtools]Gathering loop now operates directly on children, avoiding selectedItems altogether.

the test doesn't need review
Comment 25 Rob Campbell [:rc] (:robcee) 2011-04-21 06:44:07 PDT
ready to check this in, but not sure what to use for the user. Is "Adam" + bugmail address sufficient?
Comment 26 Rob Campbell [:rc] (:robcee) 2011-04-21 07:01:53 PDT
Comment on attachment 527039 [details] [diff] [review]
[checked-in][in-devtools]Gathering loop now operates directly on children, avoiding selectedItems altogether.

http://hg.mozilla.org/projects/devtools/rev/b30aa967de2e
Comment 27 Rob Campbell [:rc] (:robcee) 2011-05-09 12:20:27 PDT
Comment on attachment 527039 [details] [diff] [review]
[checked-in][in-devtools]Gathering loop now operates directly on children, avoiding selectedItems altogether.

http://hg.mozilla.org/mozilla-central/rev/b30aa967de2e
Comment 28 AndreiD[QA] 2011-05-12 06:21:03 PDT
VERIFIED FIXED ON:
Mozilla/5.0 (Windows NT 6.1; rv:6.0a1) Gecko/20110511 Firefox/6.0a1
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:6.0a1) Gecko/20110511 Firefox/6.0a1
Mozilla/5.0 (X11; Linux i686; rv:6.0a1) Gecko/20110512 Firefox/6.0a1

Note You need to log in before you can comment on or make changes to this bug.