Skip to content
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

Adopt list commands for editor's "Find References" peek UI #41275

Merged
merged 6 commits into from Jan 9, 2018
Merged

Conversation

bpasero
Copy link
Member

@bpasero bpasero commented Jan 8, 2018

fixes #21692

This adopts the WorkbenchTree for the references widget (one of the last that was still using the plain tree). The benefit is that all navigation within the tree, expand/collapse and opening are then configurable via keybindings (list.* commands) and consistent with all keybindings in other trees/lists.

Notable changes:

  • a new command openReferenceToSide is added to specifically handle the case of opening a reference to the side (this requires a new context key referenceSearchTreeFocused)
  • the controller keyboard handling is removed so that all keyboard access can go through commands from the workbench
  • as such, the widget now reacts to focus and selection events from the tree to open references

@bpasero bpasero added this to the December 2017/January 2018 milestone Jan 8, 2018
@bpasero bpasero self-assigned this Jan 8, 2018
@bpasero bpasero requested a review from jrieken January 8, 2018 10:44
weight: KeybindingsRegistry.WEIGHT.workbenchContrib(50),
primary: KeyMod.CtrlCmd | KeyCode.Enter,
mac: {
primary: KeyMod.WinCtrl | KeyCode.Enter
Copy link
Member

Choose a reason for hiding this comment

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

Why isn't this CtrlCmd?

Copy link
Member Author

Choose a reason for hiding this comment

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

I tried to be consistent with the other places where we have "Open to Side" (explorer, search, problems):

image

}

KeybindingsRegistry.registerCommandAndKeybindingRule({
id: 'closeReferenceSearch',
weight: KeybindingsRegistry.WEIGHT.editorContrib(50),
weight: KeybindingsRegistry.WEIGHT.workbenchContrib(50),
Copy link
Member

Choose a reason for hiding this comment

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

Why workbench and not editor? This feature is also available in the monaco editor....

Copy link
Member Author

Choose a reason for hiding this comment

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

@jrieken the workbench is also registering ESC for trees and lists here: https://github.com/Microsoft/vscode/blob/master/src/vs/workbench/electron-browser/commands.ts#L344. This is needed to preserve the behaviour of ESC closing the references widget. It is a bit ugly, I agree. The alternative would be to register this command also from the workbench maybe.

}
}));
this._disposables.push(controller.onDidOpenToSide((element: any) => {
this._tree = this._instantiationService.createInstance(WorkbenchTree, div.getHTMLElement(), config, options);
Copy link
Member

Choose a reason for hiding this comment

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

Somewhere the 'cmd+click to open to side' feature is being lost here

Copy link
Member Author

Choose a reason for hiding this comment

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

@jrieken fixed via 64f9e8a

@bpasero
Copy link
Member Author

bpasero commented Jan 9, 2018

Also looks like double click was broken for mouse (fixed via 6ebd589). And it looked like each mouse click was triggering the goto definitition twice. Fix is to only react to focus and selection on the tree from keyboard events.

@bpasero
Copy link
Member Author

bpasero commented Jan 9, 2018

Btw neither in stable nor in insiders does "open to side" using keyboard work for me currently from the find references tree.

@jrieken
Copy link
Member

jrieken commented Jan 9, 2018

using keyboard work for me currently from the find references tree.

It does for me

jan-09-2018 08-29-22

// listen on selection and focus
this._disposables.push(controller.onDidFocus((element) => {
var onEvent = (element: any, kind: 'show' | 'goto' | 'side') => {
Copy link
Member

Choose a reason for hiding this comment

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

var 😱

Copy link
Member Author

Choose a reason for hiding this comment

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

@jrieken that is consistent with other usages of var in _fillBody, I did not want to change that style. There are more vars throughout the file, you can clean that up afterwards if you want.

weight: KeybindingsRegistry.WEIGHT.editorContrib(),
primary: KeyMod.CtrlCmd | KeyCode.Enter,
mac: {
primary: KeyMod.WinCtrl | KeyCode.Enter
Copy link
Member

Choose a reason for hiding this comment

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

Why not cmd on macs? We use that in the explorer and elsewhere

Copy link
Member Author

Choose a reason for hiding this comment

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

@jrieken you must have changed the keybindings then, the default is Ctrl+Enter on mac for Explorer, Search and Problems view:

image

Copy link
Member

Choose a reason for hiding this comment

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

true story...

@bpasero
Copy link
Member Author

bpasero commented Jan 9, 2018

@jrieken mouse works for me, but keyboard does not.

@jrieken
Copy link
Member

jrieken commented Jan 9, 2018

Yeah, keyboard doesn't work today.

@jrieken jrieken merged commit 779152c into master Jan 9, 2018
@bpasero bpasero deleted the fix-21692 branch January 10, 2018 05:40
@github-actions github-actions bot locked and limited conversation to collaborators Mar 27, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Adopt list commands for editor's "Find References" peek UI
2 participants