Use Tern jump-to-definition search for quickeditt #3847
Conversation
ghost
assigned 
|
Reviewing. |
| /** | ||
| * Return QuickEdit helper. | ||
| */ | ||
| function getQuickEditHelper() { |
There was a problem hiding this comment.
I'm guessing that the reason you're doing this is because there isn't a way for one extension (JavaScriptQuickEdit) to call directly into another extension (JavaScriptCodeHints), which is an unfortunate limitation of our current extensibility mechanism. But I'm leery of adding public functionality like this into a core class like Editor, especially since the way this is set up, it won't generalize at all (e.g., if we had some other kind of "jump to definition" functionality for other languages in the future--e.g. going from an ID in a CSS selector to the HTML tag with that ID).
So maybe what we should do is make "Jump to Definition" actually be a command that's defined in the core, and have it rely on some findDefinition() function; that function in turn would delegate to providers that can be plugged in by language-specific extensions (where each provider would look at the current editor's mode to figure out if it applied--similar to how we do other language-specific providers).
There was a problem hiding this comment.
Yes, this is a hack to let two extensions, QuickEdit and CodeHints, communicate. The most general solution would be to implement a registration and lookup mechanism for extensions that would allow them all to call each other. I don't think moving Jump-To-Definition to the core is as good. The other possibility is to move Tern to the core and allow extensions to call it directly.
[ Also sent this comment by email to the DL-Brackets Dev mailing list. ]
There was a problem hiding this comment.
We chatted about it at the scrum this morning, and came up with two suggested alternatives:
- (easier, more hacky) Since this is a hack anyway, let's not add anything to core for it; instead, just have the code hints extension stuff a function onto some global (e.g. brackets._jsCodeHints.findDefinition) and have the quick edit extension grab it from there.
- (more work, less hacky) Just combine JS Quick Edit and JS Code Hints into a single extension (this is what the Typescript extension does). Since there's a strong dependency between them now anyway, there isn't any value in keeping them separate. Probably the easiest way to do this would be to create a single "JavaScriptSupport" extension, put the existing contents of the two extensions into separate folders within that extension, and create a new top-level main.js that loads the two main.js-es from the subfolders. (You'd probably have to do the same thing with unittests.js as well.)
Given that we're near the end of the sprint, (1) is fine for now since it's less work/less risk--we can file a bug to do (2) later on.
There was a problem hiding this comment.
Agreed. Option #1 is equivalent to what I've done. If you can be more explicit about exactly what you'd like here, I'll make that change today.
Option #2 will solve our immediate problem, but won't deal with general communication between arbitrary extensions. We should consider what we really want in the long term.
|
Review complete. It might be worth sending an email out to the team to discuss the "jump-to-definition-provider" issue...what I'm suggesting might be overkill, but I do feel like the approach in this pull request is a little too hacky. Maybe there's some better middle ground that I haven't thought of. |
| if ($deferredJump) { | ||
| response.fullPath = getResolvedPath(response.resultFile); |
There was a problem hiding this comment.
Just noticed one more thing--this addition causes a JSLint warning because JSLint wants functions to be defined before they're used. (I don't like that restriction, but it's basically because JSLint is a one-pass parser, I think, and for better or for worse we're requiring files to be JSLint-clean.) So handleJumptoDef() now needs to move below the definition of getResolvedPath().
There was a problem hiding this comment.
No problem. Pushed just now.
1. MultiRangeInlineEditor.js: simplified conditional, as requested. 2. ScopeManager.js: move function getResolvedPath before its use, to make JSLint happy. 3. QuickEdit/main.js: move functionName null check to calling routine for cleanliness. Things not yet done: 1. QuickEdit/main.js: consolidation of fallback paths. 2. QuickEditHelper: communication between extensions. Short vs. long term solutions.
|
Updates look fine. We can merge once the cross-dependency is addressed (either way)--also, looks like this needs a merge with master. |
|
Oops, I meant that it looks like there's a Travis failure, but it's not related to your code (@dangoor, it looks like a failure in the node package installation tests). Let's see if it fails again next time you push. |
…g). It should never be missing.
…kEdit:
It now gets set directly as brackets._jsCodeHintsHelper = quickEditHelpe;.
as suggested.
|
I've pushed everything that I have seen requested. Let me know if I missed anything. Thanks. |
|
Looks good--just one last case I caught. |
|
We could check if "functions" is empty. Do you want to fall back to the other search in that case? I'm not sure if that would be worthwhile or not. |
|
Pushed the requested change -- now checking for empty "functions". |
|
Looks good, thanks. Merging |
Use Tern jump-to-definition search for quickeditt
Current changes:
Future plans:
Note that the futures depend on improvements in tern.