Allow to filter keyBindings by 'Source'. by shobhitchittora · Pull Request #43393 · microsoft/vscode

shobhitchittora · 2018-02-10T17:07:47Z

This adds a source filter to the Key Bindings Editor View.
Usage: source: user | source: default

Tasks -

source filter working
Tests passing
Update the test for the file changes

sandy081 · 2018-02-14T10:46:26Z

@shobhitchittora Changes look good. To discover this feature, How about having a context menu with Show Default Keybindings and Show User Keybindings actions?

shobhitchittora · 2018-02-16T05:02:26Z

@sandy081 Why is the test failing in CI for windows ( /test.bat ) ? I cannot verify the same as my local is on mac.

About this -

To discover this feature, How about having a context menu with Show Default Keybindings and Show User Keybindings actions?

Observation - Currently in Keyboard Shortcuts Tab, when you click on keybindings.json, it open up two tabs - one is Default Keybindings and the other is keybindigs.json ( or user bindings ).

Clarification - on firing action from the context menu ( Show Default Keybindings or Show User Keybindings ) the user should be taken to the Keyboard Shortcuts Tab with the source: selected by default.

shobhitchittora · 2018-02-19T12:15:19Z

@sandy081 Any comments on this ? 😇

sandy081 · 2018-02-20T13:35:26Z

@shobhitchittora I think we can provide these as secondary actions to the keybindings editor which will be shown as follows:

This can be done by implementing getSecondaryActions method in Keybindings editor.

Let me know if you need any help

shobhitchittora · 2018-02-20T19:33:15Z

@sandy081 Thanks! This looks good. So on clicking on any of these actions, the corresponding filter is applied ?

shobhitchittora · 2018-02-20T20:01:29Z

Also more questions -

How to create dropdown menus ?
How to add items / actions to the dropdown ?
Where to create the anchor for the dropdown ?

sandy081 · 2018-02-21T17:15:38Z

@shobhitchittora You just have to implement getSecondaryActions method in Keybindings Editor. An example here

Usage -> 'source: user' or 'source: default' Closes: microsoft#43037

1. Adds action constants in preferences.ts 2. Adds showUser and showDefault keybindings actions 3. Adds getSecondaryActions to register context menu actions

shobhitchittora · 2018-03-02T19:01:56Z

@sandy081 I've added secondary actions and pushed tests too. Please have a look.

PS: Apologies for the delay. Was busy with work. 😇

shobhitchittora · 2018-03-06T06:41:21Z

Ping @sandy081

sandy081 · 2018-03-06T15:19:46Z

@shobhitchittora Provided some feedback otherwise it looks good. I guess, we can push this PR once requested changes are handled.

Thanks

shobhitchittora · 2018-03-06T18:20:19Z

@sandy081 I don't see any comments here.

sandy081 · 2018-03-06T15:11:57Z

src/vs/workbench/parts/preferences/browser/keybindingsEditor.ts

 	private keybindingFocusContextKey: IContextKey<boolean>;
 	private searchFocusContextKey: IContextKey<boolean>;
 	private sortByPrecedence: Checkbox;
+	private secondaryActions: IAction[];


This variable is not necessary

sandy081 · 2018-03-06T15:12:09Z

src/vs/workbench/parts/preferences/browser/keybindingsEditor.ts

 		return focusedElement && focusedElement.templateId === KEYBINDING_ENTRY_TEMPLATE_ID ? <IKeybindingItemEntry>focusedElement : null;
 	}

+	setKeybindingSource(searchString: string): TPromise<any> {


Inline this in the action

sandy081 · 2018-03-06T15:12:41Z

src/vs/workbench/parts/preferences/browser/keybindingsEditor.ts

+		return TPromise.as(null);
+	}
+
+	showDefaultKeyBindings(): IAction {


Inline the action

sandy081 · 2018-03-06T15:12:51Z

src/vs/workbench/parts/preferences/browser/keybindingsEditor.ts

+		};
+	}
+
+	showUserKeyBindings(): IAction {


Inline the action

Done for both actions.

sandy081 · 2018-03-06T15:13:48Z

src/vs/workbench/parts/preferences/browser/keybindingsEditor.ts

+	}
+
+	getSecondaryActions(): IAction[] {
+		if (!this.secondaryActions) {


storing in secondaryActions is not necessary. Its ok to create the array and return

Cool. done.

sandy081 · 2018-03-06T15:15:06Z

src/vs/workbench/parts/preferences/common/keybindingsEditorModel.ts

 		this.keybindingMatches = keybindingItem.keybinding ? this.matchesKeybinding(keybindingItem.keybinding, searchValue, keybindingWords) : null;
 	}

+	private checkForSourceFilter(searchValue: string) {


What happens if the label of a command matches source: default? I would parse the search string here and return results based on the parsed value. If it is a source type filter then return corresponding entries otherwise do the existing filtering.

sandy081 · 2018-03-06T15:18:48Z

src/vs/workbench/parts/preferences/browser/keybindingsEditor.ts

+			label: localize('showDefaultKeybindings', "Show Default Keybindings"),
+			enabled: true,
+			id: KEYBINDINGS_EDITOR_SHOW_DEFAULT_KEYBINDINGS,
+			run: () => this.setKeybindingSource('source: default')


I would use following format:

@source: default

This is consistent with filtering in Extensions viewlet

Changed to use @source:

sandy081 · 2018-03-07T17:28:03Z

@shobhitchittora Sorry forgot to submit

…itchittora/vscode into keyBindingsEditor-filter-by-source

…method

shobhitchittora · 2018-03-08T06:15:16Z

src/vs/workbench/parts/preferences/common/keybindingsEditorModel.ts

-					sourceMatches: keybindingMatches.sourceMatches,
-					whenMatches: keybindingMatches.whenMatches
-				});
+			if (sourceFilterApplied) {


Is there a better way to push results based on whether @source filter is applied or not?

Can we move refactor it into on method which checks for respective matches and another one which creates the final result array?

@shobhitchittora You can have two methods one for filtering based on source and the other does full text search and call either of them by checking the search text. It can look something like follows

if (/@source:/i.test(searchValue)) { return this.filterBySource(searchValue); } return this.filterByText(searchValue);

@source

1. filter By @source: 2. normal text filter 3. @source filter with quotes - @source: "user"

sandy081 · 2018-03-13T10:15:08Z

src/vs/workbench/parts/preferences/common/keybindingsEditorModel.ts

+			return keybindingItems.map(keybindingItem => ({ id: KeybindingsEditorModel.getId(keybindingItem), keybindingItem, templateId: KEYBINDING_ENTRY_TEMPLATE_ID }));
+		}
+
+		const result: IKeybindingItemEntry[] = [];


Not sure if you need this code any more because this is handled by fetchKeybindingItemsByText

fetchKeybindingItemsByText is called conditionally when no @source: filter is applied.
Otherwise fetchKeybindingItemsBySource will be called.

sandy081 · 2018-03-13T10:15:48Z

src/vs/workbench/parts/preferences/common/keybindingsEditorModel.ts

+		if (this.isSourceFilterApplied(searchValue)) {
+			searchValue = this.getSourceFilterValue(searchValue);
+			searchValue = this.parseSearchValue(searchValue);
+			return this.fetchKeybindingItemsBySource(sortByPrecedence ? this._keybindingItemsSortedByPrecedence : this._keybindingItems, searchValue, this.quoteAtFirst(searchValue) && this.quoteAtLast(searchValue));


Not sure why you need compute quotes here?

For the same reasons as above.

sandy081 · 2018-03-13T10:16:11Z

src/vs/workbench/parts/preferences/common/keybindingsEditorModel.ts

+
+		if (this.isSourceFilterApplied(searchValue)) {
+			searchValue = this.getSourceFilterValue(searchValue);
+			searchValue = this.parseSearchValue(searchValue);


Why do we need to parseSearchValue?

This is added to handle the case with quotes and source filter is applied. --> @source: "user" / @source: "default"

@shobhitchittora Lets make it simple and support only @source:user or @source:default. I do not think why we need to support quotes. This will make everything much simpler

Yeah I probably over-complicated it, because I saw quotes were also supported. Will simplify it. 💪🏼

sandy081 · 2018-03-13T12:20:57Z

src/vs/workbench/parts/preferences/common/keybindingsEditorModel.ts

+	}
+
+	private filterBySource(keybindingItems: IKeybindingItem[], searchValue: string, completeMatch: boolean): IKeybindingItemEntry[] {
+		const result: IKeybindingItemEntry[] = [];


@shobhitchittora I think we are very good with the changes except for changing filterBySource method a bit. Instead of delegating the filtering to KeybindingItemMatches lets filter keybindingItems by source directly and map them filtered results to IKeybindingItemEntry.

Sorry for providing more comments :(

@sandy081 Don't be sorry. I encourage comments / suggestions till it's the best. 😇

sandy081 · 2018-03-14T04:33:51Z

@shobhitchittora Thanks for the PR. Merged.

octref requested a review from sandy081 February 12, 2018 20:50

octref assigned sandy081 Feb 13, 2018

sandy081 added the keybindings-editor Keybinding editor issues label Feb 14, 2018

sandy081 modified the milestones: January 2018 Recovery , February 2018 Feb 14, 2018

shobhitchittora added 2 commits March 2, 2018 22:06

Allow to filter keyBindings by 'Source'.

7180828

Usage -> 'source: user' or 'source: default' Closes: microsoft#43037

Extends test for source: filter

b31e4ab

shobhitchittora force-pushed the keyBindingsEditor-filter-by-source branch from 7bfcf57 to b31e4ab Compare March 2, 2018 16:36

shobhitchittora added 4 commits March 2, 2018 23:39

Adds context menu actions for source filters -

9c06e7b

1. Adds action constants in preferences.ts 2. Adds showUser and showDefault keybindings actions 3. Adds getSecondaryActions to register context menu actions

removing un-related changes

8c93f83

adding guards for split search query

09f2c6b

fixes tests

181a0b6

sandy081 reviewed Mar 7, 2018

View reviewed changes

shobhitchittora added 3 commits March 8, 2018 11:01

fixes tests

8510c24

Merge branch 'keyBindingsEditor-filter-by-source' of github.com:shobh…

666b292

…itchittora/vscode into keyBindingsEditor-filter-by-source

removed un-related changes + moved @source check to fetchKeyBindings …

4326d41

…method

shobhitchittora commented Mar 8, 2018

View reviewed changes

test cases fix

303fc92

bpasero modified the milestones: February 2018, March 2018 Mar 8, 2018

Refactor methods out to handle -

bd7a5c7

1. filter By @source: 2. normal text filter 3. @source filter with quotes - @source: "user"

sandy081 requested changes Mar 13, 2018

View reviewed changes

refactored according to comments

ae5ec7e

sandy081 requested changes Mar 13, 2018

View reviewed changes

Adds source filtering logic directly from keybindingItems

84e2708

sandy081 approved these changes Mar 14, 2018

View reviewed changes

sandy081 merged commit 4a5e48d into microsoft:master Mar 14, 2018

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

Conversation

shobhitchittora commented Feb 10, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sandy081 commented Feb 14, 2018

Uh oh!

shobhitchittora commented Feb 16, 2018

Uh oh!

shobhitchittora commented Feb 19, 2018

Uh oh!

sandy081 commented Feb 20, 2018

Uh oh!

shobhitchittora commented Feb 20, 2018

Uh oh!

shobhitchittora commented Feb 20, 2018

Uh oh!

sandy081 commented Feb 21, 2018

Uh oh!

shobhitchittora commented Mar 2, 2018

Uh oh!

shobhitchittora commented Mar 6, 2018

Uh oh!

sandy081 commented Mar 6, 2018

Uh oh!

shobhitchittora commented Mar 6, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sandy081 commented Mar 7, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sandy081 Mar 13, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shobhitchittora commented Feb 10, 2018 •

edited

Loading

sandy081 Mar 13, 2018 •

edited

Loading