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

Allow passing arbitrary 'context' to setSelection/setFocus #64743

Closed
roblourens opened this issue Dec 10, 2018 · 13 comments
Closed

Allow passing arbitrary 'context' to setSelection/setFocus #64743

roblourens opened this issue Dec 10, 2018 · 13 comments
Assignees
Labels
under-discussion Issue is under discussion for relevance, priority, approach

Comments

@roblourens
Copy link
Member

roblourens commented Dec 10, 2018

The old tree allows us to pass some context into the setSelection/setFocus call which is sent through to onDidChangeSelection/onDidChangeFocus event handlers. This lets us implement different behavior depending on "why" the selection/focus changed.

Search uses it because F4/shift+F4 are debounced, so it can set selection and tell the controller not to open the editor yet: https://github.com/Microsoft/vscode/blob/41cb808a116e608dbfb7e6b56ef368d1795a86e7/src/vs/workbench/parts/search/browser/searchView.ts#L615

Then later it can tell the controller to open the editor and also focus the editor: https://github.com/Microsoft/vscode/blob/41cb808a116e608dbfb7e6b56ef368d1795a86e7/src/vs/workbench/parts/search/browser/searchView.ts#L578

I also see usage of this feature in the explorer, breadcrumbs, outline, and custom view trees.

@roblourens
Copy link
Member Author

Looking into the search scenario some more, it might be possible to work around this by doing the debouncing in the controller instead of SearchView.

@joaomoreno
Copy link
Member

Those links look broken, can you provide new links that point to a commit?

Looking into the search scenario some more, it might be possible to work around this by doing the debouncing in the controller instead of SearchView.

Let me know how that goes.

@joaomoreno joaomoreno added the under-discussion Issue is under discussion for relevance, priority, approach label Dec 11, 2018
@jrieken
Copy link
Member

jrieken commented Dec 11, 2018

needed by #64727

@jrieken
Copy link
Member

jrieken commented Dec 11, 2018

Can you maybe elaborate on why this is exactly needed, on the other issue?

There was a promise of compatibility between the new and old tree. Thousands of lines have been written and I while I can live some change and cannot live with conceptual changes.

@roblourens
Copy link
Member Author

Fixed the links.

@jrieken
Copy link
Member

jrieken commented Feb 1, 2019

@roblourens what did you end up doing here?

@joaomoreno
Copy link
Member

We still only have UIEvent here. We can look into specific usages of this in breadcrumbs/outline, or we can change that type to any as before and add a bunch of duck type checks where we handle it. I'd love if we could go through the first option together.

@jrieken
Copy link
Member

jrieken commented Feb 4, 2019

I'd love if we could go through the first option together.

In the outline if the tree selection changes (via mouse/keyboard) then that symbol is revealed/selected in the editor (this triggers an editor selection change). In return, when the cursor moves we reveal/select the corresponding symbol in the outline (this triggers a tree selection change). To prevent an infinite event loop a payload is used.

@jrieken
Copy link
Member

jrieken commented Feb 4, 2019

I might actually be able to prevent my "marker" it a UIEvent, e.g. FakeEvent extends UIEvent...

@roblourens
Copy link
Member Author

That is what I ended up doing, I just stick a property on a fake keyboard event.

@joaomoreno
Copy link
Member

joaomoreno commented Feb 5, 2019

Some time ago, @bpasero introduced the open event in a new tree navigator concept. This is what I would suggest we do here.

When the user interacts with the tree's focus/selection using the mouse/keyboard, an open event will fire. When code interacts with the tree's focus/selection using the API, no open even will fire. Additionally, the open event should automatically respect openOnSingleClick and openOnFocus based on the user's workbench settings, letting you go of that burden.

The current implementation might be incomplete, but I'd love to push it forward so we align everyone.

@jrieken I would love to help you getting that to work on your branch.

@bpasero
Copy link
Member

bpasero commented Feb 5, 2019

+1 for using the open event and not the raw selection/focus events for the advantages mentioned above.

@joaomoreno
Copy link
Member

I intend to push down the onDidOpen event to the tree, so we can even ditch the Navigator.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
under-discussion Issue is under discussion for relevance, priority, approach
Projects
None yet
Development

No branches or pull requests

4 participants