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

Status: RESOLVED FIXED

(DevTools :: Source Editor, defect, P2)

Description

[Mihai Sucan [:msucan]]

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)

Comment 1

[VD]

Hi! Can i work on this bug?

Comment 2

[Mihai Sucan [:msucan]]

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!

Comment 3

[VD]

Attachment: Patch for Bug 725392

Comment 4

[Mihai Sucan [:msucan]]

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.

Comment 5

[Cedric Vivier [:cedricv]]

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.

Comment 6

[VD]

Attachment: Patch for Bug 725392

Comment 7

[Mihai Sucan [:msucan]]

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.

Comment 8

[VD]

Attachment: Patch for Bug 725392

Comment 9

[Mihai Sucan [:msucan]]

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!

Comment 10

[VD]

Attachment: Patch for Bug 725392

Comment 11

[Mihai Sucan [:msucan]]

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!

Comment 12

[Girish Sharma [:Optimizer]]

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 ?

Comment 13

[Mihai Sucan [:msucan]]

(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!

Comment 14

[Rob Campbell [:rc] (:robcee)]

Moving to Source Editor component.

Filter on CHELICERAE.

Comment 15

[Mihai Sucan [:msucan]]

Attachment: updated patch

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)

Comment 16

[Rob Campbell [:rc] (:robcee)]

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.

Comment 17

[Mihai Sucan [:msucan]]

Attachment: updated patch

Thanks! Good point about "space" versus "reference".

Comment 18

[Victor Porof [:vporof][:vp]]

Attachment: exposed getLineAtOffset

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?

Comment 19

[Victor Porof [:vporof][:vp]]

Attachment: calmed down equality checks

https://tbpl.mozilla.org/?tree=Try&rev=01e25176b563

Comment 20

[Mihai Sucan [:msucan]]

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!

Comment 21

[Victor Porof [:vporof][:vp]]

Attachment: moved getLineAtOffset checks

Comment 22

[Victor Porof [:vporof][:vp]]

https://hg.mozilla.org/integration/fx-team/rev/9e7cd5519f3b

Comment 23

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/9e7cd5519f3b