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

LSP Find References Feature #14693

Merged
merged 14 commits into from
Apr 15, 2019
Merged

LSP Find References Feature #14693

merged 14 commits into from
Apr 15, 2019

Conversation

niteskum
Copy link
Collaborator

No description provided.

@niteskum
Copy link
Collaborator Author

niteskum commented Apr 10, 2019

@shubhsnov @swmitra Please review
@narayani28

@niteskum niteskum changed the title LSP Find References Feature LSP Find References Feature < work in Progress> Apr 10, 2019
@@ -0,0 +1,151 @@
/*
* Copyright (c) 2013 - present Adobe Systems Incorporated. All rights reserved.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Fix the banner. Copyright (c) 2019 - present Adobe. All rights reserved.


var model = searchModel;
_resultsView = new SearchResultsView(
model,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Directly use searchModel here.

),
removeFindReferencesProvider = _providerRegistrationHandler.removeProvider.bind(_providerRegistrationHandler);

var SHOW_FIND_REFERENCES_CMD_ID = "showrReferences",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nit: Typo "showReferences"

searchModel.numMatches = rcvdObj.numMatches;
searchModel.allResultsAvailable = true;
searchModel.setQueryInfo({query: rcvdObj.queryInfo, caseSensitive: true, isRegExp: false});
//searchModel.fireChanged();
Copy link
Collaborator

Choose a reason for hiding this comment

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

Remove commented code.

}

function _openReferencesPanel() {

Copy link
Collaborator

Choose a reason for hiding this comment

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

Remove empty line.

referencespromise = _getReferences(referencesProvider, editor, pos);

// Use default error message if none other provided
errorMsg = errorMsg || defaultErrorMsg;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Where are errorMsg and defaultErrorMsg being set?

src/languageTools/DefaultProviders.js Show resolved Hide resolved
};

ReferencesProvider.prototype.getReferences = function() {
var editor = EditorManager.getActiveEditor(),
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why aren't you using the hostEditor and pos that is being passed by manager?

@@ -72,14 +72,16 @@ define(function (require, exports, module) {
* @param {SearchModel} model The model that this view is showing.
* @param {string} panelID The CSS ID to use for the panel.
* @param {string} panelName The name to use for the panel, as passed to WorkspaceManager.createBottomPanel().
* @param {string} type type to indentify if it is reference search or string match serach
Copy link
Collaborator

Choose a reason for hiding this comment

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

Typo: identify

Copy link
Collaborator

Choose a reason for hiding this comment

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

You still need to fix this.

src/search/SearchResultsView.js Outdated Show resolved Hide resolved
@shubhsnov
Copy link
Collaborator

shubhsnov commented Apr 11, 2019

@niteskum I've still got to check the functional aspects, but I've given a few code nits that I came across on first glance.

Could you also please add a proper description for the PR along with the UI aspects.

@niteskum
Copy link
Collaborator Author

niteskum commented Apr 11, 2019

Small Modifications in SerachResultsView class to show references results.

Find References UI:

On Right Click:
image

References Results UI:
image

@niteskum niteskum changed the title LSP Find References Feature < work in Progress> LSP Find References Feature Apr 11, 2019
@shubhsnov
Copy link
Collaborator

@niteskum The string should get highlighted as in search results. Is there any particular reason for why this is not happening?
image

@niteskum
Copy link
Collaborator Author

niteskum commented Apr 12, 2019

@niteskum The string should get highlighted as in search results. Is there any particular reason for why this is not happening?

@shubhsnov fixed it.

src/features/keyboard.json Outdated Show resolved Hide resolved
@@ -72,14 +72,16 @@ define(function (require, exports, module) {
* @param {SearchModel} model The model that this view is showing.
* @param {string} panelID The CSS ID to use for the panel.
* @param {string} panelName The name to use for the panel, as passed to WorkspaceManager.createBottomPanel().
* @param {string} type type to indentify if it is reference search or string match serach
Copy link
Collaborator

Choose a reason for hiding this comment

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

You still need to fix this.

src/features/FindReferencesManager.js Outdated Show resolved Hide resolved
src/features/keyboard.json Outdated Show resolved Hide resolved
src/languageTools/DefaultProviders.js Outdated Show resolved Hide resolved
@shubhsnov
Copy link
Collaborator

shubhsnov commented Apr 13, 2019

Apart from a few refactoring changes, this looks good to me.

@niteskum
Copy link
Collaborator Author

Apart from a few refactoring changes, this looks good to me.

@shubhsnov Addressed all review comments.
for "Find All References" I am using "Ctrl-Shift-K" shortcut as "Ctrl-Shift-R" is already used for reloadLivePreview.

src/languageTools/DefaultProviders.js Outdated Show resolved Hide resolved
src/features/FindReferencesManager.js Show resolved Hide resolved
src/features/FindReferencesManager.js Outdated Show resolved Hide resolved
src/extensions/default/PhpTooling/main.js Show resolved Hide resolved
@narayani28
Copy link
Collaborator

ctrl+k is used for quick docs and ctrl+shift+k is not very intuitive. is there any non used ctrl+ available for usage?

@shubhsnov
Copy link
Collaborator

LGTM 👍

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants