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

Simplify the server status item click action #3537

Merged
merged 2 commits into from
Mar 22, 2024

Conversation

jdneo
Copy link
Collaborator

@jdneo jdneo commented Mar 20, 2024

This PR simplifies the actions when click the server status bar item. The item for the server status is removed. Instead, when clicking on the server status, it will directly open the build job terminal, and show the shortcut dropdown at the meantime.

@rgrunber rgrunber added this to the End March 2024 milestone Mar 20, 2024
@rgrunber
Copy link
Member

I'm seeing some odd behaviour with this change. The server status terminal view does get focus upon clicking, but the quick-pick also immediately disappears.

status-bar-flicker

It seems to me like the act of showing the server status closes the already opened quick-pick since the executeCommand runs asynchronously. I tried adding an await but still doesn't work. Occasionally it does stay open, so it's definitely timing related.

Signed-off-by: Sheng Chen <[email protected]>
@jdneo
Copy link
Collaborator Author

jdneo commented Mar 21, 2024

Ah, good catch!

I tried first adding the preserveFocus parameter when show the status terminal, but that did not work.

Then I went to the current solution that adding ignoreFocusOut flag when displaying the dropdown list. It did work, but a side effect is that now user needs to press esc to close it.

Copy link
Member

@rgrunber rgrunber left a comment

Choose a reason for hiding this comment

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

Change looks fine and works well. I found one minor thing so if it can be addressed, feel free to do so. If not, I'm also fine to squash+merge.

const choice = await window.showQuickPick(items);
const choice = await window.showQuickPick(items, {
ignoreFocusOut: true,
placeHolder: "Press 'ESC' to close."
Copy link
Member

@rgrunber rgrunber Mar 21, 2024

Choose a reason for hiding this comment

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

This is true in the default case, but it's just controlled by workbench.action.closeQuickOpen which has keyboard shortcuts. So technically someone could change that option (for eg. make it shift+esc) and esc would no longer work.

Does VS Code have API to get the main keyboard shortcut currently represented by some command ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Found this answer: microsoft/vscode-discussions#780

Looks like there is no command but we can inspect the json file to check if user sets other keybindings.

I tend to keep the current simple implementation, but at least, we know there is a solution if we have to address it in the future.

@jdneo jdneo merged commit 8e71cb2 into redhat-developer:master Mar 22, 2024
2 checks passed
@jdneo jdneo deleted the cs/server-status branch March 22, 2024 00:51
@rgrunber
Copy link
Member

I can definitely confirm (at least on Linux) that the first activation of the server status bar (mouse click), followed by usage of the arrow keys to navigate the quick-pick works. However, upon activating it a second time (mouse click), the arrow keys have no effect, as the quick-pick does not appear to have focus.

@jdneo
Copy link
Collaborator Author

jdneo commented Mar 28, 2024

@rgrunber Should be fixed by #3548

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants