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: revert 2debe82 #94

Merged
merged 1 commit into from
Sep 29, 2023
Merged

Conversation

willothy
Copy link
Collaborator

fixes #93

@Bekaboo
Copy link
Owner

Bekaboo commented Sep 27, 2023

@willothy scrollbar should be updated only on WinScrolled now

@willothy
Copy link
Collaborator Author

willothy commented Sep 27, 2023

That's not working properly, see #93

Scrollbar updates are seemlingly random

Before fix:

2023-09-26.21-52-28.mp4

After fix:

Scrollbar not reaching bottom of the menu as seen in this vid is addressed in #95.

2023-09-26.21-54-36.mp4

@Bekaboo
Copy link
Owner

Bekaboo commented Sep 27, 2023

@willothy Could you send the file ui.lua to me? I cannot reproduce the bug on my end.

@willothy
Copy link
Collaborator Author

willothy commented Sep 27, 2023

@willothy Could you send the file ui.lua to me? I cannot reproduce the bug on my end.

it's just a huge list of Lazy plugins, I can paste it here if you need but you could recreate the same effect by copy-pasting a bunch of plugin entries.

return {
  { -- copy paste this a bunch
      "fake plugin"  
  }
}

@Bekaboo
Copy link
Owner

Bekaboo commented Sep 27, 2023

So I guess the bug appears when the number of buffer lines exceed some threshold? Could you share how many entries are there in your menu?

@willothy
Copy link
Collaborator Author

willothy commented Sep 27, 2023

I don't think that WinScrolled autocmd is executing very often as it's marked Buffer local... it works fine when not buffer local. Happens on menus of all sizes for me, not just large ones.

You're right that it's only happening at certain heights, but I can't figure out what the threshold is.

There are 29 entries in that file.

I can reproduce by selecting from the top-level symbols in dropbar/menu.lua, you could try that.

@Bekaboo
Copy link
Owner

Bekaboo commented Sep 27, 2023

Emm.. Cannot reproduce with 100 menu entries :(

scrollbar.mp4

@willothy
Copy link
Collaborator Author

Strange, what version of Neovim are you using? I'm on the latest nightly

@Bekaboo
Copy link
Owner

Bekaboo commented Sep 27, 2023

@willothy Nightly but not the latest version

NVIM v0.10.0-dev-1181+ge353c869ce
Build type: RelWithDebInfo
LuaJIT 2.1.1694285958
Compilation: /usr/bin/cc -O2 -g -Og -g -Wall -Wextra -pedantic -Wno-unused-parameter -Wstrict-prototypes -std=gnu99 -Wshadow -Wconversion -Wvla -Wdouble-promotion -Wmissing-noreturn -Wmissing-format-attribute -Wmissing-prototypes -fno-common -Wno-unused-result -Wimplicit-fallthrough -fdiagnostics-color=auto -fstack-protector-strong -DUNIT_TESTING -DINCLUDE_GENERATED_DECLARATIONS -D_GNU_SOURCE -I/usr/include/luajit-2.1 -I/usr/include -I/build/neovim-git/src/build/src/nvim/auto -I/build/neovim-git/src/build/include -I/build/neovim-git/src/build/cmake.config -I/build/neovim-git/src/neovim-git/src

   system vimrc file: "$VIM/sysinit.vim"
  fall-back for $VIM: "/usr/share/nvim"

Run :checkhealth for more info

@willothy
Copy link
Collaborator Author

willothy commented Sep 27, 2023

Is that before or after this PR? This PR affected the way topline is restored when setting buf lines, and broke scrolling for some plugins. Testing this now.

Edit: nope, that's not the issue.

@willothy
Copy link
Collaborator Author

Is updating on CursorMoved a problem? The update_scrollbar function isn't especially complex so I doubt there will be any noticeable performance impact.

@Bekaboo
Copy link
Owner

Bekaboo commented Sep 27, 2023

Is updating on CursorMoved a problem?

Technically not a problem... But you know, we want to reduce unnecessary calls and make the code clean. I'll test with the lastet nightly version of neovim later.

@willothy
Copy link
Collaborator Author

Is updating on CursorMoved a problem?

Technically not a problem... But you know, we want to reduce unnecessary calls and make the code clean. I'll test with the lastet nightly version of neovim later.

For sure, sounds good. I originally tried this same thing when implementing the scrollbar but ran into this issue, that's why I had the call in CursorMoved in the first place. If another solution isn't found, maybe it would be good to keep the call in, until there's a cleaner fix? I've tested on a few older 0.10 versions as well and still have the issue, so I'm not sure what's going on.

@willothy willothy force-pushed the fix-scrollbar-update branch from 1b74ce0 to be7d379 Compare September 27, 2023 06:12
@Bekaboo
Copy link
Owner

Bekaboo commented Sep 27, 2023

@willothy Still cannot reproduce with minimal config with the latest neovim version:

scrollbar_repro_clean.mp4

Could you try reproducing it using nvim --clean instead of using your whole config?

@willothy
Copy link
Collaborator Author

willothy commented Sep 27, 2023

Yes, I can reproduce with nvim --clean and the minimal init provided, using the ui.lua file I showed.

2023-09-27.14-13-00.mp4

Sometimes the cursor scrolls down properly, but doesn't scroll up as well. The timing of WinScrolled seems to be very inconsistent overall.

@Bekaboo Bekaboo merged commit 7a91b7b into Bekaboo:master Sep 29, 2023
4 checks passed
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.

[Bug]: Scrollbar doesn't consistently update
2 participants