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

Fix remember window size #2120

Merged
merged 2 commits into from Nov 11, 2023
Merged

Fix remember window size #2120

merged 2 commits into from Nov 11, 2023

Conversation

Kethku
Copy link
Member

@Kethku Kethku commented Nov 10, 2023

The new option observation starts sending notifications earlier than the previous solution. So we need to ignore the columns and lines change notifications until the ui has finished initializing or we don't properly set the window size

What kind of change does this PR introduce?

  • Fix

Did this PR introduce a breaking change?

  • No

@9mm
Copy link
Contributor

9mm commented Nov 10, 2023

Hmm.. I hate to be bringer of bad news but it didn't seem to fix issue.

image

This is how it starts (instead of right 1/2 of my screen)

Also did cargo clean and cargo run --release

Let me know how else I could help if you want me to test other stuff

@9mm
Copy link
Contributor

9mm commented Nov 10, 2023

A log if that helps
neovide_rCURRENT.log

Copy link

github-actions bot commented Nov 10, 2023

Test Results

    3 files      3 suites   11s ⏱️
  98 tests   98 ✔️ 0 💤 0
294 runs  294 ✔️ 0 💤 0

Results for commit 78a2b2c.

♻️ This comment has been updated with latest results.

@Kethku
Copy link
Member Author

Kethku commented Nov 10, 2023

And you for sure dont have a line in your config that sets the lines and columns?

@9mm
Copy link
Contributor

9mm commented Nov 10, 2023

I'm 99% sure, but what is the exact config variable name and ill search for it

➜  nvim git:(master) ✗ ack colu
init.lua
58:vim.opt.signcolumn = 'yes:1' -- auto:1'

lua/zesty/plugins/mini.lua
26:          -- align first column by default
➜  nvim git:(master) ✗ pwd
/Users/zesty/.config/nvim

There's also nothing for lines except these which arent it:

➜  nvim git:(master) ✗ ack lines
lua/zesty/keymaps.lua
14:-- move lines
17:vim.keymap.set('x', '<D-k>', ":m '<-2<CR>gv=gv", { desc = 'Move lines up'})
18:vim.keymap.set('x', '<D-j>', ":m '>+1<CR>gv=gv", { desc = 'Move lines down'})
54:-- dont skip wrapped lines
61:-- delete empty lines
62:vim.keymap.set('n', '<Leader>del', ':g/^$/d<CR>', { desc = 'Delete empty lines' })

lua/zesty/plugins/theme.lua
42:            underlines = {

lua/zesty/plugins/lsp_cmp.lua
35:      max_lines = 1000,

lua/zesty/plugins/mini.lua
90:          trailspace.trim_last_lines()

lua/zesty/plugins/repl.lua
8:        ignore_blank_lines = true,
27:      desc = 'REPL lines',

lua/zesty/plugins/treesitter.lua
107:        max_lines = 2,

@Kethku
Copy link
Member Author

Kethku commented Nov 10, 2023

its super weird. Your log file indicates that the lines and columns are getting set which triggers a window resize

@9mm
Copy link
Contributor

9mm commented Nov 10, 2023

Could that be set in the config file? As in, its reading from the config file properly but breaking after?

Because one thing I do notice (like last time) is the config is actually saved properly, just reading it seems to be the problem. (indicated by it being broken for awhile, and when I use a compiled version that works, suddenly it restores back the size I had saved some time ago as if nothing was ever wrong)

@Kethku
Copy link
Member Author

Kethku commented Nov 10, 2023

The config file does have a maximized setting but I dont think that would conflict here. The persistent file is in the nvim-data directory and should be read just like before.

@9mm
Copy link
Contributor

9mm commented Nov 10, 2023

here are my main settings, and gui settings below. probably not very helpful

--                          __
-- .-----.-----.-----.--.--|__.--------.
-- |     |  -__|  _  |  |  |  |        |
-- |__|__|_____|_____|\___/|__|__|__|__|

-- essentials
vim.g.mapleader = ','
vim.g.maplocalleader = ','
vim.opt.termguicolors = true

-- lazy
local lazypath = vim.fn.stdpath('data') .. '/lazy/lazy.nvim'
if not vim.loop.fs_stat(lazypath) then
  vim.fn.system({
    'git',
    'clone',
    '--filter=blob:none',
    'https://github.com/folke/lazy.nvim.git',
    '--branch=stable',
    lazypath,
  })
end
vim.opt.rtp:prepend(lazypath)
require('lazy').setup('zesty.plugins', {
  change_detection = {
    enabled = true,
    notify = false,
  },
})

-- gui
require('zesty.gui')

-- keymaps
require('zesty.keymaps')

-- autocommands
require('zesty.autocommands')

-- aesthetic
vim.opt.guifont = 'JetBrainsMono Nerd Font:h14' -- AnonymicePro Nerd Font:h15

-- indentation
vim.opt.autoindent  = true -- continue indentation to new line
vim.opt.smartindent = true -- add extra indent when it makes sense
vim.opt.smarttab    = true -- <tab> at the start of a line behaves as expected
vim.opt.expandtab   = true -- <tab> inserts spaces
vim.opt.shiftwidth  = 2    -- >>, << shift line by 4 spaces
vim.opt.tabstop     = 2    -- <tab> appears as 4 spaces
vim.opt.softtabstop = 2    -- <tab> behaves as 4 spaces when editing

-- cursor
vim.opt.guicursor = ''
--vim.opt.virtualedit = 'all'

-- line numbers
vim.opt.number = true
vim.opt.signcolumn = 'yes:1' -- auto:1'

-- wrap
vim.opt.wrap = false
vim.opt.linebreak = true
vim.opt.breakindent = true

-- scroll
vim.opt.scrolloff = 4
vim.opt.mouse = 'nv'
vim.opt.mousescroll = 'ver:2,hor:0'

-- splits
vim.opt.splitright = true
vim.opt.splitbelow = true

-- folds
vim.opt.foldtext = 'v:lua.vim.treesitter.foldtext()'

-- search
vim.opt.hlsearch = true
vim.opt.incsearch = true
vim.opt.ignorecase = true
vim.opt.smartcase = true

-- recovery
vim.opt.backup = false
vim.opt.undolevels = 1000
vim.opt.undofile = false
vim.opt.undodir = os.getenv('HOME') .. '/.local/state/nvim/undo//'
vim.opt.swapfile = true
vim.opt.directory = os.getenv('HOME') .. '/.local/state/nvim/swap//'

-- misc
vim.opt.updatetime = 50
vim.opt.timeout = true
vim.opt.timeoutlen = 600
vim.opt.shortmess:append('I')

-- filetypes
vim.filetype.add({
  extension = {
    pcss = 'css',
  },
})
  -- padding
  vim.g.neovide_padding_top    = 20
  vim.g.neovide_padding_bottom = 20
  vim.g.neovide_padding_right  = 20
  vim.g.neovide_padding_left   = 20

  -- display
  --vim.g.neovide_transparency = 0.8
  --vim.g.transparency = 0.8
  --vim.g.neovide_background_color = '#0f0f12' .. alpha()
  vim.g.neovide_refresh_rate = 9999 -- fps peaks over 1000+ (requires NEOVIDE_VSYNC=0)
  vim.g.neovide_refresh_rate_idle = 5
  vim.g.neovide_hide_mouse_when_typing = true
  vim.g.neovide_underline_automatic_scaling = true

  -- window
  vim.g.neovide_remember_window_size = true
  vim.g.neovide_confirm_quit = true

  -- animation
  vim.g.neovide_cursor_animation_length = 0
  vim.g.neovide_cursor_trail_size = 0
  vim.g.neovide_scroll_animation_length = 0
  vim.g.neovide_cursor_animate_command_line = false

  vim.g.neovide_input_macos_option_key_is_meta = 'OnlyLeft'

  -- https://github.com/neovide/neovide/issues/1838
  vim.keymap.set('t', '<MouseMove>', '<NOP>')

@Kethku
Copy link
Member Author

Kethku commented Nov 10, 2023

Yeah doesn't look like it gets set at all. Very strange...

@9mm
Copy link
Contributor

9mm commented Nov 10, 2023

Is there any more insight here maybe?

#1580
#2027

@9mm
Copy link
Contributor

9mm commented Nov 10, 2023

More specifically the last one, 2027, is where he fixed it the first time

@Kethku
Copy link
Member Author

Kethku commented Nov 10, 2023

Ill look into it more tomorrow. The timing is subtle i think

@fredizzimo
Copy link
Member

@Kethku, based on the log the problem is that it reads the initial columns and lines.

The resizing implementation is based on the fact that the columns and lines autocommand is only called when the user sets the column or line manually But with your changes it's also called with the initial value.

And a couple of other things that I did not had time to review in the original PR

  1. The reader_func or setting the initial values was really useful for at least me. I use the autocompletion all the time. I also frequently check the existing value of the options typing it's name. So this feels like a regression, but obviously we should only do it for our settings where the initial values are controlled by us and not for neovim options.
  2. I'm ok with combining KeyboardSettings and WindowSettings, but it feels arbitrary to just do it for those two and not for CursorSettings and RenderSettings as well.
  3. This was already addressed here, original autocommands were not deleted.

@Kethku
Copy link
Member Author

Kethku commented Nov 10, 2023

Okeydokey. I will put it back. Thanks for taking a look.

@fredizzimo
Copy link
Member

And
4. This is a bigger change, and did not get much worse in your PR, but I think we should move the setting vimscript code to our Neovide init.lua and just pass the list of settings to that. This would improve the startup times on low latency connections. With the current implementation there are two separate RPC calls for each setting reading the initial value and adding the listener. So with a round-trip of just 10 ms and 50 settings, it would mean a full second extra second startup time.

@Kethku
Copy link
Member Author

Kethku commented Nov 11, 2023

I'm working on 4. Small hiccup here is that I dont think we can call dictwatcheradd from lua. Theres a fair amount of discussion in the neovim repo about using metatables to do something similar but I'm not sure how that would work with the way we do global variables.

I feel like maybe I'm missing something obvious. @fredizzimo do you know of another way to get global variable notifications?

@Kethku
Copy link
Member Author

Kethku commented Nov 11, 2023

Aight @9mm let me know if the current set of changes works for you.

@fredizzimo I ended up creating a global function via an vimscript exec and then calling that to work around not being able to pass the g: dictionary from the lua side. I think its possible theres a better way to do this but thats the best I've got for now. Other than that I addressed your other concerns. Should be good to go I think.

@9mm
Copy link
Contributor

9mm commented Nov 11, 2023

@Kethku that fixed it!

@Kethku Kethku merged commit b79b7cc into main Nov 11, 2023
19 checks passed
@Kethku Kethku deleted the fix-remember-window-size branch May 11, 2024 23:41
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

3 participants