-
Notifications
You must be signed in to change notification settings - Fork 28.9k
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
Feature Addition for Issue #24396 - New "Keyboard Shortcut" dialog should display current assignment #40405
Conversation
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
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. |
@brandonrninefive Thanks for the PR. Some changes
|
@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. |
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.
@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. |
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 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
keybinding
argument is not used, so this can be removed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@brandonrninefive My suggestion is as follows:
-
Change
onDidChange
andonLinkClick
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 statethis._firstPart
andthis._chordPart
. Seedefine()
method for how to get the key
|
||
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]>()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Events can emit key
string just like how define
method returns key string
@@ -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> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do not think changes to this function are needed, if the events return the key
string
if (value !== this.searchWidget.getValue()) { | ||
this.searchWidget.setValue(value); | ||
} | ||
return TPromise.as(null); | ||
} | ||
|
||
showOverlayConflicts(keybinding: [ResolvedKeybinding, ResolvedKeybinding]): void { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- This method should be private
- It takes the
key
as string and call thefetch
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);
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please check the comments
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please check the comments
… display items based on the same keybinding. Also reworked how conflicting keybindings are searched for.
@brandonrninefive Thanks for the changes. LGTM. |
@brandonrninefive Did a bit of clean up (cosmetic changes). Wondering why is this change needed in
|
I have removed the above change while clean up. Please let me know if it is needed. |
@sandy081 That change in |
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. |
@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. |
True. But, filtering always considered a quoted string to match a keybinding completely. |
Please let me know if there are any issues.