Find: No find history

[egamma]

Testing #29479

The find widget in the editor provides a find history (alt+up/alt+down), there is currently no such support in the terminal.

[cleidigh]

@egamma
@Tyriar
Is there any reason not to have the full Find Widget with case sensitivity and regex?
Would xterm prevent this?

[Tyriar]

@cleidigh no reason, I hope to eventually close the gap but it's not the highest priority thing for me. A bunch of improvements are laid out in #28768, most of these would need changes in both https://github.com/sourcelair/xterm.js and vscode.

Find history only needs changes in vscode though.

[cleidigh]

@Tyriar
Sounds like adding these key bindings is worthwhile in the meantime?

[Tyriar]

@cleidigh yep, ideally the editor and terminal find act as close to one another as possible. Also note that the webview find dialog relies on the same widget (Help > Release Notes, ctrl+f).

[cleidigh]

@Tyriar
I will add the find history after I do the next and previous keyboard shortcuts

[cleidigh]

@Tyriar
Quick question
It appears it will be necessary to duplicate history code from the main Find widget. Is this okay?
I assume this can be re- factored when a base Find widget can be done that will work in both situations.

[cleidigh]

@Tyriar
I know you guys are in end game so I know this will wait till later, just FYI:
I added find history to the simple find widget with support in the terminal widget
but I will wait to do a PR until after the next /previous PR is done since it's all the same files.

Seems like I should modify the webviewfindwidget with these changes also as separate PR, yes?

[Tyriar]

@cleidigh I would hope the majority of the history code is kept within the component that's shared between the terminal and the webview. I guess with keybindings being the main part outside.

@rebornix any advice on the right way of adding find history? Are we moving towards just sharing everything between the editor, terminal and webview? Is that an option?

[cleidigh]

@Tyriar
@rebornix
I already have an implementation.

• Put everything possible into the base SimpleFindwidget, next/previous functions, NavigatorHistory etc
• Terminal key bindings, layer access via terminal service
• AddingTo Web view should be fairly straightforward assuming in a separate PR
• Have to add appropriate OS X bindings, final cleanup
• Repository Link: (will do PR after next / previous PR and your guys thumbs up
  https://github.com/cleidigh/vscode/tree/terminal-find-history/add

[cleidigh]

@Tyriar
@rebornix
Simple Find Widget Status Update:

• PR Terminal: Add Find next/previous key bindings. Fixes: #29661 #31418 Adds next/previous to widget and integrated terminal (ready)
• PR # x - will add find history to widget and integrated terminal - see repository - (will post after above)
• PR # y - I have working staged code for next/previous and find history for WebView (both extensionEditor and webViewEditor)
• Realized I have to add find widget context to handle history key bindings
• Did a bit of re-factoring. Created an abstract class for webviewEditor and extensionEditor commands --- Matches preferencesEditor - see below
• Could do above as one or two PR's with associated issue posts, up to you guys

If the approach shown in the repository and PR are good for you guys I will post the other PRs, issues relative to the web view.

After these PR 's all simple find widgets will be the same. They will only lack
case sensitivity and regex options.

abstract class ExtensionEditorCommand extends Command {

public abstract runCommand(accessor: ServicesAccessor, args: any): void;

protected getExtensionEditor(accessor: ServicesAccessor): ExtensionEditor {
	const activeEditor = accessor.get(IWorkbenchEditorService).getActiveEditor() as ExtensionEditor;
	if (activeEditor instanceof ExtensionEditor) {
		return activeEditor;
	}
	return null;
}

}

class ShowExtensionEditorFindCommand extends ExtensionEditorCommand {
public runCommand(accessor: ServicesAccessor, args: any): void {
const extensionEditor = this.getExtensionEditor(accessor);
if (extensionEditor) {
extensionEditor.showFind();
}
}
}
const showCommand = new ShowExtensionEditorFindCommand({
id: 'editor.action.extensioneditor.showfind',
precondition: KEYBINDING_CONTEXT_EXTENSIONEDITOR_WEBVIEW_FOCUS,
kbOpts: {
primary: KeyMod.CtrlCmd | KeyCode.KEY_F
}
});
KeybindingsRegistry.registerCommandAndKeybindingRule(showCommand.toCommandAndKeybindingRule(KeybindingsRegistry.WEIGHT.editorContrib()));

[rebornix]

@Tyriar moving things into a shared component between editor and others is the right solution to me, especially when the search viewlet has the same feature.

@cleidigh I like your proposals. Right now we call it simple find widget as it lacks quite a few functionalities comparing to the one in Monaco but I'd like to have them merged as they should be just options to enable/disable instead of having a new component. We need to do some abstraction similar to how we share Find Input Box. I'm not sure yet if we want to refactor webview/extensions editor at this moment. Do you mind if I do some refactoring on the find widget part as you already did some work there?

[cleidigh]

@rebornix
Just to be clear I already have 95% done including the parallel work on webview and extension editor.
As part of the work done I have done re- factoring of the simple find widget to support
both next and previous find as well as find history. The only thing I am almost done on is adding a
find input FocusContext so the history key bindings only take effect in the input box.
I think I have everything pretty clean, I just have not posted the total solution since it's somewhat sequential.
How about I post my repository with everything I have so nothing gets duplicated work wise ?
I agree about making the find combined, I would be happy to work on that since it makes the most sense.
Let me know how you would like me to proceed.
Cheers

[rebornix]

@cleidigh I'll put this on my iteration plan for August. Let's start all the work from your pull request as you are almost done there and then I'll see what I can help, make sense?

[cleidigh]

@rebornix
Okay, it sounds like I should combine everything into one PR.
if you like that, I will actually post a new PR representing the combined work AND getting up-to-date.
I'm hoping that by inspection I've got everything in a layered manner you like.

[weinand]

@rebornix since this is a closed feature request, please add either 'on-testplan' or 'verification-needed' label.

[rebornix]

@weinand thanks for the notification. It's already covered in test plan.