Closed
Bug 725392
Opened 13 years ago
Closed 12 years ago
Source Editor: add a method to convert mouse coordinates to character offsets
Categories
(DevTools :: Source Editor, defect, P2)
DevTools
Source Editor
Tracking
(Not tracked)
RESOLVED
FIXED
Firefox 19
People
(Reporter: msucan, Assigned: msucan)
References
Details
Attachments
(1 file, 8 obsolete files)
(deleted),
patch
|
Details | Diff | Splinter Review |
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)
Assignee | ||
Comment 2•13 years ago
|
||
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!
Attachment #597022 -
Flags: review?(mihai.sucan)
Assignee | ||
Comment 4•13 years ago
|
||
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 5•13 years ago
|
||
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 | ||
Updated•13 years ago
|
Assignee: nobody → vandhanaa91
Status: NEW → ASSIGNED
Attachment #597022 -
Attachment is obsolete: true
Attachment #600778 -
Flags: review?(mihai.sucan)
Assignee | ||
Comment 7•13 years ago
|
||
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-
Attachment #600778 -
Attachment is obsolete: true
Attachment #601644 -
Flags: review?(mihai.sucan)
Assignee | ||
Comment 9•13 years ago
|
||
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)
Comment 10•13 years ago
|
||
Attachment #601644 -
Attachment is obsolete: true
Attachment #609460 -
Flags: review?(mihai.sucan)
Assignee | ||
Comment 11•13 years ago
|
||
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)
Comment 12•13 years ago
|
||
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 ?
Assignee | ||
Comment 13•13 years ago
|
||
(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•12 years ago
|
||
Moving to Source Editor component.
Filter on CHELICERAE.
Component: Developer Tools → Developer Tools: Source Editor
Assignee | ||
Comment 15•12 years ago
|
||
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)
Assignee | ||
Updated•12 years ago
|
Whiteboard: [sourceeditor][good first bug][mentor=msucan][lang=js]
Comment 16•12 years ago
|
||
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+
Assignee | ||
Comment 17•12 years ago
|
||
Thanks! Good point about "space" versus "reference".
Attachment #643103 -
Attachment is obsolete: true
Comment 18•12 years ago
|
||
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)
Comment 19•12 years ago
|
||
Attachment #680771 -
Flags: feedback?
Updated•12 years ago
|
Attachment #680771 -
Flags: feedback? → feedback?(mihai.sucan)
Updated•12 years ago
|
Attachment #680218 -
Attachment is obsolete: true
Attachment #680218 -
Flags: feedback?(mihai.sucan)
Assignee | ||
Comment 20•12 years ago
|
||
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+
Comment 21•12 years ago
|
||
Updated•12 years ago
|
Attachment #680771 -
Attachment is obsolete: true
Assignee | ||
Updated•12 years ago
|
Attachment #648442 -
Attachment is obsolete: true
Comment 22•12 years ago
|
||
Whiteboard: [fixed-in-fx-team]
Comment 23•12 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 19
Updated•6 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•