Closed Bug 626484 Opened 13 years ago Closed 13 years ago

Incorrect order of lines when copying from web console

Categories

(DevTools :: General, defect, P1)

defect

Tracking

(blocking2.0 -)

VERIFIED FIXED
Firefox 6
Tracking Status
blocking2.0 --- -

People

(Reporter: ehsan.akhgari, Assigned: unusualtears)

References

Details

(Whiteboard: [console-1][has-patch][fixed-in-devtools][merged-to-mozilla-central])

Attachments

(1 file, 5 obsolete files)

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.
blocking2.0: --- → ?
Really ugly, but I don't think it's a blocker. Would accept a patch if there's an obvious fix.
blocking2.0: ? → -
Blocks: devtools4
Priority: -- → P1
Assignee: nobody → mihai.sucan
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.
Assigning the bug to Adam, who has submitted a working patch. Will provide feedback.
Assignee: mihai.sucan → unusualtears
Status: NEW → ASSIGNED
OS: Mac OS X → All
Hardware: x86 → All
Version: unspecified → Trunk
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.
Attachment #509182 - Flags: feedback-
I have the filtering fix in bug 626607.
which ... landed?
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.
Attachment #509182 - Attachment is obsolete: true
Comment on attachment 509580 [details] [diff] [review]
Reworked patch based on feedback + added mochitest

This looks good! Thanks for the patch. :)
Attachment #509580 - Flags: review?(gavin.sharp)
Attachment #509580 - Flags: feedback+
Whiteboard: [console-1]
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.
Attachment #509580 - Flags: review?(gavin.sharp) → review-
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.
Attachment #509580 - Attachment is obsolete: true
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.
Attachment #524011 - Flags: review?(mihai.sucan)
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.
Attachment #524011 - Flags: review?(mihai.sucan)
Attachment #524011 - Flags: review?(gavin.sharp)
Attachment #524011 - Flags: review+
Thanks again, Mihai.
Attachment #524011 - Attachment is obsolete: true
Attachment #524011 - Flags: review?(gavin.sharp)
gavin, could you give this an r+? probably doesn't need much actual review as mihai's already given it a work-through.
Whiteboard: [console-1] → [console-1][has-patch]
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.
Attachment #524079 - Flags: review-
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.
Attachment #524079 - Attachment is obsolete: true
Attachment #526801 - Flags: feedback?(mihai.sucan)
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 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!
Attachment #526801 - Flags: feedback?(mihai.sucan) → feedback-
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.
Attachment #527039 - Flags: feedback?(mihai.sucan) → feedback+
Attachment #527039 - Flags: review?(gavin.sharp)
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)
Attachment #527039 - Flags: review?(gavin.sharp) → review+
(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 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.
Attachment #527039 - Flags: review?(rcampbell)
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
Attachment #527039 - Flags: review?(rcampbell)
ready to check this in, but not sure what to use for the user. Is "Adam" + bugmail address sufficient?
Whiteboard: [console-1][has-patch] → [console-1][has-patch][fixed-in-devtools]
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
Attachment #527039 - Attachment description: Gathering loop now operates directly on children, avoiding selectedItems altogether. → [in-devtools]Gathering loop now operates directly on children, avoiding selectedItems altogether.
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Whiteboard: [console-1][has-patch][fixed-in-devtools] → [console-1][has-patch][fixed-in-devtools][merged-to-mozilla-central]
Target Milestone: --- → Firefox 6
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
Attachment #527039 - Attachment description: [in-devtools]Gathering loop now operates directly on children, avoiding selectedItems altogether. → [checked-in][in-devtools]Gathering loop now operates directly on children, avoiding selectedItems altogether.
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
Status: RESOLVED → VERIFIED
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: