equalsraf / neovim-qt

Neovim client library and GUI, in Qt5.
https://github.com/equalsraf/neovim-qt/wiki
ISC License
1.87k stars 171 forks source link

Initial editor size is different from the actual window size #799

Closed statiolake closed 2 years ago

statiolake commented 3 years ago

This is what I get by nvim-qt -- -O a b:

image

The expected behavior is to open two panes of equal size side by side.

The reason for this unexpected behavior is the initial editor size is hard-coded relative to screen size, not window size:

https://github.com/equalsraf/neovim-qt/blob/d3f4b74021897157f3efd66a2d47791ce70c8250/src/gui/shell.cpp#L225-L227

To fix this, we need to know the actual window size before the Neovim's initialization. But current implementation does opposite: main window is waiting for Neovim's initialization, to fix other issue #70 (in commit 22aebb8e8eaf7b5799360416ff297da09c388469).

I tried removing this delaying behavior completely in my fork, https://github.com/statiolake/neovim-qt/commit/a0ab4fda0f0be3dd87fbb6c07182a983eca3eadc and for me it works very well (including issue in #70 - actually removing delay does change things a bit, I can now see the black window just a moment until the Neovim get initialized, but it's not frustrating for me.)

jgehrig commented 3 years ago

This looks like a duplicate of #508.

I'm not familiar enough with this area of code to comment. I will take a look.

We'll want to make sure we don't induce any startup flicker. Lots of users complained about this in the issue tracker, but not many have complained about the split-panel behavior. I think this delay was introduced to reduce rapid window flashing and resizing on startup?

We'll want to make sure any changes we make to master are compatible with remote Neovim sessions. We should test with slow-to-start instances of nvim, --maximized calls, and --geometry calls. Do you know if your changes are compatible with these cases?

I'm all for fixing this issue, I also find it annoying...

jgehrig commented 3 years ago

For the sake of search discovery...

Here is a common workaround for the scenario: https://github.com/wjrogers/dotfiles/commit/cef4d64bae5b96d3025f123a85f55cb850ca9fd1

It isn't perfect: it has side-effects on window resize and won't work for non-evenly spaced panels.

statiolake commented 3 years ago

This looks like a duplicate of #508.

Exactly. I'm sorry for my lack of searching.

Lots of users complained about this in the issue tracker, but not many have complained about the split-panel behavior. I think this delay was introduced to reduce rapid window flashing and resizing on startup?

For reference, I recoded the startup of both version.

With this fix:

https://user-images.githubusercontent.com/20490597/102810669-7991f300-4407-11eb-9935-ed0f84f9195c.mp4

Without this fix (current released version):

https://user-images.githubusercontent.com/20490597/102810700-831b5b00-4407-11eb-941a-646f7b737a39.mp4

Personally I can't find large difference in my environment.

The situation is the same with --maximized and --qwindowgeometry, looks good for both performance and functionality. Here is screen record launching various screen sizes using --qwindowgeometry

https://user-images.githubusercontent.com/20490597/102812183-f7ef9480-4409-11eb-93b2-3ceb6e469d22.mp4

For remote nvim, I sometimes use Neovim-Qt to connect with the WSL's neovim. I recoded the startup for this use case. In the below screen record, nvim is running in WSL and listening on localhost:16391. Command is nvim --listen localhost:16391 --headless. Now connecting with nvim-qt --server localhost:16391 for fixed version and current release.

With this fix:

https://user-images.githubusercontent.com/20490597/102811685-16a15b80-4409-11eb-9778-56e4600d957c.mp4

Current release:

https://user-images.githubusercontent.com/20490597/102811708-20c35a00-4409-11eb-86d4-4c692ffcdae1.mp4

Current release now seems faster than the fixed version, but I think this is because current release is waiting for at most 1 sec to be connected with Neovim. With careful looking the window appears faster than the current release (since it no longer delays) so overall performance seems almost same.

I'm all for fixing this issue, I also find it annoying...

Yes... If others think it's better to keep current behavior for performance reasons or this fix breaks someone's use case, I'll use this fix only for myself. I agree that -O a b is rarely used. And I know this sort of fix is hard to test. I don't mind the startup behavior in my environment, but in other environment there may be noticeable lags.

Here is a common workaround for the scenario:

Thanks for workaround. I thought I could fix this by vim scripts too, but this seemed some sort of dirty hack for me...

(By the way, uploading MP4 is very useful feature.)

jgehrig commented 3 years ago

Exactly. I'm sorry for my lack of searching.

No worries! I just wanted to create a GitHub link. Same reason for the workaround. For users searching this issue in the future.

Awesome use of screen recordings and very thorough testing! Thanks :smile:

I will apply the patch to my machines and test. I want to be cautious with changes in this area. I'm open to a merge, your improvements sound reasonable.

@equalsraf Any thoughts? Is there anything we need to be aware of regarding the delay?

nfd commented 3 years ago

Just wanted to add another voice here -- I encounter this window-size error when using sessions. Specifically, nvim-qt -- -S Session.vim, where the restored session has a vertical split, results in a layout like the OP picture. Applying statiolake's patch fixes the issue for me, though I haven't tested it much. I can confirm that the patched version flashes on startup, though (which is okay for me personally since I tend to start nvim-qt once and run it all day).

Just thought I'd mention it, because I would have thought (though perhaps incorrectly, given the relative inactivity of this issue!) that what I was doing was rather common.

statiolake commented 3 years ago

As a note, I discovered another problem in my patch. The issue may appear for users who customize the font in their init.vim (like :set guifont, :GuiFont, etc.) even if they are using my patched version.

With my patch, at first Neovim-qt calculates the appropriate number of lines and columns based on the size of the default font (Consolas of size 11 on Windows, for example). We can't use the actual font because we don't know the final guifont before launching Neovim. Then, if guifont is changed in init.vim, the calculated lines and columns no longer match to the main window's size in most cases. Unlike GVim, Neovim-qt doesn't adjust the main window's size to preserve lines and columns, so the same problem at last.

What I want to say, with more concrete example:

  1. Neovim-qt is launched with option -O (or load session file containing splits).
  2. Neovim-qt creates main window.
  3. Neovim-qt calculates the lines and columns from the size of the main window and the default font (like Consolas of size 11). Suppose the width was 120 for now.
  4. Neovim-qt launches Neovim and pass the lines and columns to it.
  5. Neovim equally divides them for each window. In the above example, one is 60 and the other is also 60 (ignore UI elements for simplicity)
  6. Neovim loads init.vim containing :set guifont=SomeFont:hXX
  7. Neovim-qt changes the guifont acccordingly, preserving window size as is. Say the new font was 1.2x larger than the default font.
  8. Neovim-qt recalculates the lines and columns according to the new font size, so new lines and columns will be 100x30.
  9. Neovim receives the new lines and columns and set to them.
  10. We have inequally divided windows. One is still 60 but the other is now 40 (=100-60).

I think we need to find some solution for this problem before solving the original issue.

jgehrig commented 3 years ago

@nfd

Thanks for the ping! This is helpful in prioritizing work and understanding what work users value.

I've seen several users complain about this. It seems like something worth merging!

@statiolake

As a note, I discovered another problem in my patch. The issue may appear for users who customize the font in their init.vim (like :set guifont, :GuiFont, etc.) even if they are using my patched version.

I think #825 would resolve this issue.

We can read the last-known GuiFont value synchronously via QSettings on startup. That provides a last-known font without requiring Neovim attachment.

I was a little concerned the new behavior might annoy/confuse people... When a user issues :GuiFont or any of the other covered commands, the value will persist. What do you think?

The PR above would resolve this issue, right?

statiolake commented 3 years ago

@jgehrig

I think #825 would resolve this issue.

I confirmed that the issue is resolved, but I understand your concern too. It breaks reproducibility in a sense...

Two things I (and maybe you) are concerned about:

  1. The cached settings are not overwritten before the next launch: so we need to launch twice before getting everything correctly configured. For example, we'll have this issue for the first time after guifont changes in init.vim. That might confuse users why the issue occurs only at the first time.
  2. The changes made for a temporary situation also persist. For example, increasing font size to do something like live coding also persists in the settings and it will be used in the next startup calculations unintentionally.

Although I think 2. may not be a big issue since the target settings are limited to GuiFont, GuiTabline and GuiPopupmenu - GuiFont is explicitly specified in init.vim in most cases, and GuiTabline and GuiPopupmenu are rarely changed. But even if so, the next time window splitting will be broken just as 1.

Speaking of personal things, I'm not using ginit.vim. All my settings are done in init.vim, so neovim-qt -- --clean doesn't load any of my gui settings, so the last settings will be used in --clean. Anyone doing like me might feel surprised, especially --clean is expected to clear any user preferences like GVim (though in Neovim --clean is a Neovim option and not Neovim-qt's, so it's different from GVim's situation). However I also think such a situation can hardly happen in ordinal users.

That said, we can't solve them entirely without parsing init.vim (and ginit.vim) before launching Neovim-qt (and it can't --- that's too complex and error prune!). Changing window size like GVim can also cause problems in some WM (such as tiling window manager). Therefore #825 seems good as a realistic solution to me. If I add one more thing to it, I think it's better to have a command line options to clear those cached settings.

jgehrig commented 3 years ago

@statiolake

Generally, I agree with everything you've said. This isn't ideal, but it is the best option available to us.

If you create a Pull Request, I'll work to get it merged (after #825).


I don't think the corner cases you've pointed out are of big concern. We won't be able to fix those scenarios, but we also won't make any existing behavior worse. Some cases that are broken would remain broken; most cases would be fixed.

By biggest concern is the following scenario:

  1. The user doesn't have a default font set in init.vim or `ginit.vim
  2. The user increases the font size with :GuiFont Some_Font:h20: live coding, presentation, etc.
  3. The user reboots neovim-qt, expecting the default font back.
  4. Behavior change: the temporary font now persists.

The general case should be safe. Any user that sets GuiFont or guifont will override the QSettings value. As you called out in 2 this would temporarily break the screen-size logic, but that is broken today. The change is a (partial) improvement.

The cached settings are not overwritten before the next launch: so we need to launch twice before getting everything correctly configured. For example, we'll have this issue for the first time after guifont changes in init.vim. That might confuse users why the issue occurs only at the first time.

Can you elaborate on this? I'd expect the QSettings values to reflect guifont immediately after it was called.

There is nothing we can do for the first launch. But the second launch should be fine as long as guifont has not changed.

That said, we can't solve them entirely without parsing init.vim (and ginit.vim) before launching Neovim-qt (and it can't --- that's too complex and error prune!).

Agreed completely!

If I add one more thing to it, I think it's better to have a command line options to clear those cached settings.

What do you envision?

Deleting the cache is simple (except on Windows): rm ~/.config/nvim-qt/nvim-qt.conf

I suppose an optional flag to ignore the cache values temporarily could be added. If there is a compelling scenario, we can add something. Otherwise, I prefer to keep-it-simple with less options.

If "except on Windows" and avoiding regedit is the problem, we could add a :GuiClearSettings, QCommandLineParser is awful and I prefer not relying on it if possible.

statiolake commented 3 years ago

@jgehrig

If you create a Pull Request, I'll work to get it merged (after #825).

OK. I'll open a PR when I have time after #825 is merged.

By biggest concern is the following scenario:

First of all, I don't think they are big issues. I agree with the fix using QSettings, as it's only practical way to fix the original issue. I just wanted to say I thought it's better to have a command line options to clear or disable QSettings if it's easy, since it can reduce my (already little) concerns a bit more.

The below is just what I thought. It may be a misplaced concern, since I'm a new to develop or contribute to software. And I would agree if you said those were my preference...

I'm concerned about rather debugging (or problem reporting) hardness than usability. Generally speaking, it's better to have less states we need to consider. In Neovim-qt, currently we only need to care about init.vim and ginit.vim, and they are explicit (we can ask the issue author what do they change or to show their init.vim). init.vim can also be disabled by --clean. So when someone found a bug we can basically reproduce the same problem with the same instruction. But we are adding a new, implicit state here: QSettings, and, this is not transparent in the sense that "behavior may change even if explicit settings like init.vim, ginit.vim or steps to reproduce are all the same". It can make debugging a bit difficult.

Low-quality examples...

1. If someone found a bug which is caused by a temporal `GuiFont` change (like this issue, but not limited to this), at first the ordinary user may feel the bug appears randomly, since the launch after the next launch doesn't cause the problem. Some user (like me) may hasitate to create a issue about it because it's unclear that how to reproduce it. Even if someone posted a issue, we need to investigate how to reproduce it first. 2. If there is a bug only appears when `GuiTabline` is enabled, and if the user have enabled `GuiTabline` previously, the bug is always appear for that user even with the clean, default settings. But anyone who haven't enabled `GuiTabline` can't reproduce the bug, and this case the investigation to find the root cause might be harder ― especially the consequence doesn't seem to be related with `GuiTabline` such as "always crash when I open specific file, and that was because the file name can't be displayed as the tab label due to Qt's bug" ... this is completely imaginary situation.

However that's actually not a big problem, because stored settings are few and unlikely to increase, so we can easily test a problem with all combinations of settings manually.

Can you elaborate on this?

That's about when changing init.vim directly, not running the command in command line. Exactly what you said after that:

There is nothing we can do for the first launch. But the second launch should be fine as long as guifont has not changed.

  1. Change commands like :GuiFont in init.vim.
  2. First launch: during loading of init.vim, settings are written in QSettings. (this time QSettings and init.vim disagree, problems can occur)
  3. Second launch: now init.vim and QSettings agree. I called this situation "everything correctly configured".

What I concern is that problems only happen in the first launch can be harder to discover and investigate. In addition, I feel slightly unreliable if my editor is sometimes broken for unclear reason.

What do you envision?

With such a option, if I encountered problems like above, I can simply remove QSettings to check if it is QSettings related. And if there's an issue which seems to be related to GUI settings, we can simply ask the issue author like "Does your problem persist even if you run neovim-qt --clean-qsettings?". For those purposes, disabling QSettings temporary is enough, but I slightly prefer the option to clear QSettings because I don't want to do something like "hack" (finding the setting storage depending on the platform: $XDG_CONFIG_HOME, registry, %USERPROFILE%\AppData..., etc) to reset Neovim-qt.

I also slightly prefer a command line option rather than :GuiClearSettings, since command line option can strictly guarantee that there is no QSettings affected in the run, whereas we can change settings after :GuiClearSettings even implicitly via autocmd. However, I didn't know about

QCommandLineParser is awful and I prefer not relying on it if possible.`

... so now I think :GuiClearSettings is OK if the command line option is too complicated. That's no more than my preference...

jgehrig commented 3 years ago

All sounds good. I think we're on the same page.

I'll get #825 merged in the next day or two. I'll @-mention you once it is completed.


it's better to have less states we need to consider.

+1 Avoiding state whenever possible is a good thing; great design practice.

... so now I think :GuiClearSettings is OK if the command line option is too complicated. That's no more than my preference...

Okay, lets do :GuiClearSetttings.

Good points, this would be valuable if users file bugs. Ex) Try :GuiClearSettings then run your steps twice. Simple.

The below is just what I thought. It may be a misplaced concern, since I'm a new to develop or contribute to software.

It doesn't show. You're doing great, keep up the good work!

Don't hesitant to file issues, open pull requests, @-mention me, etc. Your contributions have all been great so far!

Nope, no misplaced concerns. I like to bounce ideas off people and discuss designs. I find a few people working together results in more robust designs than one person working alone. You might think of something I missed and vice-versa.

jgehrig commented 3 years ago

@statiolake

Pull Request 825 has been merged. I'm going to introduce something like :GuiClearSettings in a follow-up.

I think I will structure the function similar to GuiWindowMaximized, omitting a command will keep it more hidden.

I'd like to see if anyone complains about this feature before adding to the shim plugin...

MIvanchev commented 3 years ago

Just like @nfd I encounter this issue with sessions (I use vim-obsession), here's an excerpt of my init.vim:

" Autoload sessions (https://gist.github.com/robmiller/5135652)
augroup sourcesession
  autocmd!
  autocmd VimEnter * nested
    \ if !argc() && empty(v:this_session) && file_readable($HOME . '/Session.vim') |
    \   source $HOME/Session.vim |
    \ endif |
    \
    \ Obsession $HOME/Session.vim
augroup END

My top split is getting smaller and smaller with every neovim restart. Is this going to be fixed in the next release?

jgehrig commented 3 years ago

@MIvanchev

Yes, this will definitely be fixed in the next release.

The Pull Request has some minor issues that should be resolved before merge.

In the meantime, you can download a build that contains the fix. Click the ✔️ or ❌, and look for "Artifacts" in the Actions "Build and Test" run for your platform.

https://github.com/equalsraf/neovim-qt/actions/runs/1064999418

MIvanchev commented 3 years ago

Nice thank you!

bybor commented 2 years ago

Could you please confirm a couple things? I don't know if this fix has been already released - I run 0.6.1 in Windows. I cannot find a test build referenced above - may be it is no longer available?

1) So, Windows 10, nvim-qt, and when I put GuiFont! into init.vim it doesn't apply and it errors out during start - I have to put it into ginit.vim. Is it specific to Windows? 2) I understand it in a way that the fix is to set font (using GuiFont) before GUI initialization, so it just catches up. 3) it doesn't work with set guifont=, right?

Thank you.

MIvanchev commented 2 years ago

@bybor you're probably in the wrong discussion, but yes, the Gui- commands need to be in ginit.vim. This is not only on Windows. I don't quite understand point 2, GuiFont is part of the GUI initialization. As to point 3, no, you can't set GuiFont. GuiFont is a command.

bybor commented 2 years ago

@MIvanchev , thank you!

I think this is the right discussion because I have an issue when I start nvim-qt with a command line to open a text file, it starts with one window size but after it executes GuiFont (from ginit.vim), it resizes a window.

I wrote my message because I wanted to understand if the fix had been released - and if I can try it. Some of the messages in this discussion mention putting GuiFont command into init.vim, which confused me since it doesn't work on my end. That is why I wanted to make sure that I understand the idea behind the fix correctly.

set guifont=<FontName> is the regular vim command - I understand that it is differnet from nvim-qt's GuiFont <FontName> command, but I was curious if the fix is specific to GuiFont or if it also applies to regular set guifont.

I think I'm all set. I subscribed to this issue to see when it is resolved, which would mean that it is merged.

Thank you!

MIvanchev commented 2 years ago

@bybor I'm still having the size issue myself although in a different context so it doesn't appear to be solved yet, the PR is also not merged yet.

jgehrig commented 2 years ago

The fix has been merged. Marking the issue as closed. Please post here if you see any outstanding issues.

Thanks again @statiolake for this fix!