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

feat: handle nvim statusline #1895

Merged
merged 1 commit into from
Apr 21, 2024

Conversation

xiyaowong
Copy link
Collaborator

@xiyaowong xiyaowong commented Apr 20, 2024

Resolve #1809

"get_status_item" has been removed.

This is a relatively simple implementation at the moment, with a few points to discuss:

  1. Setting a different background color. Be simple
  2. Making the refresh interval configurable. Be simple
  3. In nvim, detecting changes in the statusline and then notifying vscode for updates can reduce RPC calls and make refreshes more timely. However, this approach leads to less concise code. RPC calls are cheap.

@xiyaowong xiyaowong changed the title feat: render nvim statusline feat: handle nvim statusline Apr 20, 2024
@xiyaowong xiyaowong force-pushed the render-nvim-statusline branch 2 times, most recently from 8c48219 to f568553 Compare April 20, 2024 08:36
@justinmk
Copy link
Collaborator

justinmk commented Apr 20, 2024

Why is this needed? Can users use vscode.get_status_item to set the statusline to the result of nvim_eval_statusline ? Can we document some Lua config they can add to achieve that?

Making the refresh interval configurable.

Please, no.

detecting changes in the statusline and then notifying vscode for updates can reduce RPC calls

Premature optimization. RPC calls are cheap.

@xiyaowong
Copy link
Collaborator Author

xiyaowong commented Apr 20, 2024

Why is this needed? Can users use vscode.get_status_item to set the statusline to the result of nvim_eval_statusline ? Can we document some Lua config they can add to achieve that?

There are two differences:

  1. This works out of the box.
  2. I put the statusline on the far left, the status_item on the far right, and the priority between multiple status_items is uncertain.

My personal understanding is that get_status_item provides a simple API for manipulating the status bar of VSCode, while the content added in this PR deals with the statusline of Neovim.

I am neutral about this PR

@justinmk
Copy link
Collaborator

Alternatively, after this PR, can we remove get_status_item ?

@xiyaowong
Copy link
Collaborator Author

I suggest not. There are already plugins using this API. 😶

@justinmk
Copy link
Collaborator

justinmk commented Apr 20, 2024

There are already plugins using this API. 😶

That really doesn't matter. The worst case scenario is simply that people get the default statusline. That doesn't break anyone.

We need to focus on simplifying this codebase.

@xiyaowong
Copy link
Collaborator Author

Let's keep this for now, because it's indeed more convenient compared to statusline. Here are the current use cases. It would be difficult to achieve the same effect without letting users configure it if we were to use statusline.

@justinmk
Copy link
Collaborator

justinmk commented Apr 20, 2024

because it's indeed more convenient compared to statusline

How is it more convenient? Nvim 'statusline' allows users to configure their statusline. If users want to set the vscode statusline, they could just configure the Nvim 'statusline'.

It would be difficult to achieve the same effect without letting users configure it if we were to use statusline.

? folke/flash.nvim#269 is just setting the .text field. Why do we need a separate API for that, if users can set the Nvim 'statusline' and it will be shown in vscode after this PR?

@xiyaowong
Copy link
Collaborator Author

After careful consideration, you are correct. I was thinking from the perspective of a vscode-neovim user and being familiar with the get_status_item API, which was unreasonable. For regular plugin authors, this adds maintenance overhead, and for users, it is often unnecessary.

@xiyaowong xiyaowong force-pushed the render-nvim-statusline branch 2 times, most recently from d5cf8cc to 2e1155e Compare April 21, 2024 05:20
@xiyaowong xiyaowong marked this pull request as ready for review April 21, 2024 05:20
@xiyaowong xiyaowong requested a review from justinmk April 21, 2024 12:37
@xiyaowong xiyaowong force-pushed the render-nvim-statusline branch 2 times, most recently from 4083ffe to 1760250 Compare April 21, 2024 13:01
let sl = (await this.client.lua(`
if vim.o.laststatus == 0 then return "" end
return vim.api.nvim_eval_statusline(vim.o.statusline, {}).str
`)) as any as string;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Just FYI: after the next node-client release, the as any can be dropped. neovim/node-client#353

Copy link
Collaborator

@justinmk justinmk left a comment

Choose a reason for hiding this comment

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

Beautiful!

BREAKING CHANGE: vscode.get_status_item has been removed.
@xiyaowong xiyaowong merged commit fa5fc14 into vscode-neovim:master Apr 21, 2024
8 checks passed
@xiyaowong xiyaowong deleted the render-nvim-statusline branch April 21, 2024 14:50
const refreshNvimStatusLineTimer = setInterval(async () => {
let sl = (await this.client.lua(`
if vim.o.laststatus == 0 then return "" end
return vim.api.nvim_eval_statusline(vim.o.statusline, {}).str
Copy link
Collaborator

@justinmk justinmk Apr 21, 2024

Choose a reason for hiding this comment

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

When 'statusline' option is blank, Nvim still shows a statusline. In that case, the vscode statusline will be blank even though Nvim's rendered statusline is not blank.

That's a historical quirk of Vim, not really up to vscode-neovim to work around it. Tracking issue: neovim/neovim#28444

OTOH, it's probably good that users can disable this vscode-neovim behavior by setting 'statusline' to empty. We can just mention in the docs that if they want the default statusline, they can set it as follows:

set statusline=%<%f\ %h%m%r%=%-14.(%l,%c%V%)\ %P

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

When 'statusline' option is blank, Nvim still shows a statusline. In that case, the vscode statusline will be blank even though Nvim's rendered statusline is not blank.

I thought about this, I don't think anyone needs this default statusline

OTOH, it's probably good that users can disable this vscode-neovim behavior by setting 'statusline' to empty

Yes. So I removed the settings for the laststatus and statusline options instead of setting a default value :)

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.

feature: support for statusline plugins
2 participants