SimpleFindWidget Add next,previous,history: Fixes #29708 #29661 #32113

cleidigh · 2017-08-08T02:50:12Z

Replaced PR SimpleFindWidget Add next,previous,history: Fixes #29708 #29661 #32074
Of course the last five percent was all the work
The need for a new context for FindInput Focused was somewhat messy
Had to touch more files than expected, was necessary as there are four clients
Re-FactoringTheFindWidget for common use everywhere will take quite a bit of worksince it's so intertwined with the editor
Hopefully this is useful until we re- factor TheWidget
Fixes Find: No find history #29708 No next and previous keyboard shortcuts for Find widget in Integrated Terminal #29661 also services the modified simpleFindWidget to all users
--

rebornix · 2017-08-08T17:18:03Z

product.json

you don't want to check in this, right :)

cleidigh · 2017-08-08T17:34:07Z

@rebornix
I kept it in so the extension editor could be tested, not intended to be merged.
Similar issue with the release notes editor, could not figure out a way to test that client
since the update facilities are not exposed in the OSS

BTW I saw in the August milestones the re- factoring of the FindWidget and a red ball indicating
no associated issue. Shall I create an issue for discussion there?

rebornix · 2017-08-08T18:22:05Z

src/vs/workbench/parts/terminal/electron-browser/terminalFindWidget.ts

@@ -44,4 +47,12 @@ export class TerminalFindWidget extends SimpleFindWidget {
 	protected onFocusTrackerBlur() {
 		this._terminalService.getActiveInstance().notifyFindWidgetFocusChanged(false);
 	}
+
+	protected onFindInputFocusTrackerFocus() {


We'd better not expose input box to external callers, let's handle input, checkboxes inside the widget.

@rebornix
I really wanted to hide this further but for right now the context key has to come from above

rebornix · 2017-08-08T18:32:57Z

src/vs/workbench/parts/extensions/browser/extensionEditor.ts

@@ -54,12 +53,14 @@ import { IContextKeyService, RawContextKey, ContextKeyExpr, IContextKey } from '
 import { Command } from 'vs/editor/common/editorCommonExtensions';
 import { IWorkbenchEditorService } from 'vs/workbench/services/editor/common/editorService';
 import { KeybindingsRegistry } from 'vs/platform/keybinding/common/keybindingsRegistry';
+import { default as WebView, KEYBINDING_CONTEXT_WEBVIEW_FIND_WIDGET_INPUT_FOCUSED } from 'vs/workbench/parts/html/browser/webview';


Let's align the casing this time, use Webview always instead of mix of both WebView and Webview.

rebornix · 2017-08-08T18:34:21Z

src/vs/workbench/parts/html/browser/webviewFindWidget.ts

 	) {
 		super(_contextViewService);

+		this._findInputFocused = _findInputContextKey.bindTo(this._contextKeyService);
+		console.debug(this._findInputContextKey.keys());


Doh, have to get find in open files working! Bad stray console lines.

rebornix · 2017-08-08T18:42:30Z

src/vs/workbench/parts/terminal/electron-browser/terminal.contribution.ts

+actionRegistry.registerWorkbenchAction(new SyncActionDescriptor(NextMatchTerminalFindWidgetAction, NextMatchTerminalFindWidgetAction.ID, NextMatchTerminalFindWidgetAction.LABEL, {
+	primary: KeyCode.F3,
+	mac: { primary: KeyMod.CtrlCmd | KeyCode.KEY_G, secondary: [KeyCode.F3] }
+}, KEYBINDING_CONTEXT_TERMINAL_FOCUS), 'Terminal: Find Next', category);


Terminal: prefix is redundant.

rebornix · 2017-08-08T18:51:44Z

Issues found while playing with it locally

Open Extension Editor; Search keyword a, navigate through results; Esc to close the widget; Invoke Find Widget again, the old keyword a is still in the widget, type in another keyword b; Do not press enter, try to navigate through history but nothing happens.
When we hide the find widget and run find next/previous in terminal/extension editor, the find widget is not toggled. However the find widget is toggled in code editor. We should align this.

cleidigh · 2017-08-08T19:44:56Z

@rebornix
The search term is added when Enter pressed, otherwise we would have to incrementally add
every keystroke to the navigation history using onInputChanged
The first search term should be in the navigation history, I would think this issue
would be located in the navigation history class perhaps?

Update:
It seems that the editor find widget does seem to add keystroke by keystroke to the history
maybe with the delay so if someone types the word fast only the full word will be added in the history
I can somewhat replicate this by voice going letter by letter versus a fast word

The editor FindWidget does use an update history delay:
this._updateHistoryDelayer = new Delayer(500);
this._currentHistoryNavigator = new HistoryNavigator();

I can make this parallel

cleidigh · 2017-08-08T22:49:31Z

@rebornix

Matched editor find widget delayed update history on all entry
Show Find if not visible for next previous find
There are still some naming inconsistencies I inherited and had to choose as opposed to changing lots of names

A few more adds and will have two competing find widgets ;-)

rebornix · 2017-08-08T23:24:13Z

@cleidigh your latest change looks good to me. Currently I only have one question left, is it necessary to create keyshorts for ShowNextFind/ShowPreviousFind for every context (Terminal, Extension Editor, Webview)? The key shortcuts are all the same and the context doesn't affect the behavior. You may want to simply create two generic commands which covers all of them.

FindPrevious/FindNext is good, they sit where they should be 👍

@mjbvz it makes a few changes to webview so I'd like to have you review it.

rebornix · 2017-08-08T23:31:46Z

We can probably have a generic context key called FindWidgetInputBoxFocus, similar to the existing searchInputBoxFocus, and history navigation commands only apply this one single context.

mjbvz

Webview changes look good overall. I'd like to see if we can minimize the number of duplicate commands though and just have a single set of commands for navigating any find widget's history

cleidigh · 2017-08-09T01:01:39Z

@rebornix
@mjbvz
I totally agree, and have been thinking that when we create a single find widget we should have global commands regardless of client.
I had some difficulties with access the context Constants , I will see if I can combine them
may have to do it tomorrow.
Where would it be appropriate to add the command entries ?
Also there will be some issue with naming convention, what is a clean differentiation
between next/ previous Find history for the editor FindWidget and the simple find widget
commands that would be relevant for the terminal and Webview clients?
Once there is a single find widget there should only be a single set of commands for sure.

Lastly, it seems like we would have to demultiplex somewhere in the middle of the call stack
based on the client since the owner will be different in each case even if the top level command is the same.

cleidigh · 2017-08-09T01:37:49Z

@rebornix
@mjbvz
okay my inclination is to create simpleFindController that has the global commands
and accesses the simple find Widget history functions directly using a static registry
to map the focused input to the appropriate widget.
But, this could just be
the vino clouding my thinking ! I will give it a go

rebornix · 2017-08-09T17:51:58Z

@cleidigh there are several possible approaches

Follow the list view. We have a lot of list views in the workbench, like File Explorer, Search, etc but you can use the same set of list operations on anyone of them and we don't register methods for everyone. The way is creating a standalone service for find widget registering and access it when the command is triggered, instead of accessing the host (Terminal or Webview) and then accessing the find widget on it. You can take a look at https://github.com/Microsoft/vscode/blob/master/src/vs/platform/list/browser/listService.ts#L16 and find its references.
Creating a controller like you proposed. Right now we are only trying to share View code instead of making it a full-featured component. Wrapping FindWidget into a controller and reference the controller directly sounds pretty good.
This might not be a good option but do we necessarily need commands for history navigation? If not, we can just bind operations to keyboard events in the input box. But the catch is users lost the ability to change it to their favorite shortcuts.

The first approach is more intuitive and straightforward to me.

cleidigh · 2017-08-09T18:24:20Z

@rebornix
The list service approach is also what I had in mind. I was going to start with a simple approach
using static methods and properties within the simple find widget. I think the service will be useful
in the long run if we are going to consolidate as many commands as possible across the various uses
of the Widget.

I don't like the hardwired Keybord event handling within the widget either.

I will work on option one, probably take me a bit given my limitations and learning to do it correctly.
it will be one more good learning experience, I'll try not to bug you too much.

rebornix · 2017-08-09T21:41:07Z

@cleidigh no worries, ping me whenever you need suggestions or so. Thanks for doing most of the code work for me, it used to be my debt task :)

cleidigh · 2017-08-11T20:06:32Z

@rebornix
I don't want to lose the weekend banging my head !
I have a service set up, service decorator and instantiation okay
Also the initial separation for making global commands and a FindInputFocused context

I thought I had the right place to add the simple find widget command contributions:
'vs/editor/contrib/common/simpleFindWidget.contribution'
This does not seem to get executed yet I believe it follows the other conventions.
Is there are some registration that I'm missing that will allow these commands to be registered?

Thanks

rebornix · 2017-08-11T22:17:10Z

@cleidigh did you add this file to https://github.com/Microsoft/vscode/blob/master/src/vs/editor/editor.all.ts#L44 ?

cleidigh · 2017-08-11T23:14:14Z

@rebornix
That was the trick, nothing like hidden files!
For my reference:
I see find.ts imported by editor.all
Where or how is findController.ts imported for its commands? That was the breadcrumb I was trying to follow.

rebornix · 2017-08-11T23:55:35Z

@cleidigh for editor contributions, we usually have a controller, in which we registers commands, and the controller usually gets added to editor.all. I agree it's not intuitive but consider it as a nodejs application, if you add a file and no one else references it, then it's not loaded :)

BTW, I really appreciate your help on this but I don't want it to take too much of your time. So we can make the PR focus on you intended to do and have it merge fast. Then everyone interested in this topic can jump in and help fix things. If you need any kind of help, let me know, you can even add me to your fork and let me contribute there (fix annoying hidden magic things like this one).

Have a good weekend.

cleidigh

works save placeholder - cleanup to follow Monday afternoon

cleidigh · 2017-08-14T19:47:57Z

@rebornix

Consolidate all SimpleFind code inside widget
Service to access widget from anywhere
Removed all duplicated commands inside clients
Clients instantiate and register widget
Added next,previous and history inside widget

Commenting on your remarks Friday:
Since March I have been basically working full-time on VS Code. This is good for me and I don't mind.
In fact if you guys accept volunteers to become interns/ex officio members, I would really enjoy joining
and can devote a fair amount of time on any tasks small or otherwise.

That said, I of course don't want to be in the way of progress. I think this PR is ready for a good review
and you can tell me what you'd like me to do next. If useful I have added you to my repository as you suggested.

cc: @Tyriar
cc: @mjbvz
Also added you guys to my repository if you want.

I made a fair amount of changes, actually mostly removing code, to the integrated terminal and webView
clients to create a more isolated widget. I have done a decent amount of testing of each of the three clients on Windows 7. I don't expect any platform specific issues, however, I certainly want to make sure
you guys like and accept the changes. I think the extrication of code will continue to be relevant for a global find widget once re- factored .

Tyriar · 2017-08-21T20:18:32Z

src/vs/workbench/parts/terminal/common/terminalService.ts

@@ -122,6 +124,11 @@ export abstract class TerminalService implements ITerminalService {
 		}
 	}

+	// ContextKey to match terminal with simpleFindWidget
+	public getTerminalFocusContextKey(): IContextKey<boolean> {


Remove since we already have the terminalFocusContextKey getter at line 41?

Tyriar · 2017-08-21T20:20:51Z

src/vs/workbench/parts/html/browser/webviewFindWidget.ts

-		super(_contextViewService);
+		super(_contextViewService, _contextKeyService, _simpleFindWidgetService);
+
+		// this._findInputFocused = _findInputContextKey.bindTo(this._contextKeyService);


Tyriar · 2017-08-21T20:20:56Z

src/vs/workbench/parts/html/browser/webviewFindWidget.ts

+		@IContextKeyService _contextKeyService: IContextKeyService,
+		@ISimpleFindWidgetService _simpleFindWidgetService: ISimpleFindWidgetService,
+		private webview: Webview,
+		// private _findInputContextKey: RawContextKey<boolean>


Tyriar · 2017-08-21T20:21:41Z

src/vs/workbench/parts/html/browser/webviewEditor.ts

@@ -33,7 +27,7 @@ export const KEYBINDING_CONTEXT_WEBVIEWEDITOR_NOT_FOCUSED: ContextKeyExpr = KEYB
 export abstract class WebviewEditor extends BaseWebviewEditor {

 	protected _webviewFocusContextKey: IContextKey<boolean>;
-	protected _webview: WebView;
+	protected _webview: Webview;


Change this and the import back to it's regular casing?

Tyriar · 2017-08-21T20:21:57Z

src/vs/workbench/parts/html/browser/webview.ts

@@ -442,12 +449,30 @@ export default class Webview {
 		this._findStarted = false;
 		this._webview.stopFindInPage(keepSelection ? StopFindInPageActions.keepSelection : StopFindInPageActions.clearSelection);
 	}
+	/*


Remove comments?

How did I miss that???
No more late-night commits ;-)

Tyriar · 2017-08-21T20:22:20Z

src/vs/workbench/parts/extensions/browser/extensionEditor.ts

-import { Command } from 'vs/editor/common/editorCommonExtensions';
-import { IWorkbenchEditorService } from 'vs/workbench/services/editor/common/editorService';
-import { KeybindingsRegistry } from 'vs/platform/keybinding/common/keybindingsRegistry';
+import { default as Webview } from 'vs/workbench/parts/html/browser/webview';


Change the casing back to normal

Tyriar · 2017-08-21T20:22:25Z

src/vs/workbench/parts/extensions/browser/extensionEditor.ts


 /**  A context key that is set when an extension editor webview has focus. */
 export const KEYBINDING_CONTEXT_EXTENSIONEDITOR_WEBVIEW_FOCUS = new RawContextKey<boolean>('extensionEditorWebviewFocus', undefined);
 /**  A context key that is set when an extension editor webview not have focus. */
 export const KEYBINDING_CONTEXT_EXTENSIONEDITOR_WEBVIEW_NOT_FOCUSED: ContextKeyExpr = KEYBINDING_CONTEXT_EXTENSIONEDITOR_WEBVIEW_FOCUS.toNegated();

+


Remove extra space

Tyriar · 2017-08-21T20:22:45Z

src/vs/workbench/electron-browser/workbench.ts

@@ -632,6 +633,9 @@ export class Workbench implements IPartService {
 		this.toShutdown.push(this.quickOpen);
 		serviceCollection.set(IQuickOpenService, this.quickOpen);

+		// cleidigh


Remove comment

Tyriar · 2017-08-21T20:24:37Z

src/vs/editor/contrib/find/browser/simpleFindWidget.ts


 	constructor(
 		@IContextViewService private _contextViewService: IContextViewService,
+		@IContextKeyService private _contextKeyService: IContextKeyService,
+		@ISimpleFindWidgetService private _simpleFindWidgetService: ISimpleFindWidgetService,
 		private animate: boolean = true


@Tyriar
I found many pre-existing inconsistencies for private variables , eg some with underscore some without.
When working with any files should I fix these types of things? Change any private variables
to use underscore?

I know there are inconsistencies, try to stick to the style of the file, all the properties and constructor args above use the _ prefix

Okay, will follow that convention
Done

Tyriar · 2017-08-21T20:24:56Z

src/vs/editor/contrib/find/browser/simpleFindWidget.ts

+	}
+
+	// Allow subclass to provide find
+	public baseFind(previous: boolean) {


What is this.find? Can subclasses just reimplement that instead?

Mistaken thought that base class function was required.
Removed, tested.

cleidigh · 2017-08-22T17:44:27Z

@Tyriar
@rebornix
As "Global" commands I use the context key for the simple widget client. This consolidation was a recommendation from @mjbvz. You are correct in that the show/focus simpleFind are context/focus
dependent. If we want individual commands they can show / focus the simpleFind from anywhere,
I can add back the focus commands for each client (integrated terminal, Webview et cetera) keeping
history, find next/previous Global.
I thought command conservation was the initial goal.

I can change to whatever you guys want.

Tyriar · 2017-08-22T18:17:44Z

@mjbvz @rebornix what do you guys think about the generic simpleFind commands not working in the command palette? I'm sure a bug will be created out of this during endgame testing (just as it was for copy selection in terminal).

mjbvz · 2017-08-23T00:42:26Z

@Tyriar Does this effect commands like find next for the regular editor find widget? Do the keybindings work in these cases or not?

cleidigh · 2017-08-23T03:16:09Z

@mjbvz
The main Find commands are supposed to be separate. However right now there is no context limitation
for It . This is actually something I would have to fix if we keep the simpleFind show commands
Global.
The history commands both use a context associated with the input box.
The Find next and previous use the focused part context to get to the correct client.

If it is desired show/focus either the Find or simpleFind from anywhere including the command palette
then we will have to have specific commands for each possible client of the simpleFind.

cleidigh · 2017-08-23T18:54:25Z

@rebornix
@Tyriar
@mjbvz
I thought I would summarize what exists today (shipping insiders) as well as the intermediary PR commits
I have done and finally some options. As you will see below there are some inconsistencies that led me to have to make choices including changing as per ongoing suggestions. I hope I'm been helpful here, as opposed to making this more complicated ;-)

Existing Find/simpleFind commands and behavior:

Show/Focus Find:

Displays main Find
No required context (activates no matter where current focus is)
Standard KeyBindings Control+F ...
Appears in command pallet
Hide Find:
Hides main Find
Editor must be focused or Find
Escape
NOT available in commands pallet
Find History Next/Previous:
Main Find input box focused
Commands appear in pallet but are not enabled unless editor Find input box focused
Find Next/Previous:
Available in editor context
Commands appear on pallet but are not enabled unless editor focused

Terminal simpleFind:

Show/Focus:

Displays simpleFind in terminal
Standard KeyBinding within the context Control+F
Available in command pallet from anywhere regardless of current focus
Hide:
Escape
Available in command pallet from anywhere regardless of current focus
History Next/Previous:
Not available (part of original issue/PR)
simpleFind Next/Previous:
Not available (part of original issue/PR)

ExtensionEditor and WebView (readme,MarkDown):

Show/Focus:

Displays simpleFind in extensionEditor/WebView
Standard KeyBinding within the context Control+F
NOT Available in command pallet
Hide:
Escape within context
NOT Available in command pallet
History Next/Previous:
Not available (part of original issue/PR)
simpleFind Next/Previous:
Not available (part of original issue/PR)

Intermediary commit abaae0cc6ea88060d3d8912ef7befdfafc829795 :

Added History Next/Previous commands for terminal, extensionEditor, WebView (2*3 new commands using existing client structures)
History commands require simpleFind input box focused
Added simpleFind next/previous commands for terminal, extensionEditor, WebView (2*3 new commands)
Terminal commands for find next/previous are actionable from anywhere including command pallet
extensionEditor, WebView require context focus and not available from command palette

Latest PR Commit:

Single service based set of commands for simpleFind
ShowWidgetSimpleFindCommand: 'simpleFind.show', - requires client focus (error must change Find to use editor context to work consistently)
HideWidgetSimpleFindCommand: 'simpleFind.hide', - requires client focus
HistoryNextSimpleFindCommand: 'simpleFind.nextHistory', - requires client simpleFind input box focus

HistoryPreviousSimpleFindCommand: 'simpleFind.previousHistory', - same as above
FindNextSimpleFindCommand: 'simpleFind.nextMatch', - requires client focus
FindPreviousSimpleFindCommand: 'simpleFind.previousMatch' - same as above

These commands do not show up in the command pallet.

Questions:

Should Show/Focus have commands for each client (actionable from anywhere including pallet)
Should terminal be different than the other clients (currently is)
Should Find next/previous be available from any focus (client specific commands)
Should Hide have a command available anywhere
What potentially changes when there is a single, unified findWidget

…nds Cleanup1

rebornix · 2017-08-26T02:24:33Z

@cleidigh thanks for your great work. Sat with @mjbvz this afternoon and went through the PR, we think it's Okay that the commands are not avail in Command Palette. As we might have more commands avail in the Find Widget and maybe more places using Find (or just Webview), adding all commands to Command Palette may make it query a bit confusing. Merge it and I'll tweak the code when necessary.

cleidigh · 2017-08-26T03:09:20Z

@rebornix
Excellent !
We have to add the editor context for the MainFind to work in the editor.

rebornix · 2017-08-26T03:39:39Z

We have to add the editor context for the MainFind to work in the editor.
Delete branch

@cleidigh can you elaborate more on this? We played with the bits and the Find Widget in Editor works as usual, we didn't want to change the behavior of it, right?

cleidigh · 2017-08-26T03:58:12Z

@rebornix
After I did my final commit I was doing some further playing around and I found that the main
editor Find was not activated with Control+F if a terminal or WebView had been instantiated.
I mentioned this in my summary but perhaps it was not clear. I think if we had a context requirement
it would fix it. Also I think you have command weights that also might to the trick. You are correct, I did not intend to change any of the regular behavior.

I will redo testing tomorrow to see if I can reproduce. I apologize if I didn't make this clear before , I was expecting to discuss it along with the other issues. I will post my results as soon as possible tomorrow. I am also hoping I can help out more on further re- factoring !

cleidigh · 2017-08-27T23:22:40Z

@rebornix
Neither you nor I saw the issue since it requires a particular sequence. After some more debugging, I can reproduce it all the time and I also understand why one condition was intermittently failing. The main find command is not always orthogonal with the simple find command.

Steps to reproduce (simplest method)

Open Code with no files and no terminal
Open any file text, code et cetera
Use Control+F to activate main Find (succeeds normally)
Hide Find with Escape
Create new/only integrated terminal (terminal gets focus)
Use Control+F to activate simple Find (succeeds normally)
Hide simple Find with Escape
Give editor file focus
Use Control+F to activate main Find:
Fails since there is still one active simple Find and show simpleFind command seems to be a higher
priority than main Find command.
Opening an extension editor page will do the same thing
The WebView pages dispose of their simpleFind widgets when the lose focus which is one reason this behavior was hard to detect.

One "fix": Add context requirement to ShowSimpleFind (editor not focused)

const showWidgetSimpleFindCommand = new ShowWidgetSimpleFindCommand({
id: SIMPLE_FIND_IDS.ShowWidgetSimpleFindCommand,
precondition: KEYBINDING_CONTEXT_SIMPLE_FIND_WIDGET_ACTIVE,
kbOpts: {
primary: KeyMod.CtrlCmd | KeyCode.KEY_F,
kbExpr: EditorContextKeys.focus.toNegated()
}
});

This does one good thing and one bad thing:

If any editor is focused the main Find works
If any sidebar viewlet is focused the main Find DOES NOT WORK - That is different than pre-simpleFind command additions

If we had a ContextOr operation we could 'Or' all simpleFind contexts for the show command.
Also tried to change command weight to make main Find higher priority - could not get the work, but I don't really know how this operates.
Not sure what is best, perhaps opening main Find is not important from sidebar?

I am frustrated I did not detect this before...

cleidigh · 2017-08-28T02:58:19Z

@rebornix
I found a second solution that allows the main Find to be executed from anywhere as before. If the context check in the simpleFind service proves false (no current client focused) then execute the main Find.

public show(): void {
	const activeSimpleFindWidget = this.getActiveSimpleFindWidget();
	if (!!activeSimpleFindWidget) {
		activeSimpleFindWidget.reveal(true);
	}
	else {
		this._commandService.executeCommand('actions.find');
	}
}

I also added an editor focused check for simpleFind next/previous. Without a key binding comparison,
this does assume the same keys for both Show Find. I think we are running into one of several issues of having almost identical functionality and key bindings for two different widgets.

let me know if either of these look good and I will do a new PR (I don't think I can use this one?)

Revert "Use simpleFind.xyz for simpleFind command IDS" This reverts commit 9066eed. Revert "Review cleanup" This reverts commit c0a2180. Revert "SimpleFindWidget Re-Factor next,previous,history, single global commands Cleanup1" This reverts commit 1a26638. Revert "Working SimpleFind Service, isolation - pre- cleanup" This reverts commit bd8f5eb. Revert "Reveal Find on find n/p, add delayed update history" This reverts commit c586e52. Revert "Hide input focus tracker, cleanup" This reverts commit 27f4523. Revert "simpleFind Add next,previous,history fix" This reverts commit b69d45b.

alexdima · 2017-08-28T13:34:25Z

@rebornix @Tyriar @mjbvz I've had to revert all these commits one-by-one. Please test before merging anything and please merge with one merge commit which would make it easy for someone to revert a bad merge.

The issue is #33240

cleidigh · 2017-08-28T17:09:06Z

@alexandrudima
@rebornix
@mjbvz
@Tyriar
My fault here. Apologies for not catching it pre-merge and for you having to back out the single commits.

Will determine best fix with group.

cleidigh · 2017-08-28T17:19:09Z

The third possible fix is to reintroduce Terminal: Focus simpleFind and ExtensionEditor, WebView as well.
Each possible solution has pros and cons. I also do not think this addresses the layering issue which I think
if I understand correctly was already introduced with the original simpleFind.

Let me know how I can help fix this.

cleidigh · 2017-08-28T21:05:42Z

@rebornix
Summary of three possible fixes (that I have tried, surely not all possible fixes)

Add !editorFocus context requirement to show,hide,next,previous - Downside - Cannot activate Find from sidebar
Execute Find from simpleFind if no client context active - Downside - probably to entangled, odd if key bindings not the same
Re-Add Show and other commands for each client - Downside - More commands, probably will be deprecated later (essentially my earlier PR)

Other options:

Find a way to distinguish mainFind show from simpleFind using some other Context trick
Dive into real re- factor of Find - Forces decisions on editor and workbench layering, and how commands would work to avoid the problems like I just created :-(

msftclas added the cla-already-signed label Aug 8, 2017

cleidigh mentioned this pull request Aug 8, 2017

SimpleFindWidget Add next,previous,history: Fixes #29708 #29661 #32074

Closed

weinand assigned rebornix Aug 8, 2017

rebornix reviewed Aug 8, 2017

View reviewed changes

product.json Outdated

"urlProtocol": "code-oss",

Copy link

Member

rebornix Aug 8, 2017

Choose a reason for hiding this comment

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

you don't want to check in this, right :)

rebornix reviewed Aug 8, 2017

View reviewed changes

rebornix requested a review from mjbvz August 8, 2017 23:24

rebornix assigned mjbvz Aug 8, 2017

mjbvz reviewed Aug 9, 2017

View reviewed changes

cleidigh mentioned this pull request Aug 10, 2017

Refactor FindWidget: shareable find component with history/commands #32241

Closed

3 tasks

cleidigh commented Aug 14, 2017

View reviewed changes

Tyriar requested changes Aug 21, 2017

View reviewed changes

rebornix force-pushed the simpleFind-next-prev-history2/add branch from d820ee1 to 25c79be Compare August 25, 2017 21:57

cleidigh added 7 commits August 25, 2017 14:59

simpleFind Add next,previous,history fix

618e497

Hide input focus tracker, cleanup

8735a6e

Reveal Find on find n/p, add delayed update history

e950aad

Working SimpleFind Service, isolation - pre- cleanup

e473134

SimpleFindWidget Re-Factor next,previous,history, single global comma…

58f5d20

…nds Cleanup1

Review cleanup

89666bc

Use simpleFind.xyz for simpleFind command IDS

bda04b8

rebornix force-pushed the simpleFind-next-prev-history2/add branch from 25c79be to bda04b8 Compare August 25, 2017 23:07

rebornix merged commit 9066eed into microsoft:master Aug 26, 2017

This was referenced Aug 28, 2017

Duplication/Layering in simpleFindWidget #33264

Closed

CTRL+F stopped working #33240

Closed

Tyriar mentioned this pull request Dec 27, 2017

No next and previous keyboard shortcuts for Find widget in Integrated Terminal #29661

Closed

github-actions bot locked and limited conversation to collaborators Mar 27, 2020

SimpleFindWidget Add next,previous,history: Fixes #29708 #29661 #32113

SimpleFindWidget Add next,previous,history: Fixes #29708 #29661 #32113

Conversation

cleidigh commented Aug 8, 2017 • edited Loading

Choose a reason for hiding this comment

cleidigh commented Aug 8, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rebornix commented Aug 8, 2017 • edited Loading

cleidigh commented Aug 8, 2017 • edited Loading

cleidigh commented Aug 8, 2017 • edited Loading

rebornix commented Aug 8, 2017

rebornix commented Aug 8, 2017

mjbvz left a comment

Choose a reason for hiding this comment

cleidigh commented Aug 9, 2017 • edited Loading

cleidigh commented Aug 9, 2017 • edited Loading

rebornix commented Aug 9, 2017

cleidigh commented Aug 9, 2017

rebornix commented Aug 9, 2017

cleidigh commented Aug 11, 2017

rebornix commented Aug 11, 2017

cleidigh commented Aug 11, 2017

rebornix commented Aug 11, 2017

cleidigh left a comment

Choose a reason for hiding this comment

cleidigh commented Aug 14, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cleidigh commented Aug 22, 2017 • edited Loading

Tyriar commented Aug 22, 2017

mjbvz commented Aug 23, 2017

cleidigh commented Aug 23, 2017 • edited Loading

cleidigh commented Aug 23, 2017 • edited Loading

rebornix commented Aug 26, 2017

cleidigh commented Aug 26, 2017

rebornix commented Aug 26, 2017

cleidigh commented Aug 26, 2017 • edited Loading

cleidigh commented Aug 27, 2017 • edited Loading

cleidigh commented Aug 28, 2017 • edited Loading

alexdima commented Aug 28, 2017

cleidigh commented Aug 28, 2017

cleidigh commented Aug 28, 2017

cleidigh commented Aug 28, 2017 • edited Loading

cleidigh commented Aug 8, 2017 •

edited

Loading

rebornix commented Aug 8, 2017 •

edited

Loading

cleidigh commented Aug 8, 2017 •

edited

Loading

cleidigh commented Aug 8, 2017 •

edited

Loading

cleidigh commented Aug 9, 2017 •

edited

Loading

cleidigh commented Aug 9, 2017 •

edited

Loading

cleidigh commented Aug 14, 2017 •

edited

Loading

cleidigh commented Aug 22, 2017 •

edited

Loading

cleidigh commented Aug 23, 2017 •

edited

Loading

cleidigh commented Aug 23, 2017 •

edited

Loading

cleidigh commented Aug 26, 2017 •

edited

Loading

cleidigh commented Aug 27, 2017 •

edited

Loading

cleidigh commented Aug 28, 2017 •

edited

Loading

cleidigh commented Aug 28, 2017 •

edited

Loading