Skip to content
This repository was archived by the owner on Sep 6, 2021. It is now read-only.

Use Tern jump-to-definition search for quickeditt #3847

Merged
njx merged 6 commits into from
May 20, 2013
Merged

Use Tern jump-to-definition search for quickeditt #3847

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
6c36599
Use tern jump-to-definition search for QuickEdit.
jeffkenton May 15, 2013
7a86aad
Tweaks to make some test cases pass.
jeffkenton May 15, 2013
706d72d
Fixes for comments on push'ed code:
jeffkenton May 17, 2013
2bcdc3a
Remove fallback for missing helper function (just fail if it's missin…
jeffkenton May 17, 2013
8fbbeed
Change how QuickEdit helper function is passed from CodeHints to Quic…
jeffkenton May 17, 2013
59587ec
Add check for no "functions" returned (as requested by code review).
jeffkenton May 20, 2013
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 25 additions & 1 deletion src/editor/EditorManager.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -94,6 +94,12 @@ define(function (require, exports, module) {
*/
var _inlineEditProviders = [];

/**
* QuickEdit helper function.
* @type {function}
*/
var _quickEditHelper = null;

/**
* Registered inline documentation widget providers. See {@link #registerInlineDocsProvider()}.
* @type {Array.<function(...)>}
Expand Down Expand Up @@ -214,7 +220,23 @@ define(function (require, exports, module) {
function registerInlineEditProvider(provider) {
_inlineEditProviders.push(provider);
}


/**
* Registers a helper for QuickEdit to provide a hook into Tern's jump-to-defintion.
*
* @param {function}
*/
function registerQuickEditHelper(helper) {
_quickEditHelper = helper;
}

/**
* Return QuickEdit helper.
*/
function getQuickEditHelper() {
Copy link
Copy Markdown

@njx njx May 16, 2013

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Copy Markdown
Contributor Author

@jeffkenton jeffkenton May 17, 2013

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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. ]

Copy link
Copy Markdown

@njx njx May 17, 2013

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We chatted about it at the scrum this morning, and came up with two suggested alternatives:

  1. (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.
  2. (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.

Copy link
Copy Markdown
Contributor Author

@jeffkenton jeffkenton May 17, 2013

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

return _quickEditHelper;
}

/**
* Registers a new inline docs provider. When Quick Docs is invoked each registered provider is
* asked if it wants to provide inline docs given the current editor and cursor location.
Expand Down Expand Up @@ -732,6 +754,8 @@ define(function (require, exports, module) {
exports.getFocusedInlineWidget = getFocusedInlineWidget;
exports.resizeEditor = resizeEditor;
exports.registerInlineEditProvider = registerInlineEditProvider;
exports.registerQuickEditHelper = registerQuickEditHelper;
exports.getQuickEditHelper = getQuickEditHelper;
exports.registerInlineDocsProvider = registerInlineDocsProvider;
exports.getInlineEditors = getInlineEditors;
exports.closeInlineWidget = closeInlineWidget;
Expand Down
11 changes: 7 additions & 4 deletions src/editor/MultiRangeInlineEditor.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -126,13 +126,13 @@ define(function (require, exports, module) {
this.$editorsDiv.on("mousewheel.MultiRangeInlineEditor", function (e) {
e.stopPropagation();
});

// Outer container for border-left and scrolling
this.$relatedContainer = $(window.document.createElement("div")).addClass("related-container");

// List "selection" highlight
this.$selectedMarker = $(window.document.createElement("div")).appendTo(this.$relatedContainer).addClass("selection");

// Inner container
this.$related = $(window.document.createElement("div")).appendTo(this.$relatedContainer).addClass("related");

Expand Down Expand Up @@ -165,8 +165,11 @@ define(function (require, exports, module) {
// select the first range
self.setSelectedIndex(0);

// attach to main container
this.$htmlContent.append(this.$relatedContainer).append(this.$editorsDiv);
if (this._ranges.length > 1) { // attach to main container
this.$htmlContent.append(this.$relatedContainer).append(this.$editorsDiv);
Copy link
Copy Markdown

@njx njx May 16, 2013

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd prefer to just break out the .append(this.$editorsDiv) separately:

if (this._ranges.length > 1) {
    this.$htmlContent.append(this.$relatedContainer);
}
this.$htmlContent.append(this.$editorsDiv);

(That might look like it's semantically different, but because of the way jQuery chaining works, a chain of append()s always appends to the initial item--they don't append to each other.)

Copy link
Copy Markdown
Contributor Author

@jeffkenton jeffkenton May 17, 2013

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed.

} else { // no need to show ranges found if there's only one
this.$htmlContent.append(this.$editorsDiv);
}

// Listen for clicks directly on us, so we can set focus back to the editor
var clickHandler = this._onClick.bind(this);
Expand Down
3 changes: 1 addition & 2 deletions src/extensions/default/JavaScriptCodeHints/ScopeManager.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -449,9 +449,8 @@ define(function (require, exports, module) {

var $deferredJump = getPendingRequest(file, offset, HintUtils.TERN_JUMPTODEF_MSG);

// pendingTernRequests[file] = null;

if ($deferredJump) {
response.fullPath = getResolvedPath(response.resultFile);
Copy link
Copy Markdown

@njx njx May 16, 2013

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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().

Copy link
Copy Markdown
Contributor Author

@jeffkenton jeffkenton May 17, 2013

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No problem. Pushed just now.

$deferredJump.resolveWith(null, [response]);
}
}
Expand Down
13 changes: 13 additions & 0 deletions src/extensions/default/JavaScriptCodeHints/main.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -573,9 +573,22 @@ define(function (require, exports, module) {
}
}

/*
* Helper for QuickEdit jump-to-definition request.
*/
function quickEditHelper() {
var offset = session.getOffset(),
response = ScopeManager.requestJumptoDef(session, session.editor.document, offset);

return response;
}

// Register command handler
CommandManager.register(Strings.CMD_JUMPTO_DEFINITION, JUMPTO_DEFINITION, handleJumpToDefinition);

// Register quickEditHelper.
EditorManager.registerQuickEditHelper(quickEditHelper);

// Add the menu item
var menu = Menus.getMenu(Menus.AppMenuBar.NAVIGATE_MENU);
if (menu) {
Expand Down
1 change: 1 addition & 0 deletions src/extensions/default/JavaScriptCodeHints/tern-worker.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -143,6 +143,7 @@ importScripts("thirdparty/requirejs/require.js");

var request = buildRequest(dir, file, "definition", offset, text);
request.query.lineCharPositions = true;
// request.query.typeOnly = true; // FIXME: tern doesn't work exactly right yet.
ternServer.request(request, function (error, data) {
if (error) {
_log("Error returned from Tern 'definition' request: " + error);
Expand Down
101 changes: 82 additions & 19 deletions src/extensions/default/JavaScriptQuickEdit/main.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@ define(function (require, exports, module) {
var MultiRangeInlineEditor = brackets.getModule("editor/MultiRangeInlineEditor").MultiRangeInlineEditor,
FileIndexManager = brackets.getModule("project/FileIndexManager"),
EditorManager = brackets.getModule("editor/EditorManager"),
DocumentManager = brackets.getModule("document/DocumentManager"),
JSUtils = brackets.getModule("language/JSUtils"),
PerfUtils = brackets.getModule("utils/PerfUtils");

Expand Down Expand Up @@ -105,24 +106,85 @@ define(function (require, exports, module) {
function _createInlineEditor(hostEditor, functionName) {
var result = new $.Deferred();
PerfUtils.markStart(PerfUtils.JAVASCRIPT_INLINE_CREATE);

_findInProject(functionName).done(function (functions) {
if (functions && functions.length > 0) {
var jsInlineEditor = new MultiRangeInlineEditor(functions);
jsInlineEditor.load(hostEditor);

PerfUtils.addMeasurement(PerfUtils.JAVASCRIPT_INLINE_CREATE);
result.resolve(jsInlineEditor);
} else {
// No matching functions were found
PerfUtils.addMeasurement(PerfUtils.JAVASCRIPT_INLINE_CREATE);
result.reject();

// Use Tern jump-to-definition helper, if it's available, to find InlineEditor target.
var helper = EditorManager.getQuickEditHelper();
if (helper !== null) {
var response = helper();
if (response.hasOwnProperty("promise")) {
response.promise.done(function (jumpResp) {
var resolvedPath = jumpResp.fullPath;
if (resolvedPath) {

// Tern doesn't always return entire function extent.
// Use QuickEdit search now that we know which file to look at.
Copy link
Copy Markdown

@njx njx May 16, 2013

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was actually a little surprised to find that we're not using Tern to resolve to the actual definition when calling a function in a module where the exports are assigned separately from the definition (see #3863). I think your approach here will probably work for most cases (since most people would name the export after the original function anyway), but I wonder if we could leverage Tern to trace that somehow?

Copy link
Copy Markdown
Contributor Author

@jeffkenton jeffkenton May 17, 2013

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We asked Marijn to tweak Tern to provide this information, but didn't get what we needed. This way works, although it's a hack.

Copy link
Copy Markdown

@njx njx May 17, 2013

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's leave this in for now, but keep #3863 open on doing it using Tern--if/when that gets fixed, we can take this out.

var fileInfos = [];
fileInfos.push({name: jumpResp.resultFile, fullPath: resolvedPath});
JSUtils.findMatchingFunctions(functionName, fileInfos)
.done(function (functions) {
var jsInlineEditor = new MultiRangeInlineEditor(functions);
jsInlineEditor.load(hostEditor);

PerfUtils.addMeasurement(PerfUtils.JAVASCRIPT_INLINE_CREATE);
result.resolve(jsInlineEditor);
})
.fail(function () {
PerfUtils.addMeasurement(PerfUtils.JAVASCRIPT_INLINE_CREATE);
result.reject();
});

} else { // no result from Tern. Fall back to _findInProject().

if (!functionName) {
Copy link
Copy Markdown

@njx njx May 16, 2013

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like we should fail much earlier if this is the case :)

Copy link
Copy Markdown

@njx njx May 16, 2013

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(i.e., if we need to detect this, it should be at the top of the function.)

Copy link
Copy Markdown
Contributor Author

@jeffkenton jeffkenton May 17, 2013

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test is will go back into the calling function (where it was originally), and I will remove it here. I had done what you see so that the Tern jump-to-definition part of the search might also find var names, but that turned out to have other problems. So, QuickEdit only finds function names, as it always has.

result.reject();
} else {

_findInProject(functionName).done(function (functions) {
Copy link
Copy Markdown

@njx njx May 16, 2013

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should encapsulate both of the fallback cases (this one and the one on line 170 below) into a common function.

Copy link
Copy Markdown

@njx njx May 16, 2013

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It also feels like since the done/fail handlers once you do have a set of functions is the same in all three cases (the success case above plus these two failure cases), there should be some clever way to factor them all out.

Copy link
Copy Markdown
Contributor Author

@jeffkenton jeffkenton May 17, 2013

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, if we're willing to assume that there should always a helper function registered -- there should be -- we can just remove the second fallback and fail immediately if Jump-To-Definition isn't available. We'll still have the fallback if Tern doesn't find anything.

On the whole, if we leave the second fallback in there, I don't thinks it's enough code to factor out into a separate function.

Copy link
Copy Markdown

@njx njx May 17, 2013

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's fine to remove the helper function fallback. Still would like to see the other two encapsulated if they're literally the same code (I just have a visceral reaction to duplication of more than a couple of lines :))

if (functions && functions.length > 0) {
var jsInlineEditor = new MultiRangeInlineEditor(functions);
jsInlineEditor.load(hostEditor);

PerfUtils.addMeasurement(PerfUtils.JAVASCRIPT_INLINE_CREATE);
result.resolve(jsInlineEditor);
} else {
// No matching functions were found
PerfUtils.addMeasurement(PerfUtils.JAVASCRIPT_INLINE_CREATE);
result.reject();
}
}).fail(function () {
PerfUtils.finalizeMeasurement(PerfUtils.JAVASCRIPT_INLINE_CREATE);
result.reject();
});
}
}

}).fail(function () {
PerfUtils.finalizeMeasurement(PerfUtils.JAVASCRIPT_INLINE_CREATE);
result.reject();
});

}
}).fail(function () {
PerfUtils.finalizeMeasurement(PerfUtils.JAVASCRIPT_INLINE_CREATE);
result.reject();
});


} else {

_findInProject(functionName).done(function (functions) {
if (functions && functions.length > 0) {
var jsInlineEditor = new MultiRangeInlineEditor(functions);
jsInlineEditor.load(hostEditor);

PerfUtils.addMeasurement(PerfUtils.JAVASCRIPT_INLINE_CREATE);
result.resolve(jsInlineEditor);
} else {
// No matching functions were found
PerfUtils.addMeasurement(PerfUtils.JAVASCRIPT_INLINE_CREATE);
result.reject();
}
}).fail(function () {
PerfUtils.finalizeMeasurement(PerfUtils.JAVASCRIPT_INLINE_CREATE);
result.reject();
});
}

return result.promise();
}

Expand All @@ -147,14 +209,15 @@ define(function (require, exports, module) {
if (sel.start.line !== sel.end.line) {
return null;
}

// Always use the selection start for determining the function name. The pos
// parameter is usually the selection end.
var functionName = _getFunctionName(hostEditor, sel.start);

if (!functionName) {
return null;
}

return _createInlineEditor(hostEditor, functionName);
}

Expand Down