Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Picks up the highlighted text for file search #194906

Closed
wants to merge 1,041 commits into from
Closed

Picks up the highlighted text for file search #194906

wants to merge 1,041 commits into from

Conversation

nikhilkarve
Copy link

@nikhilkarve nikhilkarve commented Oct 5, 2023

Fixes #194573

@nikhilkarve

This comment was marked as resolved.

@nikhilkarve
Copy link
Author

nikhilkarve commented Oct 6, 2023

@TylerLeonhardt also as October has started, I was wondering if can we add hacktoberfest-accepted label to this when all looks good? 😃

@TylerLeonhardt
Copy link
Member

@nikhilkarve you don't have to worry about updating the branch. That's not a blocker for merging. Also, I marked the PR as hacktoberfest-accepted so you should get credit for it... unfortunately, my life is a little chaotic right now so I can't look at this fully right now.

}

// only happen if it would also happen for the search view
const seedSearchStringFromSelection = configurationService.getValue<boolean>('editor.find.seedSearchStringFromSelection');
Copy link
Member

Choose a reason for hiding this comment

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

We should have our own setting. Follow what we did for preserveInput

Copy link
Author

Choose a reason for hiding this comment

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

Got it, added that! Great suggestion @TylerLeonhardt . Let me know if that's correct! Thanks

@TylerLeonhardt
Copy link
Member

btw, I opened these two issues on the quickTextSearch #194675 and #194835

see if you experience these same bugs in your code as well. If you do, can you fix them in your code please 🙏 I want to merge this PR in without those same bugs.

Also, if you figure out those bugs in your code... you can send us 2 additional PRs fixing the bugs in the quickTextSearch too giving you more hacktoberfest PRs :) if you're interested.

@nikhilkarve
Copy link
Author

btw, I opened these two issues on the quickTextSearch #194675 and #194835

see if you experience these same bugs in your code as well. If you do, can you fix them in your code please 🙏 I want to merge this PR in without those same bugs.

Also, if you figure out those bugs in your code... you can send us 2 additional PRs fixing the bugs in the quickTextSearch too giving you more hacktoberfest PRs :) if you're interested.

Sure, I would be more than happy to take a look at those issues as well. No problem, I was anyways looking for anymore issues where I can contribute.

@07tAnYa
Copy link

07tAnYa commented Oct 12, 2023

@TylerLeonhardt Sir if you could add more issues under hactoberfest label, then it would be very benificial for new contributors like me.

@@ -382,6 +382,11 @@ configurationRegistry.registerConfiguration({
'description': nls.localize('search.experimental.quickAccess.preserveInput', "Controls whether the last typed input to Quick Search should be restored when opening it the next time."),
'default': false
},
'search.experimental.quickAccess.seedFileSearchStringFromSelection': {
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think this needs to be an experimental setting. I think search.quickOpen.seedStringFromSelection should be sufficient.

Copy link
Author

Choose a reason for hiding this comment

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

Got it, will update that

@@ -382,6 +382,11 @@ configurationRegistry.registerConfiguration({
'description': nls.localize('search.experimental.quickAccess.preserveInput', "Controls whether the last typed input to Quick Search should be restored when opening it the next time."),
'default': false
},
'search.experimental.quickAccess.seedFileSearchStringFromSelection': {
'type': 'boolean',
'description': nls.localize('search.experimental.quickAccess.seedFileSearchStringFromSelection', "Controls whether the search string in the Search File Widget is seeded from the editor selection."),
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of Search File Widget, we should use Quick Open.

Copy link
Author

Choose a reason for hiding this comment

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

Got it will change

@nikhilkarve
Copy link
Author

Hi @andreamah I pushed the changes as per your comment. Let me know if that is correct, otherwise I will push what's required asap

Comment on lines 51 to 90
export function getFileSelectionTextFromEditor(allowUnselectedWord: boolean, editor: IEditor): string | null {

if (!isCodeEditor(editor) || !editor.hasModel()) {
return null;
}

const range = editor.getSelection();
if (!range) {
return null;
}

if (range.isEmpty()) {
if (allowUnselectedWord) {
const wordAtPosition = editor.getModel().getWordAtPosition(range.getStartPosition());
return wordAtPosition?.word ?? null;
} else {
return null;
}
}

let searchText = '';
for (let i = range.startLineNumber; i <= range.endLineNumber; i++) {
let lineText = editor.getModel().getLineContent(i);
if (i === range.endLineNumber) {
lineText = lineText.substring(0, range.endColumn - 1);
}

if (i === range.startLineNumber) {
lineText = lineText.substring(range.startColumn - 1);
}

if (i !== range.startLineNumber) {
lineText = '\n' + lineText;
}

searchText += lineText;
}

return searchText;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

just noticed this, but I think that

export function getSelectionTextFromEditor(allowUnselectedWord: boolean, activeEditor: IEditor): string | null {
let editor = activeEditor;
if (isDiffEditor(editor)) {
if (editor.getOriginalEditor().hasTextFocus()) {
editor = editor.getOriginalEditor();
} else {
editor = editor.getModifiedEditor();
}
}
if (!isCodeEditor(editor) || !editor.hasModel()) {
return null;
}
const range = editor.getSelection();
if (!range) {
return null;
}
if (range.isEmpty()) {
if (allowUnselectedWord) {
const wordAtPosition = editor.getModel().getWordAtPosition(range.getStartPosition());
return wordAtPosition?.word ?? null;
} else {
return null;
}
}
let searchText = '';
for (let i = range.startLineNumber; i <= range.endLineNumber; i++) {
let lineText = editor.getModel().getLineContent(i);
if (i === range.endLineNumber) {
lineText = lineText.substring(0, range.endColumn - 1);
}
if (i === range.startLineNumber) {
lineText = lineText.substring(range.startColumn - 1);
}
if (i !== range.startLineNumber) {
lineText = '\n' + lineText;
}
searchText += lineText;
}
return searchText;
}

has pretty much the same code, but also makes sure to get text from diff editors. Can you reuse that?

Copy link
Author

@nikhilkarve nikhilkarve Oct 31, 2023

Choose a reason for hiding this comment

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

I tried that earlier, I get the following error if I do that while committing

Imports violates 'vs/css!.//* or vs/base/{common,browser}/ or vs/base/parts//{common,browser}/** or vs/platform//{common,browser}/** or vs/editor/{common,browser}/** or vs/editor/contrib//{common,browser}/** or vs/workbench/{common,browser}/** or vs/workbench/services//{common,browser}/** or assert or vs/nls or vs/amdX' restrictions.

Its not letting me import anything from the contrib folder.
https://github.com/microsoft/vscode/wiki/Source-Code-Organization

Copy link
Author

Choose a reason for hiding this comment

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

One way I can think of is writing this function somewhere, probably a folder from which is import is allowed. And then reusing it in both the places. Not sure exactly where though. Are there any conventions?

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm okay with this code living in quickaccess.ts as long as you have the most up-to-date version of the function (including the newest changes from today).
@TylerLeonhardt are you okay with the selection-grabbing code being here and search just importing it? Ideally, the selection code could be somewhere else more communal in the future and be shared by everything that gets editor selection.

Copy link
Author

Choose a reason for hiding this comment

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

Shall I import the function from quickaccess.ts to searchView.ts then? I think this import is possible. I can change the function name from getFileSelectionTextFromEditor to getSelectionTextFromEditor

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, the function would only occur once in quickAccess.ts. Make sure that you have the part

 	if (isDiffEditor(editor)) { 
 		if (editor.getOriginalEditor().hasTextFocus()) { 
 			editor = editor.getOriginalEditor(); 
 		} else { 
 			editor = editor.getModifiedEditor(); 
 		} 
 	} 

Since it looks like your current implementation doesn't have it and it's needed to get the selection in diff editors.

Copy link
Author

Choose a reason for hiding this comment

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

Sure, I will add that too

Copy link
Member

Choose a reason for hiding this comment

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

Yes I think having this in quickaccess.ts for this and search to use makes sense.

Copy link
Author

Choose a reason for hiding this comment

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

I updated the PR with those changes, let me know if this is correct!

@andreamah
Copy link
Contributor

looks good! one more thing: can you resolve the merge conflicts with main?

roblourens and others added 15 commits October 31, 2023 21:48
* Add sessionId, agent, and slash command to core telemetry

* Enable used references by default
…t-response

Revert "accept chat response when session is released, prevent audio cue loop from continuing"
* aux window - disable main menu

* 💄
Tyriar and others added 24 commits October 31, 2023 22:06
Adopt documentOverride in xterm.js
Start moving window/document to use layout service
Allow hiding of launcher on Run and Debug title bar when debugging
Show Reload Required buttons simultaneously after updateAllExtensions (#163627)
Add multi-select renames to terminalAction
Pass terminal context to relevant actions
Layout when switching from welcome to terminal.
@nikhilkarve
Copy link
Author

nikhilkarve commented Nov 1, 2023

looks good! one more thing: can you resolve the merge conflicts with main?

There are a lot of conflicts not sure why. Is it okay if I create a new branch from the current main and create a new PR?

@TylerLeonhardt
Copy link
Member

Creating a new PR is totally ok :)

@nikhilkarve nikhilkarve closed this by deleting the head repository Nov 3, 2023
nikhilkarve pushed a commit to nikhilkarve/vscode that referenced this pull request Nov 3, 2023
@nikhilkarve
Copy link
Author

@TylerLeonhardt @andreamah
I have created a new PR with latest changes. I am sorry for the trouble!
#197380

@github-actions github-actions bot locked and limited conversation to collaborators Dec 18, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Search Files By Name Should Use Highlighted Text