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: persist grid size along with window size #2127

Merged
merged 2 commits into from Nov 14, 2023

Conversation

sid-6581
Copy link
Contributor

What kind of change does this PR introduce?

  • Feature

Did this PR introduce a breaking change?

  • No

When neovide starts up, it uses a default grid size unless the grid size has explicitly been provided. This is problematic when relying on neovide to remember the last window size, because the grid size will be out of sync with the physical window size. The UI will initially be rendered at the wrong size, which not only doesn't look great, but exposes some other issues like the noice mini view not handling resizes properly. So if a noice mini view is rendered immediately upon startup, and neovide resizes the grid, the mini view will stay in place until it times out and disappears. In my case it stays in the middle of the screen until the 5 second timeout passes, which is quite annoying.

I thought about playing with not making the window visible until the first resize had been handled, but that wouldn't solve the problem of plugins not handling resizes properly, and it would also slow down the time until window is first shown. It didn't seem worth going down that path.

I decided to not introduce another setting flag since I think it makes sense to save the physical window size and the grid size together. The saved grid size would be incorrect if the font dimensions change between neovide launches, but most people probably won't change to very different font dimensions between launches. Even if that happens, the saved grid size will probably be more reasonable than the default, and if it's not it will be resized immediately if needed, so it's no worse than the current situation.

@fredizzimo
Copy link
Member

Hm... I need to check a bit more what's going on here, since the window should not be visible until the actual size is determined after the changes I did here, and some additional fixes after that.

It was briefly broken, but should have been fixed again here #2120, unless the fix does not properly work.

The window should never be visible with the wrong physical size unless you set columns/lines, guifont or padding options after Neovim has already rendered it's first frame. And they are needed in order to properly set the number of lines and columns to be used. The first render might use a different amount of columns / lines than the subsequent ones though, but plugins should handle resizing, so I don't see that as a problem. I think it only concerns Folke's plugins, since lazy.nvim and noice, since they do a lot of work before VimEnter and UIEnter. I'm personally not a fan of making those events almost meaningless, but that's what we have to live with.

Finally, I don't think saving the columns along with the window size is a good solution, since they can conflict with each other, for example if the guifont or padding has been changed since the options have been changed. But maybe it could work if the saved grid size is just used as an initial hint.

@sid-6581
Copy link
Contributor Author

@fredizzimo the physical size of the window is fine on startup, but the initial grid size is just the default, which causes the problems I described. It also looks bad with the statusbar and tabline not being the correct size until the subsequent resize/redraw, so it's not just noice that's the issue here. It's very jarring, and it's worse in neovide (without this PR) than it is in e.g. neovim-qt on my machines. I haven't looked at the neovim-qt code to see if they're doing anything special.

Another thought I just had is maybe we can calculate the correct grid size from the physical window size before doing ui_attach? I haven't looked at the source code to see if the window and renderer exist at that point, but I can try that if you think that might work.

@fredizzimo
Copy link
Member

Another thought I just had is maybe we can calculate the correct grid size from the physical window size before doing ui_attach? I haven't looked at the source code to see if the window and renderer exist at that point, but I can try that if you think that might work.

Unfortunately that's impossible, we need guifont, neovide_padding_top, neovide_padding_bottom neovide_padding_right and neovide_padding_left in addition to the window size, to calculate the columns and lines. And all of that are set in init.lua or init.vim, which are run after nvim_ui_attach is called. We do source ginit.vim before that, but requiring the user to set those options there would be a big breaking change.

@sid-6581
Copy link
Contributor Author

Another thought I just had is maybe we can calculate the correct grid size from the physical window size before doing ui_attach? I haven't looked at the source code to see if the window and renderer exist at that point, but I can try that if you think that might work.

Unfortunately that's impossible, we need guifont, neovide_padding_top, neovide_padding_bottom neovide_padding_right and neovide_padding_left in addition to the window size, to calculate the columns and lines. And all of that are set in init.lua or init.vim, which are run after nvim_ui_attach is called. We do source ginit.vim before that, but requiring the user to set those options there would be a big breaking change.

Indeed, that's what I just found out after trying to go down that path and get guifont. 😊

So then the dilemma is that ui_attach needs to specify the grid size, but we have no way of properly determining the grid size at that point. The two main solutions I see are basically:

  1. what this PR does, which will at least result in a correct grid size 99% of the time vs. 0% of the time for the default grid size. The remaining 1% of the time it wouldn't work would be if they changed the padding or font, but then they're no worse off than they are now since that will trigger a resize.

  2. Don't draw the first frame until nvim has been told the new grid size and has given neovide new content to draw. I'm not sure how complicated this will be with the current structure, or if it will have other drawbacks.

Any thoughts? If you have some ideas I'd be happy to explore them. I've only worked in the neovide codebase since last week, so there's a lot I've yet to familiarize myself with.

@fredizzimo
Copy link
Member

I think we can only do 1. And I think this PR does almost that, but it can only affect the grid size given to ui_attach and nothing else.

  1. Seems impossible, since Neovim has already used the grid size and decided to draw when Neovide gets to know about it. This is when we send a new grid size and hope that the UI redraws itself with the new size. We could of course delay the drawing slightly, but there's no guarantee that Neovim responds to the resize requests, especially since it's not running it's normal update loop until UIEnter has been called.

I tried to delay the showing of the window until UIEnter is called or an error message is shown. I think that would work much better, and Neovim would respond to the resize request almost immediately. But plugins like lazy.nvim manually forces neovim to redraw bypassing all the normal rules, so you ended up with the window not showing when lazy was updating the plugins. So that's why I chose this way of showing the window when the first change is received.

@sid-6581
Copy link
Contributor Author

Is there anything else you'd like me to try or change, or are you still experimenting with the delayed rendering?

src/window/mod.rs Show resolved Hide resolved
@fredizzimo
Copy link
Member

I think it looks good as it is. Upon further inspection it looks like it only affects the initial grid size as it should, so once the compilation problem is fixed it should be ok.

@sid-6581
Copy link
Contributor Author

@fredizzimo should be good to go now.

@fredizzimo
Copy link
Member

I will wait with the merging until tomorrow, in case I think of a better idea. but it looks good.

@fredizzimo fredizzimo merged commit d4ccaba into neovide:main Nov 14, 2023
2 checks passed
@fredizzimo
Copy link
Member

Thanks! This is good enough for now, and hopefully we can find an even better solution in the future, maybe with some extra functionality from Neovim.

@sid-6581
Copy link
Contributor Author

Thanks! This is good enough for now, and hopefully we can find an even better solution in the future, maybe with some extra functionality from Neovim.

I hope so! I'm not a fan of hacky workarounds, but I didn't see a clean solution for this.

@sid-6581 sid-6581 deleted the persist-grid-size branch January 31, 2024 14:15
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.

None yet

2 participants