Closed Bug 725392 Opened 10 years ago Closed 10 years ago

Source Editor: add a method to convert mouse coordinates to character offsets

Categories

(DevTools :: Source Editor, defect, P2)

defect

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 19

People

(Reporter: msucan, Assigned: msucan)

References

Details

Attachments

(1 file, 8 obsolete files)

Once we expose the Orion mouse events we also need a way to convert the mouse coordinates to character offsets.

As a bonus we also need a method to find the line index from a given character offset.

(both methods are available in Orion, we just need to expose this API in the Source Editor)
Hi! Can i work on this bug?
Hey VD! Welcome back! Yes, of course you can!

Please connect to IRC and we can talk about what you need to do for this patch. Thank you!
Attached patch Patch for Bug 725392 (obsolete) — Splinter Review
Attachment #597022 - Flags: review?(mihai.sucan)
Comment on attachment 597022 [details] [diff] [review]
Patch for Bug 725392

Review of attachment 597022 [details] [diff] [review]:
-----------------------------------------------------------------

This patch is looking good. Thanks a lot for your contribution! This is awesome!

Below I have only some coding style comments. Nothing really broken, but we try to keep our code in a consistent style. As a general suggestion, scroll through the file you edit to see what is the coding style, when you make some changes.

Looking forward for the updated patch - and as we discussed, I'm interested to hear about your thoughts on the test. Thanks a lot!

::: browser/devtools/sourceeditor/source-editor-orion.jsm
@@ +926,5 @@
>    },
> +  
> +  /**
> +   * Convert the given rectangle from one coordinate spaces to another.
> +   *

Please also add a description of the allowed coordinate spaces.

@@ +930,5 @@
> +   *
> +   * @param object aRect
> +   *         The rectangle to convert
> +   * @param string aFrom
> +   *         The Source coordinate space

Lower case "source".

@@ +933,5 @@
> +   * @param string aFrom
> +   *         The Source coordinate space
> +   * @param string aTo
> +   *         The destination coordinate space
> +   * @return Rectangle aRect

@return object

@@ +934,5 @@
> +   *         The Source coordinate space
> +   * @param string aTo
> +   *         The destination coordinate space
> +   * @return Rectangle aRect
> +   *         Returns the rectangle with changed coordinates

Returns an object representing the given rectangle in the new coordinate space.

@@ +936,5 @@
> +   *         The destination coordinate space
> +   * @return Rectangle aRect
> +   *         Returns the rectangle with changed coordinates
> +   *
> +   */

Please remove the empty comment line. Also, please end descriptions with a period.

@@ +946,5 @@
> +   * Get the character offset nearest to the given pixel location.
> +   *
> +   * @param number aValueX
> +   *
> +   * @param number aValueY

Please change aValueX and aValueY to aX and aY.

@@ +951,5 @@
> +   *
> +   * @return number 
> +   *         Returns the character offset at the given location
> +   *
> +   */

Please remove the empty comment line at the end, and you don't need to have empty lines between @params themselves, nor between @params and @returns.
Attachment #597022 - Flags: review?(mihai.sucan) → feedback+
Comment on attachment 597022 [details] [diff] [review]
Patch for Bug 725392

Review of attachment 597022 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/devtools/sourceeditor/source-editor-orion.jsm
@@ +932,5 @@
> +   *         The rectangle to convert
> +   * @param string aFrom
> +   *         The Source coordinate space
> +   * @param string aTo
> +   *         The destination coordinate space

Perhaps we should document a bit more here, describing which strings are valid 'coordinate spaces'.
I have no idea what to set this argument to, without looking at the underlying Orion's documentation.
Assignee: nobody → vandhanaa91
Status: NEW → ASSIGNED
Attached patch Patch for Bug 725392 (obsolete) — Splinter Review
Attachment #597022 - Attachment is obsolete: true
Attachment #600778 - Flags: review?(mihai.sucan)
Comment on attachment 600778 [details] [diff] [review]
Patch for Bug 725392

Review of attachment 600778 [details] [diff] [review]:
-----------------------------------------------------------------

This patch has broken rebase and you did not address all of my requests in comment 4. More specifically, I asked for some changes related to code comments that I see are not applied in this patch.

Please rebase again and please address my comments. Thank you!


(as we discussed on IRC, I'll look into fixing the test, once the patch is fine.)

::: browser/devtools/sourceeditor/source-editor-orion.jsm
@@ +81,1 @@
>  };

This shouldn't be here...

@@ +935,5 @@
> +   *         The rectangle to convert 
> +   * @param aRect.x the x of the rectangle.
> +	* @param aRect.y the y of the rectangle.
> +	* @param aRect.width the width of the rectangle.
> +	* @param aRect.height the height of the rectangle.

These lines have broken indentation. Also, please format as follows:

@param object aRect
       The rectangle to convert. Object properties: x, y, width and height.

The property names are self explanatory. They don't need further description.

::: browser/devtools/sourceeditor/source-editor.jsm
@@ +172,5 @@
> +   * annotation. The event object properties:
> +   *   - event - the DOM mouseout event object.
> +   *   - x and y - the mouse coordinates relative to the document being edited.
> +   */
> +   MOUSE_OUT: "MouseOut",

Why did you include these changes here?

This is a broken rebase.
Attachment #600778 - Flags: review?(mihai.sucan) → review-
Attached patch Patch for Bug 725392 (obsolete) — Splinter Review
Attachment #600778 - Attachment is obsolete: true
Attachment #601644 - Flags: review?(mihai.sucan)
Comment on attachment 601644 [details] [diff] [review]
Patch for Bug 725392

Review of attachment 601644 [details] [diff] [review]:
-----------------------------------------------------------------

VD, thanks for the updated patch!

I will get back to writing the test when I get a chance - when my time allows me to do so. Currently I have some higher priority work to do. If, in the mean time, you figure it out and you get the test working, please attach an updated patch. Thanks for your understanding!
Attachment #601644 - Flags: review?(mihai.sucan)
Attached patch Patch for Bug 725392 (obsolete) — Splinter Review
Attachment #601644 - Attachment is obsolete: true
Attachment #609460 - Flags: review?(mihai.sucan)
Comment on attachment 609460 [details] [diff] [review]
Patch for Bug 725392

Review of attachment 609460 [details] [diff] [review]:
-----------------------------------------------------------------

VD: thanks for your update, but the test is still wrong.

Please re-review the work you did, recheck the jsdoc comments of Orion and of the source editor. You need to think what needs to be tested and how - the broad perspective on things. I am sorry that until now I failed to be sufficiently clear in my explanations on IRC. We can try again, but it's best that you do some dump() calls in those event handlers, add tons of text in the editor, scroll through the editor, and see how all those coordinate spaces relate to each other, and see how they are expected to work.

Thank you again!
Attachment #609460 - Flags: review?(mihai.sucan)
I can help regarding this bug, I have already implemented this feature in my extension. What is the concern here. Is the patch missing something or the tests ?
(In reply to Girish Sharma [:Optimizer] from comment #12)
> I can help regarding this bug, I have already implemented this feature in my
> extension. What is the concern here. Is the patch missing something or the
> tests ?

I would really appreciate help here with the test. Can you please look into writing a test that works well for the new methods? Thanks!
Assignee: vandhanaa91 → mihai.sucan
Depends on: 759351
Moving to Source Editor component.

Filter on CHELICERAE.
Component: Developer Tools → Developer Tools: Source Editor
Attached patch updated patch (obsolete) — Splinter Review
Updated the patch.

The hard part was writing a test that appropriately checks how the different coordinate spaces interplay. Please let me know if I got this right.

Thank you!

(please note the patch dependency on the orion update)
Attachment #609460 - Attachment is obsolete: true
Attachment #643103 - Flags: review?(rcampbell)
Whiteboard: [sourceeditor][good first bug][mentor=msucan][lang=js]
Blocks: 732981
Comment on attachment 643103 [details] [diff] [review]
updated patch

Review of attachment 643103 [details] [diff] [review]:
-----------------------------------------------------------------

Not a lot to say other than the comment nit. Test looks good from here. Try it out on fx-team when the orion update's ready.

::: browser/devtools/sourceeditor/source-editor-orion.jsm
@@ +2025,5 @@
> +   * Convert the given rectangle from one coordinate space to another.
> +   *
> +   * Known coordinate spaces:
> +   * - "document" - gives the coordinates relative to the entire document.
> +   * - "view" - gives the coordinates relative to the editor viewport.

These aren't really "spaces". They don't describe different coordinate types (e.g., cartesian vs. polar), merely different origins. Could change the comment to "coordinate reference" to reflect that.
Attachment #643103 - Flags: review?(rcampbell) → review+
Attached patch updated patch (obsolete) — Splinter Review
Thanks! Good point about "space" versus "reference".
Attachment #643103 - Attachment is obsolete: true
Attached file exposed getLineAtOffset (obsolete) —
Unfortunately: https://tbpl.mozilla.org/?tree=Try&rev=8183035cde41

The test fails when checking character offsets, which were already exposed and added in the previous patch. The failure occurs regardless of my changes. They pass on linux though, surprisingly. Any ideas, Mihai?
Attachment #680218 - Flags: feedback?(mihai.sucan)
Attachment #680771 - Flags: feedback? → feedback?(mihai.sucan)
Attachment #680218 - Attachment is obsolete: true
Attachment #680218 - Flags: feedback?(mihai.sucan)
Comment on attachment 680771 [details] [diff] [review]
calmed down equality checks

Review of attachment 680771 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for the update.

The only concern I have is with the way getLineAtOffset() is tested. Please move those checks out from the test file and put them in the _line_api.js test - there you can check the exact values being returned, no ranges. Do a couple of checks there and it should be fine. Thanks!
Attachment #680771 - Flags: feedback?(mihai.sucan) → feedback+
Attachment #680771 - Attachment is obsolete: true
Attachment #648442 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/9e7cd5519f3b
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 19
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.