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(tui): reset clear_region attributes during startup #28713

Merged
merged 1 commit into from
May 26, 2024

Conversation

luukvbaal
Copy link
Contributor

@luukvbaal luukvbaal commented May 12, 2024

Problem: Fix added in #28676 worked accidentally(used variables were
themselves uninitialized at this point during startup) and
does not always work.
Solution: Reset attributes when clearing regions during startup.

@github-actions github-actions bot added the tui label May 12, 2024
src/nvim/tui/tui.c Outdated Show resolved Hide resolved
test/functional/terminal/tui_spec.lua Show resolved Hide resolved
src/nvim/tui/tui.c Outdated Show resolved Hide resolved
@luukvbaal luukvbaal marked this pull request as draft May 12, 2024 14:52
@luukvbaal luukvbaal changed the title fix(tui): intentionally initialize clear_attrs to -1 fix(tui): reset clear_region attributes during startup May 12, 2024
@luukvbaal luukvbaal marked this pull request as ready for review May 13, 2024 00:03
@github-actions github-actions bot requested a review from gpanders May 13, 2024 00:04
Problem:  Fix added in neovim#28676 worked accidentally(used variables were
          themselves uninitialized at this point during startup) and
          does not always work.
Solution: Reset attributes when clearing regions during startup.
@gpanders
Copy link
Member

@luukvbaal IIUC the current behavior on master is fine as-is and this PR fixes an issue when a redraw is performed in _defaults.lua? So kicking this to the 0.11 milestone should be ok?

@gpanders gpanders modified the milestones: 0.10, 0.11 May 13, 2024
@luukvbaal
Copy link
Contributor Author

Flush, not redraw. But yeah for reference: I noticed the test repro still producing a black screen on my ext_ui branch, which calls vim.ui_attach()->ui_flush() at the top of _defaults.lua. Adding any flush before the tty shenannigans in _defaults.lua may still produce a black screen(don't ask me why).

That isn't to say that I know for sure that that is the only way to still produce a black screen during start up. So in that sense this fix seems safer but if no one dares press merge before 0.10, after works too.

@gpanders
Copy link
Member

(don't ask me why)

This is really the root of my hesitation to merge this pre 0.10. If it doesn't fix any user facing bugs (that we know of) in master then it should be safe to slow down and find a root cause. I'd like to understand this too, but I don't have the bandwidth to chase it down in the next day or two, so my preference is to kick the can to the 0.11 release cycle.

@luukvbaal
Copy link
Contributor Author

Yeah fair. I kind of lost interest in debugging the behavior on master further once I found this alternative solution and established that #28676 made incorrect assumptions.

Figuring out why trying to force clear_region()->update_attrs() to use the default background by initializing to 0/-1(which it turned #28676 actually amounted to), may still be interesting but I'm not sure how useful that actually still is give it's accidental nature. The updated test output(actual blank screen vs some attr) also seems more reliable.

@justinmk justinmk merged commit bc63ffc into neovim:master May 26, 2024
28 checks passed
@github-actions github-actions bot removed the request for review from gpanders May 26, 2024 17:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants