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

LSP Find References Feature#14693

Merged
shubhsnov merged 14 commits intoadobe:masterfrom
niteskum:lspFindReferences
Apr 15, 2019
Merged

LSP Find References Feature#14693
shubhsnov merged 14 commits intoadobe:masterfrom
niteskum:lspFindReferences

Conversation

@niteskum
Copy link
Copy Markdown
Collaborator

No description provided.

@niteskum
Copy link
Copy Markdown
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
Comment thread src/features/FindReferencesManager.js Outdated
@@ -0,0 +1,151 @@
/*
* Copyright (c) 2013 - present Adobe Systems Incorporated. All rights reserved.
Copy link
Copy Markdown
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.

Comment thread src/features/FindReferencesManager.js Outdated

var model = searchModel;
_resultsView = new SearchResultsView(
model,
Copy link
Copy Markdown
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.

Comment thread src/features/FindReferencesManager.js Outdated
),
removeFindReferencesProvider = _providerRegistrationHandler.removeProvider.bind(_providerRegistrationHandler);

var SHOW_FIND_REFERENCES_CMD_ID = "showrReferences",
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Nit: Typo "showReferences"

Comment thread src/features/FindReferencesManager.js Outdated
searchModel.numMatches = rcvdObj.numMatches;
searchModel.allResultsAvailable = true;
searchModel.setQueryInfo({query: rcvdObj.queryInfo, caseSensitive: true, isRegExp: false});
//searchModel.fireChanged();
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Remove commented code.

Comment thread src/features/FindReferencesManager.js Outdated
}

function _openReferencesPanel() {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Remove empty line.

Comment thread src/features/FindReferencesManager.js Outdated
referencespromise = _getReferences(referencesProvider, editor, pos);

// Use default error message if none other provided
errorMsg = errorMsg || defaultErrorMsg;
Copy link
Copy Markdown
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?

Comment thread src/languageTools/DefaultProviders.js
Comment thread src/languageTools/DefaultProviders.js Outdated
};

ReferencesProvider.prototype.getReferences = function() {
var editor = EditorManager.getActiveEditor(),
Copy link
Copy Markdown
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?

Comment thread src/search/SearchResultsView.js Outdated
* @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
Copy Markdown
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
Copy Markdown
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.

Comment thread src/search/SearchResultsView.js Outdated
@shubhsnov
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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.

Comment thread src/features/keyboard.json Outdated
Comment thread src/search/SearchResultsView.js Outdated
* @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
Copy Markdown
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.

Comment thread src/features/FindReferencesManager.js Outdated
Comment thread src/features/keyboard.json Outdated
Comment thread src/languageTools/DefaultProviders.js Outdated
@shubhsnov
Copy link
Copy Markdown
Collaborator

shubhsnov commented Apr 13, 2019

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

@niteskum
Copy link
Copy Markdown
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.

Comment thread src/languageTools/DefaultProviders.js Outdated
Comment thread src/features/FindReferencesManager.js
Comment thread src/features/FindReferencesManager.js Outdated
Comment thread src/extensions/default/PhpTooling/main.js
@narayani28
Copy link
Copy Markdown
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
Copy Markdown
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