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

Allow to filter keyBindings by 'Source'. #43393

Merged
merged 13 commits into from Mar 14, 2018

Conversation

Projects
None yet
3 participants
@shobhitchittora
Copy link
Contributor

shobhitchittora commented Feb 10, 2018

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

Closes: #43037

@octref octref requested a review from sandy081 Feb 12, 2018

@sandy081

This comment has been minimized.

Copy link
Member

sandy081 commented Feb 14, 2018

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

@shobhitchittora

This comment has been minimized.

Copy link
Contributor

shobhitchittora commented Feb 16, 2018

@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

This comment has been minimized.

Copy link
Contributor

shobhitchittora commented Feb 19, 2018

@sandy081 Any comments on this ? 😇

@sandy081

This comment has been minimized.

Copy link
Member

sandy081 commented Feb 20, 2018

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

image

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

Let me know if you need any help

@shobhitchittora

This comment has been minimized.

Copy link
Contributor

shobhitchittora commented Feb 20, 2018

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

@shobhitchittora

This comment has been minimized.

Copy link
Contributor

shobhitchittora commented Feb 20, 2018

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

This comment has been minimized.

Copy link
Member

sandy081 commented Feb 21, 2018

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

shobhitchittora added some commits Feb 10, 2018

Allow to filter keyBindings by 'Source'.
Usage ->  'source: user'  or 'source: default'

Closes: #43037

@shobhitchittora shobhitchittora force-pushed the shobhitchittora:keyBindingsEditor-filter-by-source branch from 7bfcf57 to b31e4ab Mar 2, 2018

shobhitchittora added some commits Mar 2, 2018

Adds context menu actions for source filters -
1. Adds action constants in preferences.ts
2. Adds showUser and showDefault keybindings actions
3. Adds getSecondaryActions to register context menu actions
@shobhitchittora

This comment has been minimized.

Copy link
Contributor

shobhitchittora commented Mar 2, 2018

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

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

@shobhitchittora

This comment has been minimized.

Copy link
Contributor

shobhitchittora commented Mar 6, 2018

Ping @sandy081

@sandy081

This comment has been minimized.

Copy link
Member

sandy081 commented Mar 6, 2018

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

Thanks

@shobhitchittora

This comment has been minimized.

Copy link
Contributor

shobhitchittora commented Mar 6, 2018

@sandy081 I don't see any comments here.

@@ -97,6 +98,7 @@ export class KeybindingsEditor extends BaseEditor implements IKeybindingsEditor
private keybindingFocusContextKey: IContextKey<boolean>;
private searchFocusContextKey: IContextKey<boolean>;
private sortByPrecedence: Checkbox;
private secondaryActions: IAction[];

This comment has been minimized.

@sandy081

sandy081 Mar 7, 2018

Member

This variable is not necessary

This comment has been minimized.

@shobhitchittora

shobhitchittora Mar 8, 2018

Contributor

Removed.

@@ -176,6 +178,39 @@ export class KeybindingsEditor extends BaseEditor implements IKeybindingsEditor
return focusedElement && focusedElement.templateId === KEYBINDING_ENTRY_TEMPLATE_ID ? <IKeybindingItemEntry>focusedElement : null;
}

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

This comment has been minimized.

@sandy081

sandy081 Mar 7, 2018

Member

Inline this in the action

This comment has been minimized.

@shobhitchittora
return TPromise.as(null);
}

showDefaultKeyBindings(): IAction {

This comment has been minimized.

@sandy081

sandy081 Mar 7, 2018

Member

Inline the action

This comment has been minimized.

@shobhitchittora
};
}

showUserKeyBindings(): IAction {

This comment has been minimized.

@sandy081

sandy081 Mar 7, 2018

Member

Inline the action

This comment has been minimized.

@shobhitchittora

shobhitchittora Mar 8, 2018

Contributor

Done for both actions.

}

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

This comment has been minimized.

@sandy081

sandy081 Mar 7, 2018

Member

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

This comment has been minimized.

@shobhitchittora

shobhitchittora Mar 8, 2018

Contributor

Cool. done.

this.whenMatches = keybindingItem.when ? this.matches(searchValue, keybindingItem.when, or(matchesWords, matchesCamelCase), words) : null;
this.keybindingMatches = keybindingItem.keybinding ? this.matchesKeybinding(keybindingItem.keybinding, searchValue, keybindingWords) : null;
}

private checkForSourceFilter(searchValue: string) {

This comment has been minimized.

@sandy081

sandy081 Mar 7, 2018

Member

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.

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

This comment has been minimized.

@sandy081

sandy081 Mar 7, 2018

Member

I would use following format:

@source: default

This is consistent with filtering in Extensions viewlet

This comment has been minimized.

@shobhitchittora

shobhitchittora Mar 8, 2018

Contributor

Changed to use @source:

@sandy081

This comment has been minimized.

Copy link
Member

sandy081 commented Mar 7, 2018

@shobhitchittora Sorry forgot to submit

shobhitchittora added some commits Mar 2, 2018

Merge branch 'keyBindingsEditor-filter-by-source' of github.com:shobh…
…itchittora/vscode into keyBindingsEditor-filter-by-source
sourceMatches: keybindingMatches.sourceMatches,
whenMatches: keybindingMatches.whenMatches
});
if (sourceFilterApplied) {

This comment has been minimized.

@shobhitchittora

shobhitchittora Mar 8, 2018

Contributor
  1. Is there a better way to push results based on whether @source filter is applied or not?
  2. Can we move refactor it into on method which checks for respective matches and another one which creates the final result array?

This comment has been minimized.

@sandy081

sandy081 Mar 12, 2018

Member

@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);

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

Refactor methods out to handle -
1. filter By @source:
2. normal text filter
3. @source filter with quotes - @source: "user"
return keybindingItems.map(keybindingItem => ({ id: KeybindingsEditorModel.getId(keybindingItem), keybindingItem, templateId: KEYBINDING_ENTRY_TEMPLATE_ID }));
}

const result: IKeybindingItemEntry[] = [];

This comment has been minimized.

@sandy081

sandy081 Mar 13, 2018

Member

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

This comment has been minimized.

@shobhitchittora

shobhitchittora Mar 13, 2018

Contributor

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

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));

This comment has been minimized.

@sandy081

sandy081 Mar 13, 2018

Member

Not sure why you need compute quotes here?

This comment has been minimized.

@shobhitchittora

shobhitchittora Mar 13, 2018

Contributor

For the same reasons as above.


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

This comment has been minimized.

@sandy081

sandy081 Mar 13, 2018

Member

Why do we need to parseSearchValue?

This comment has been minimized.

@shobhitchittora

shobhitchittora Mar 13, 2018

Contributor

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

This comment has been minimized.

@sandy081

sandy081 Mar 13, 2018

Member

@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

This comment has been minimized.

@shobhitchittora

shobhitchittora Mar 13, 2018

Contributor

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

}

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

This comment has been minimized.

@sandy081

sandy081 Mar 13, 2018

Member

@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 :(

This comment has been minimized.

@shobhitchittora

shobhitchittora Mar 14, 2018

Contributor

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

This comment has been minimized.

@shobhitchittora

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

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
license/cla All CLA requirements met.
Details
@sandy081

This comment has been minimized.

Copy link
Member

sandy081 commented Mar 14, 2018

@shobhitchittora Thanks for the PR. Merged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment