Feature Addition for Issue #24396 - New "Keyboard Shortcut" dialog should display current assignment #40405

brandontruggles · 2017-12-18T02:24:59Z

Please let me know if there are any issues.

msftclas · 2017-12-18T02:25:10Z

All CLA requirements met.

brandontruggles · 2017-12-18T02:49:53Z

I noticed that the automated builds failed...this might have to do with the last commit I made to remove types/which from the dependencies, not sure if I should've done that. During development I accidentally installed this package into the normal dependencies when I encountered compiler errors. Please advise.

dadlerj

lgtm

brandontruggles · 2018-04-20T02:28:55Z

Re-synced with master, re-ran the smoke tests, and all the failed tests now pass. Seems to have been an issue with code outside of my own from the commit that I initially cloned from.

sandy081 · 2018-04-20T10:52:37Z

@brandonrninefive Thanks for the PR. Some changes

Let's not call them conflicts as they are not really conflicts. Because keybindings depend on when context value . That's why we call them same keybindings. How about 2 Exists or 1 Exist?
{0} Exists can be rendered as a link, on clicking, it will take back to Keybindings editor to show them
Layout: I think the keybinding should be always centred. Above message can be shown to the right

brandontruggles · 2018-04-21T03:15:32Z

@sandy081 Thank you for the feedback! I'm working on implementing these changes now, and I should have an updated PR within the next few days.

sandy081 · 2018-04-23T14:44:56Z

Please ping me when you have changes ready. Thanks

…ing entries exist for the same keybinding. Clicking the link searches the list of assigned keybindings for all commands matching that specific keybinding.

brandontruggles · 2018-04-24T01:46:28Z

@sandy081 Please let me know if the updated PR is acceptable. I followed your advice and made the label a clickable link which searches the input keybinding upon click. For the text of the label itself, I settled for "Existing", because I think it covers both the singular and plural cases of "Exists" nicely.

I have also re-centered the keybinding label as suggested, but instead of placing the "Existing" label to the right, I chose to place it centered underneath the keybinding label because I personally think it looks nicer this way. Please see the below screenshot for a quick glimpse at how I styled things.

If you see any issues with my new commit, please let me know, and I'll work to fix them.

sandy081 · 2018-05-02T15:03:34Z

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

 		new KeybindingLabel(this._outputNode, OS).set(this._firstPart, null);
 		if (this._chordPart) {
 			this._outputNode.appendChild(document.createTextNode(nls.localize('defineKeybinding.chordsTo', "chord to")));
 			new KeybindingLabel(this._outputNode, OS).set(this._chordPart, null);
 		}
 	}

+	public printConflicts(numConflicts: number, keybinding: [ResolvedKeybinding, ResolvedKeybinding]): void {


keybinding argument is not used, so this can be removed.

@sandy081 Within this method, I am using the keybinding argument for the event listener for when the link I created is clicked. Please see line 267.

Since I generate the link within the method, I believe I need the argument here to pass the proper keybinding to the event such that it can be passed on to showSimilarKeybindings in keybindingsEditor.ts, in order to display the matching keybinding entries.

@brandonrninefive My suggestion is as follows:

Change onDidChange and onLinkClick events to emit key string just like how define method returns key string

While you are triggering onLinkClick you can get the key from the current state this._firstPart and this._chordPart. See define() method for how to get the key

sandy081 · 2018-05-02T15:10:19Z

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


 	private _firstPart: ResolvedKeybinding = null;
 	private _chordPart: ResolvedKeybinding = null;
 	private _isVisible: boolean = false;

 	private _onHide = this._register(new Emitter<void>());

+	private _onDidChange = this._register(new Emitter<[ResolvedKeybinding, ResolvedKeybinding]>());


Events can emit key string just like how define method returns key string

sandy081 · 2018-05-02T15:10:46Z

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

@@ -259,19 +259,49 @@ export class KeybindingsEditor extends BaseEditor implements IKeybindingsEditor
 		this.searchWidget.clear();
 	}

-	showSimilarKeybindings(keybindingEntry: IKeybindingItemEntry): TPromise<any> {
-		const value = `"${keybindingEntry.keybindingItem.keybinding.getAriaLabel()}"`;
+	showSimilarKeybindings(keybindingEntry: IKeybindingItemEntry | [ResolvedKeybinding, ResolvedKeybinding]): TPromise<any> {


I do not think changes to this function are needed, if the events return the key string

sandy081 · 2018-05-02T15:13:24Z

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

 		if (value !== this.searchWidget.getValue()) {
 			this.searchWidget.setValue(value);
 		}
 		return TPromise.as(null);
 	}

+	showOverlayConflicts(keybinding: [ResolvedKeybinding, ResolvedKeybinding]): void {


This method should be private

It takes the key as string and call the fetch on the model by wrapping the key in quotes.
This method will become a 2 liner like

const keybindingsEntries: IKeybindingItemEntry[] = this.keybindingsEditorModel.fetch(`"${keybinding}"`, this.sortByPrecedence.checked); this.defineKeybindingWidget.printConflicts(keybindingsEntries.length);

@sandy081 I attempted to make these changes as recommended, but I came across an issue. It seems that using keybindingsEditorModel.fetch on certain keybindings such as "Y" or "S D" will return any items that include these characters on any of their fields, and not necessarily their keybinding field. As you can see in the below screenshot, this behavior is what currently occurs when clicking the "show similar keybindings" button on a particular keybinding:

If this is the intended behavior of that method, that is fine, but it will not work for this purpose, as the length of the list returned by keybindingsEditorModel.fetch will often be much greater than the number of matching keybindings.

If I may make a recommendation, it would be to change the behavior of that method to concatenate a "+" after any characters in the keybinding, as doing so seems to return the results that we're looking for. As a side effect, this would also make the "show similar keybindings" button produce the same behavior, which makes more sense to me than searching through all the fields of the keybinding items for the characters in the keybinding. Please see the below screenshots for my proposed behavior:

@brandonrninefive Agreed to fix the behaviour of the method to match only keybindings when the filter string is quoted.

…ing entries exist for the same keybinding. Clicking the link searches the list of assigned keybindings for all commands matching that specific keybinding.

sandy081

Please check the comments

sandy081

Please check the comments

… display items based on the same keybinding. Also reworked how conflicting keybindings are searched for.

brandontruggles · 2018-05-18T23:56:37Z

@sandy081 Please see my latest commit. I tried to follow your advice above as closely as possible. Let me know if things look good:

c4a19e0

sandy081 · 2018-05-23T08:29:03Z

@brandonrninefive Thanks for the changes. LGTM.

sandy081 · 2018-05-23T08:43:41Z

@brandonrninefive Did a bit of clean up (cosmetic changes). Wondering why is this change needed in showSimilarKeybindings

let value = `"${keybindingEntry.keybindingItem.keybinding.getAriaLabel().replace(' ', '+')}+"`;

sandy081 · 2018-05-23T09:02:50Z

I have removed the above change while clean up. Please let me know if it is needed.

brandontruggles · 2018-05-23T13:48:55Z

@sandy081 That change in showSimilarKeybindings is the fix we discussed above to make the feature only display actions mapped to the same keybinding. Without concatenating the "+" character in between each letter in the keybinding, the search will display actions not necessarily matching the keybinding, but rather containing those letters on any field. This change is vital to everything working properly.

sandy081 · 2018-05-23T16:12:55Z

Ah I see... I would rather fix it in the keybindings model. If it is for a full match, then only compare keybindings and not compare with values of other properties.

brandontruggles · 2018-05-23T16:20:45Z

@sandy081 Something to keep in mind with that approach is how we want to differentiate the full search from a keybindings-only search in the search box. Right now the "+" symbol acts as a delimiter.

sandy081 · 2018-05-23T16:25:48Z

True. But, filtering always considered a quoted string to match a keybinding completely. Show Same Keybindings action also does the same.

brandontruggles added 9 commits June 28, 2017 23:53

Fix for microsoft#29608 'QuickOpen looks misaligned in monokai theme'.

9614bad

Merge remote-tracking branch 'upstream/master'

45db051

Merge remote-tracking branch 'upstream/master'

04305b5

Merge branch 'master' of https://github.com/Microsoft/vscode

c40c89a

Merge remote-tracking branch 'upstream/master'

dd6867a

Merge remote-tracking branch 'upstream/master'

6ffd68f

Merge remote-tracking branch 'upstream/master'

979ad6a

Conflicts are now displayed when defining a new keybinding.

e1e7be6

Removed dependency accidentally added to package.json and yarn.lock.

3149bda

brandontruggles mentioned this pull request Dec 18, 2017

New "Keyboard Shortcut" dialog should display current assignment #24396

Closed

ramya-rao-a assigned sandy081 Dec 18, 2017

sandy081 added this to the December 2017/January 2018 milestone Dec 18, 2017

dadlerj approved these changes Dec 19, 2017

View reviewed changes

alexdima modified the milestones: January 2018, February 2018 Feb 2, 2018

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

bpasero modified the milestones: March 2018, April 2018 Apr 6, 2018

brandontruggles added 2 commits April 19, 2018 00:38

Merge remote-tracking branch 'upstream/master'

71029de

Merge remote-tracking branch 'upstream/master'

765da3e

Merge remote-tracking branch 'upstream/master'

73d104d

New keybinding prompt now includes a clickable link of how many exist…

793f571

…ing entries exist for the same keybinding. Clicking the link searches the list of assigned keybindings for all commands matching that specific keybinding.

sandy081 reviewed May 2, 2018

View reviewed changes

brandontruggles added 5 commits May 2, 2018 22:16

Fix for microsoft#29608 'QuickOpen looks misaligned in monokai theme'.

3b02dc2

Conflicts are now displayed when defining a new keybinding.

e5dfb9e

Removed dependency accidentally added to package.json and yarn.lock.

2c0eda7

New keybinding prompt now includes a clickable link of how many exist…

1cec9bc

…ing entries exist for the same keybinding. Clicking the link searches the list of assigned keybindings for all commands matching that specific keybinding.

Merge branch 'master' of https://github.com/brandonrninefive/vscode

f124b70

sandy081 mentioned this pull request May 7, 2018

Option to find conflicting (duplicate) keyboard shortcuts #33745

Closed

bpasero modified the milestones: April 2018, May 2018 May 8, 2018

sandy081 reviewed May 15, 2018

View reviewed changes

sandy081 requested changes May 15, 2018

View reviewed changes

Changed the behavior of the 'show similar keybindings' button to only…

c4a19e0

… display items based on the same keybinding. Also reworked how conflicting keybindings are searched for.

Merge remote-tracking branch 'upstream/master'

409041b

sandy081 approved these changes May 23, 2018

View reviewed changes

sandy081 merged commit 06d5047 into microsoft:master May 23, 2018

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature Addition for Issue #24396 - New "Keyboard Shortcut" dialog should display current assignment #40405

Feature Addition for Issue #24396 - New "Keyboard Shortcut" dialog should display current assignment #40405

brandontruggles commented Dec 18, 2017

msftclas commented Dec 18, 2017 •

edited

brandontruggles commented Dec 18, 2017 •

edited

dadlerj left a comment

brandontruggles commented Apr 20, 2018

sandy081 commented Apr 20, 2018

brandontruggles commented Apr 21, 2018

sandy081 commented Apr 23, 2018

brandontruggles commented Apr 24, 2018

sandy081 May 2, 2018

brandontruggles May 12, 2018

sandy081 May 15, 2018

sandy081 May 2, 2018

sandy081 May 2, 2018

sandy081 May 2, 2018

brandontruggles May 12, 2018

sandy081 May 15, 2018

sandy081 left a comment

sandy081 left a comment

brandontruggles commented May 18, 2018 •

edited

sandy081 commented May 23, 2018

sandy081 commented May 23, 2018

sandy081 commented May 23, 2018

brandontruggles commented May 23, 2018

sandy081 commented May 23, 2018

brandontruggles commented May 23, 2018

sandy081 commented May 23, 2018

Feature Addition for Issue #24396 - New "Keyboard Shortcut" dialog should display current assignment #40405

Feature Addition for Issue #24396 - New "Keyboard Shortcut" dialog should display current assignment #40405

Conversation

brandontruggles commented Dec 18, 2017

msftclas commented Dec 18, 2017 • edited

brandontruggles commented Dec 18, 2017 • edited

dadlerj left a comment

Choose a reason for hiding this comment

brandontruggles commented Apr 20, 2018

sandy081 commented Apr 20, 2018

brandontruggles commented Apr 21, 2018

sandy081 commented Apr 23, 2018

brandontruggles commented Apr 24, 2018

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

sandy081 left a comment

Choose a reason for hiding this comment

sandy081 left a comment

Choose a reason for hiding this comment

brandontruggles commented May 18, 2018 • edited

sandy081 commented May 23, 2018

sandy081 commented May 23, 2018

sandy081 commented May 23, 2018

brandontruggles commented May 23, 2018

sandy081 commented May 23, 2018

brandontruggles commented May 23, 2018

sandy081 commented May 23, 2018

msftclas commented Dec 18, 2017 •

edited

brandontruggles commented Dec 18, 2017 •

edited

brandontruggles commented May 18, 2018 •

edited