852931 - Jump to function definition on cmd+click

Status: RESOLVED FIXED

(DevTools :: Debugger, defect, P3)

Description

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

This was originally planned for bug 762160 but I haven't got the time to finish it. The implementation exists and working properly as far as I can tell (although I've heard that on Windows there's a bit of weirdness going on with event listeners).

Pretty much the only thing remaining is to write some tests, so I'm forking that bug here. I'll add a patch here shortly, if anyone feels like completing this work, that would be fabulous.

Comment 1

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

Attachment: v1

WIP. Does what it's supposed to. Needs tests.

Comment 2

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Victor, are you still working on this? If not, want to mentor this bug instead? Feel free to add the whiteboard stuff...

Comment 3

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

Nope, I am not assigned to this. It might be a good mentored bug, definitely not easy. The attached patch is most likely bitrotted to oblivion.

Comment 4

[mitchfriedman5]

Attachment: A patch of a basic prototype

Comment 5

[mitchfriedman5]

Hi, I've been playing around with this issue and I've created a small proof of concept just to see if I would be able to complete it.  As of this patch I can click on a function and jump to the definition (I only tested it with a function in the same source, not sure how it works with externally defined functions).

Some things I noticed is that I basically copy pasted a function, _findIdentifier, so maybe abstracting that to some utility file would be good, but I defer to someone who knows the code base before doing that.

Also, thoughts on what should happen from a UX point of view if there are no matches, or alternatively more than one match found, for a function definition?

This is my first time committing to Mozilla so if I've done anything horribly wrong let me know :)

Comment 6

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Do you want feedback on your patch?  It's always best to set a feedback or review flag on your patch to ensure it's not missed in the deluge of bug mail.  When you're ready, set the appropriate attachment flag to this bug's mentor (Victor).

Comment 7

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

Comment on attachment 8646194 [details] [diff] [review]
A patch of a basic prototype

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

Great start!

I'd say that in the case of no matches, a tooltip or some sort of a message could be displayed in the UI. In the case of multiple matches, we could reuse the existing search mechanism and show a popup there (like when you use the @ operator to search for function definitions).

Obviously abstracting duplicated functionality in some sort of a helper function or class would be better than copy pasting code around.

Please submit your next feedback or review requests to :jlongster as he is primarily responsible with the debugger frontend nowadays. I can still help with code reviews if necessary.

Comment 8

[mitchfriedman5]

Attachment: Update to previous patch, refactored code to be more modular and include handling no definitions found

Comment 9

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8648904 [details] [diff] [review]
Update to previous patch, refactored code to be more modular and include handling no definitions found

I am guessing this was meant to be a review request.

Comment 10

[mitchfriedman5]

Hey, thanks for the feedback Victor, I just updated it again and set it reviewer to jlongster.  I refactored some of the code to be more modular so that I can use the code in other places, which I plan to do after a few more reviews on the core functionality of this bug (I want to stick to one thing first, then I can work on the refactors).  

I also added handling the case where there's no function definitions found.

I am running into a few problems now though.  I'm not sure why, but for some reason the marked text that I set to the _markedIdentifier variable isn't being cleared in the hideNoResultsTooltip function.  

And yes, Ryan, this was meant to be asking for review, sorry about that, misclicked!

Comment 11

[mitchfriedman5]

Attachment: Didn't include some of my changes in previous patch

Comment 12

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Just to set expectations, :jlongster is on PTO until Thursday, so it likely won't be until end of the week / next week for his review.

Comment 13

[James Long (:jlongster)]

Comment on attachment 8648904 [details] [diff] [review]
Update to previous patch, refactored code to be more modular and include handling no definitions found

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

looks like it's just the 3rd patch that needs to be reviewed. fwiw, when uploading a new patch you can set the older patches to "obsolete" and they won't show up anymore.

Comment 14

[James Long (:jlongster)]

Comment on attachment 8648910 [details] [diff] [review]
Didn't include some of my changes in previous patch

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

This is great, thank you for working on this (and sorry it took me a few days to get to it)! I think we really need to make it so that it only tries to jump to the definition if you Cmd+click an identifier (which is what the title of this bug suggests). Currently in `_onEditorCursorActivity` it executes this behavior, no matter what. So whenever you try to interact with the editor at all, it aggressively moves the cursor around which is jarring.

If anything, in the future we hope to have live editing so you'll be able to actually write code in the editor.

There is also some strange behavior, like after it selects the definition I can't click elsehwhere unless I click just the right place. But I think these kinds of bugs would go away if we only did it on cmd+click.

Lastly, our code for finding identifiers seems a little off, as shown in the screenshot I attached. I wouldn't worry about this now though, but it's just another reason in my opinion to do it only when the user really intends to, otherwise the "no definition found" tooltip appears in seemingly arbitrary places.

Thanks a lot for working on this, please r? me when you get cmd+click working! (and let me know if you need any help)

Comment 15

[James Long (:jlongster)]

Attachment: Screen Shot 2015-08-25 at 4.31.20 PM.png

The cursor was on the line right about "foo", but it landed on the number 4. Don't worry about fixing it here, just wanted to document it. We might be able to improve our `findIdentifer` logic later.

Comment 16

[mitchfriedman5]

Attachment: Add jump to function definition on cmd + click

Hey, thanks for the feedback on the previous patches!  I initially implemented the feature with just click because I wanted to do the hard part first and do a light prototype, but yeah, now that it works (mostly) it definitely makes sense to add that in.  

This patch includes that, as requested, and after playing around with it a bit it seems to work MUCH better. There's no more weird jumping around and tooltips in random places.  

Let me know what you think of this and what we should change. I still haven't implemented multiple definitions found, now sure to approach that one from an implementation standpoint.  I think in terms of UX, what Victor said makes sense, having the same feature that search has with the next and previous matches makes sense.

Thanks!

Comment 17

[James Long (:jlongster)]

Comment on attachment 8652639 [details] [diff] [review]
Add jump to function definition on cmd + click

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

::: browser/devtools/debugger/views/sources-view.js
@@ +975,5 @@
>    _onEditorUnload: function(aName, aEditor) {
>      aEditor.off("cursorActivity", this._onEditorCursorActivity);
>    },
>  
> +  _onMouseMove: function(e) {

Why do you do this on mousemove? I'd like to avoid doing the extra work just when the user is hovering over the text. Using cmd+click should be the only thing that triggers a full parse of the source.

It also makes this code cleaner and more straight-forward: when a meta+click happens, do X. You have the same coordinates `e.clientX` and `e.clientY` in the mouseDown event.

Comment 18

[mitchfriedman5]

Attachment: Move logic of finding definition into mousedown from mouseMove

Good point, I just used what was given in the base patch from Victor as a starting point, but it makes complete sense to remove that from the mouseMove and keep the logic contained in the mouseDown.

This small patch updates that. 

Let me know if there's anything else you can see that would help make this patch better!

Comment 19

[James Long (:jlongster)]

Comment on attachment 8655004 [details] [diff] [review]
Move logic of finding definition into mousedown from mouseMove

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

One last thing: clean up that function some and I'll r+ the next patch. Thanks again!

::: browser/devtools/debugger/views/sources-view.js
@@ +981,5 @@
> +
> +    let identifier = this._findIdentifier(e.clientX, e.clientY);
> +
> +    if (identifier) {
> +      this._currentHoveredFunction = identifier;

You don't need to set this variable anymore. All the logic is now nicely contained in this single function. Clean up this function and make it as small as possible, and it's an r+, looks great to me!

(feel free to still r? me on the next version, I'll reply a lot faster. do you need help pushing this patch to our testing servers?)

Comment 20

[mitchfriedman5]

Attachment: Clean up onMouseDown function

Hey thanks for the feedback!  I tried to clean up that function as much as possible, let me know if there is anything you can see that will make it cleaner.  Thanks!

Comment 21

[James Long (:jlongster)]

Comment on attachment 8659960 [details] [diff] [review]
Clean up onMouseDown function

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

r+ with a few nits! It would be good write a test case for this too, however. Do you want me to do that, or do you want to learn how to write tests?

::: browser/devtools/debugger/views/sources-view.js
@@ +984,5 @@
> +    let editor = this.DebuggerView.editor;
> +    let identifier = this._findIdentifier(e.clientX, e.clientY);
> +
> +    if (!identifier) {
> +        return;

nit: 2 space indentation instead of 4

@@ +999,5 @@
> +
> +  /**
> +   * Searches for function definition of a function in a given source file
> +   */
> +

nit: remove this newline

Comment 22

[mitchfriedman5]

Attachment: Bug852931.patch

Comment 23

[James Long (:jlongster)]

Attachment: 852931.patch

I could not reliably get a synthetic mouse event working for the CodeMirror editor. Instead I just get the coords of the word I want to click and call `Sources._onMouseDown` directly. This will be much more reliable and I think it's fine. The test now works.

Comment 24

[James Long (:jlongster)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=4c4101bf330c

I will change the commit name to yours when we actually land this.

Comment 25

[James Long (:jlongster)]

Attachment: 852931.patch

Changed author's name

Comment 26

[Nigel Babu [:nigelb]]

Hello, this patch doesn't apply anymore. Can it be refreshed please?

Comment 27

[James Long (:jlongster)]

Attachment: 852931.patch

Rebased

Comment 28

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/84b20f66736b

Comment 29

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/84b20f66736b

Comment 30

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Release Note Request (optional, but appreciated)
[Why is this notable]: Improves navigation between functions in the debugger
[Suggested wording]: Jump to function definitions in the debugger with Cmd-Click
[Links (documentation, blog post, etc)]: None yet, hopefully on MDN soon

Comment 31

[Will Bamberg [:wbamberg]]

I've added a note here: https://developer.mozilla.org/en-US/docs/Tools/Keyboard_shortcuts#Debugger
and here: https://developer.mozilla.org/en-US/docs/Tools/Debugger/UI_Tour#Source_pane.

Comment 32

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

(In reply to Will Bamberg [:wbamberg] from comment #31)
> I've added a note here:
> https://developer.mozilla.org/en-US/docs/Tools/Keyboard_shortcuts#Debugger
> and here:
> https://developer.mozilla.org/en-US/docs/Tools/Debugger/UI_Tour#Source_pane.

Thanks, seems good to me!

Comment 33

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Added to FF44 release notes.