add focus state to minihtml tree view#2954
Conversation
|
(forgot to add test keybindings) |
✅ Deploy Preview for sublime-lsp ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
It's not really clear from the description and code comments which parts of this PR are intended to be included in LSP, but a few things that I saw from a quick look over the code are:
|
|
The idea was to not include key bindings and the command for navigating. |
I thought about it but couldn't think of pros and cons of either. |
Yes, but we don't have any menu item for up/down/left/right (LspMoveFocusCommand) and hopefully also won't add that in the future. Adding key bindings for such essential keys up/down/left/right without any context restriction would feel too dangerous from my side. I doubt that something like that would be accepted if this were a new package for https://github.com/sublimehq/package_control_channel
I haven't tested the feature, but that seems like a better idea. That is also the behavior in the sidebar. |
I am not sure but I think is_enabled and context have essentially equivalent behavior so I don't think that is_enabled is more dangerous. As I've realized in sublimelsp/LSP-json#231 (comment), a context doesn't have any advantage if the command is not available for some reason - ST still wouldn't ignore it. That said, I'm not against changing to context, if only for better transparency. |
|
EventListener should be triggered when checking context for key bindings executing a WindowCommand, I assume. But I haven't tested that.
That was what I assumed. Because the key binding from the sublime-keymap file would just override the corresponding bindings from the Default package, if not restricted by a context. |
No, HtmlSheets are half baked... I don't see any way to ensure that key binding can only trigger in their context. |
|
Pushed my keybinding updates which DON'T WORK given the above. I will have to remove those from the PR but maybe ST will wake up one day and make it possible... |
| // Navigate in Tree Views | ||
| { | ||
| "keys": ["left"], | ||
| "command": "lsp_handle_tree_view_action", | ||
| "args": {"action": "move_left"}, | ||
| "context": [{"key": "lsp.tree_view_sheet_has_focus"}] | ||
| }, | ||
| { | ||
| "keys": ["right"], | ||
| "command": "lsp_handle_tree_view_action", | ||
| "args": {"action": "move_right"}, | ||
| "context": [{"key": "lsp.tree_view_sheet_has_focus"}] | ||
| }, | ||
| { | ||
| "keys": ["up"], | ||
| "command": "lsp_handle_tree_view_action", | ||
| "args": {"action": "move_up"}, | ||
| "context": [{"key": "lsp.tree_view_sheet_has_focus"}] | ||
| }, | ||
| { | ||
| "keys": ["down"], | ||
| "command": "lsp_handle_tree_view_action", | ||
| "args": {"action": "move_down"}, | ||
| "context": [{"key": "lsp.tree_view_sheet_has_focus"}] | ||
| }, | ||
| { | ||
| "keys": ["enter"], | ||
| "command": "lsp_handle_tree_view_action", | ||
| "args": {"action": "activate"}, | ||
| "context": [{"key": "lsp.tree_view_sheet_has_focus"}] | ||
| }, | ||
| { | ||
| "keys": ["escape"], | ||
| "command": "lsp_handle_tree_view_action", | ||
| "args": {"action": "close"}, | ||
| "context": [{"key": "lsp.tree_view_sheet_has_focus"}] | ||
| }, |
There was a problem hiding this comment.
Since those use context that is never set, we could keep those in the code, waiting for better times?
There was a problem hiding this comment.
I would probably comment them out for now. It seems that ST considers the context as "false" if there is no plugin that handles this context key. If we want to keep the key bindings, maybe we should already add a handler like
def on_query_context(self, view: sublime.View, key: str, operator: sublime.QueryOperator, operand: str, match_all: bool) → bool | None:
if key == 'lsp.tree_view_sheet_has_focus':
return False
return None
| self.id = str(uuid.uuid4()) | ||
|
|
||
| def html(self, sheet_name: str, indent_level: int) -> str: | ||
| def html(self, sheet_name: str, indent_level: int, is_active: bool = False) -> str: |
| padding-left: 0.5rem; | ||
| }} | ||
| .active {{ | ||
| background-color: color(var(--accent)); |
There was a problem hiding this comment.
I think something like color(var(--accent) alpha(0.3)) would be better because otherwise the text foreground might not have sufficient contrast to the accent background color.
| // Navigate in Tree Views | ||
| { | ||
| "keys": ["left"], | ||
| "command": "lsp_handle_tree_view_action", | ||
| "args": {"action": "move_left"}, | ||
| "context": [{"key": "lsp.tree_view_sheet_has_focus"}] | ||
| }, | ||
| { | ||
| "keys": ["right"], | ||
| "command": "lsp_handle_tree_view_action", | ||
| "args": {"action": "move_right"}, | ||
| "context": [{"key": "lsp.tree_view_sheet_has_focus"}] | ||
| }, | ||
| { | ||
| "keys": ["up"], | ||
| "command": "lsp_handle_tree_view_action", | ||
| "args": {"action": "move_up"}, | ||
| "context": [{"key": "lsp.tree_view_sheet_has_focus"}] | ||
| }, | ||
| { | ||
| "keys": ["down"], | ||
| "command": "lsp_handle_tree_view_action", | ||
| "args": {"action": "move_down"}, | ||
| "context": [{"key": "lsp.tree_view_sheet_has_focus"}] | ||
| }, | ||
| { | ||
| "keys": ["enter"], | ||
| "command": "lsp_handle_tree_view_action", | ||
| "args": {"action": "activate"}, | ||
| "context": [{"key": "lsp.tree_view_sheet_has_focus"}] | ||
| }, | ||
| { | ||
| "keys": ["escape"], | ||
| "command": "lsp_handle_tree_view_action", | ||
| "args": {"action": "close"}, | ||
| "context": [{"key": "lsp.tree_view_sheet_has_focus"}] | ||
| }, |
There was a problem hiding this comment.
I would probably comment them out for now. It seems that ST considers the context as "false" if there is no plugin that handles this context key. If we want to keep the key bindings, maybe we should already add a handler like
def on_query_context(self, view: sublime.View, key: str, operator: sublime.QueryOperator, operand: str, match_all: bool) → bool | None:
if key == 'lsp.tree_view_sheet_has_focus':
return False
return None3 participants
The idea: implement focus handling in tree view (used by call hierarchy) so that it's possible to navigate quickly using keyboard. I kinda want this functionality for another use case that doesn't exist yet (navigating symbol list) but it would be nice to have for call hierarchy also.
Problem with implementing keyboard navigation for call hierarchy is that clicking a caller focuses another view which is interfering with being able to navigate quickly. This is not a solved problem and it means that supporting keyboard navigation in it might not be feasible (though I haven't thought hard enough about it yet). That said, supporting focus state would be useful anyway.
I've included some keybindings and a command for navigating using keyboard for testing but those are not meant to be integrated in their current form, most likely. This PR mostly just concerns adding focus state. The keyboard navigation is what it allows to build on top of that but this PR is not really meant to add that, it's here just to be able to test the focus state.
Screen.Recording.2026-06-13.at.12.26.47.mov