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

Allow user to specify zindex so that some floating window is treated as normal #2408

Closed
wants to merge 4 commits into from

Conversation

xzbdmw
Copy link

@xzbdmw xzbdmw commented Mar 7, 2024

What kind of change does this PR introduce?

截屏2024-03-08 06 03 15 On the left is the normal multi grid, and on the right is the `vim.g.neovide_no_multigrid_zindex=20` that I set to make it completely transparent(no shadow no winblend no background color) and no longer appear like a glass

I don't know if this feature is good or bad, It is my first real code pr in github :)

Did this PR introduce a breaking change?

  • No

introduce a new setting no_multigrid_zindex
Add new setting
Allow full transparency behaves like multigrid disabled
Copy link
Contributor

@MultisampledNight MultisampledNight left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds very useful to me, thank you!!

I'm not aware of any API in Neovim available to control this, and there are uses indeed where having a plugin's floating window be treated as being part of the main grid would be desirable.

@@ -47,6 +47,7 @@ pub struct RendererSettings {
floating_blur_amount_y: f32,
floating_shadow: bool,
floating_z_height: f32,
no_multigrid_zindex: u64,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since a user might want to have this apply to multiple zindices, how about making this a list of indices instead?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe it would make sense to say that all zindex values below the configured one will be treated as flat?

Tbh I kinda think a name like flatten_floating_windows_below_zindex or something more concise but in that direction might help communicate what the setting actually does. Multigrid is a feature for neovim guis that might not make sense to end users.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe it would make sense to say that all zindex values below the configured one will be treated as flat?

Tbh I kinda think a name like flatten_floating_windows_below_zindex or something more concise but in that direction might help communicate what the setting actually does. Multigrid is a feature for neovim guis that might not make sense to end users.

There is a usage scenario where a floating window can be used as a preview and can do some editing. Generally, the zindex of this type of window is set very low.
For example, I set glance.nvim's zindex to 10 and treesitter-context's zindex to 20, and keep glance's preview's shadow like this:
image
That being said, a specified zindex would allow this kind of customization.

Comment on lines 369 to 370
if self.anchor_info.is_some()
&& self.anchor_info.as_ref().unwrap().sort_order != settings.no_multigrid_zindex
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might want to combine these two? As in e.g.:

Suggested change
if self.anchor_info.is_some()
&& self.anchor_info.as_ref().unwrap().sort_order != settings.no_multigrid_zindex
if self.anchor_info
.as_ref()
.map_or(false, |info| info.sort_order != settings.no_multigrid_zindex)

@MultisampledNight
Copy link
Contributor

(fwiw you can just run cargo fmt and commit to make the rustfmt lint stop complaining)

@MultisampledNight
Copy link
Contributor

Sidenote for future PRs (no biggie here, really just for the future): It is generally preferred to make a new branch on your fork for each new PR. This allows both

  1. parallel work on your fork, if you'd happen to have any other interesting things to tackle
  2. easier testing locally, since now the tester can just do git switch your-branch instead of doing git checkout -b xzbdmw-main xzbdmw/main (thus having to watch out where to push on changes)

Optionally see #1468 (comment) for further details.

@xzbdmw
Copy link
Author

xzbdmw commented Mar 23, 2024

Sidenote for future PRs (no biggie here, really just for the future): It is generally preferred to make a new branch on your fork for each new PR. This allows both

  1. parallel work on your fork, if you'd happen to have any other interesting things to tackle
  2. easier testing locally, since now the tester can just do git switch your-branch instead of doing git checkout -b xzbdmw-main xzbdmw/main (thus having to watch out where to push on changes)

Optionally see #1468 (comment) for further details.

Very detailed explaination! I changed the name to flatten_floating_zindex as @Kethku suggested, and make it a comma separated list such as `vim.g.neovide_flatten_floating_zindex = "20,30,40" which is the same way font is configured.

@fredizzimo
Copy link
Member

fredizzimo commented Mar 23, 2024

Before merging this, I would like to see another approach being tried, which I think fixes this issue and also these

Currently we draw each floating window separately, sorting by z-index, then by x position, and finally by y position. With each window blending together with everything being drawn so far. Especially the sort by x and y position is quite random and arbitrary, not much better than completely random.

We should instead change the drawing to be done by z-index layers, to draw the complete full layer before blending with the lower ones and applying shadows to the combined outline. That way as long as the windows have the same z-index they will be treated as one. Some plugins might have to be changed to support that, but it should always be possible to do so, without breaking the regular terminal rendering.

There are two special cases that needs to be handled, since the windows with the same z-index can have conflicting contents.

  1. The windows draw something with the foreground color to the same cell. This is really a bug in either the user configuration or the plugin. But perhaps the most logical thing for Neovide to do in this situation to draw the text on top of each other (in random order). In practice this should not be a problem, since properly made plugins will only have text in one of the overlapping windows.
  2. The windows draws with different colors or blend factors. To support the telescope case, render the background with the lowest blend factor (the least transparent) one. If there are still conflicts, randomly by deterministically, maybe by the x/y sort, select one of them.

NOTE: That this only solves a subset of what this PR attempts to do. But I'm not sure if configuring a z-index, is the best way to disable the shadows from a subset of the windows. I think we need something more configurable that the theme and plugins can control. Maybe some additional windows settings in Neovim itself or something else.

It also does not solve this issue #1676, which I think only telescope can fix.

Would either you @xzbdmw, or @TwIStOy, be able to work on that?

@TwIStOy
Copy link
Contributor

TwIStOy commented Mar 23, 2024

That's a great idea! I will take a look and try to implement it on Monday.

@xzbdmw
Copy link
Author

xzbdmw commented Mar 23, 2024

If I understand correctly, this approach mainly solves the problem of multiple overlapping(or adjacent) floating windows, and there will still be a combined shadow (if they have the same zindex), I wander these two features can coexisting together(better rendering and don't render at all).

NOTE: That this only solves a subset of what this PR attempts to do. But I'm not sure if configuring a z-index, is the best way to disable the shadows from a subset of the windows. I think we need something more configurable that the theme and plugins can control. Maybe some additional windows settings in Neovim itself or something else.

This method is straightforward(intuitive?), both in terms of code and configuration, allowing users who are concerned with these shadows to disable them easily. Typically, only a few plugins would require adjustments, waiting a setting option from Neovim is also a viable path:)

@TwIStOy
Copy link
Contributor

TwIStOy commented Mar 24, 2024

IMO, disables the shadow and blur of a specific z-index window seems somewhat unusual. I notice that all floating window related ui-events contains a win field. Therefore could we use the win field to decide whether to apply shadow and blur to that floating window? This also solves an issue with another plugin colorful-winsep, which uses floating windows to create these colorful separators.

@xzbdmw
Copy link
Author

xzbdmw commented Mar 24, 2024

Hmm, https://github.com/nvim-zh/colorful-winsep.nvim can already be disabled by setting zindex,
before :
image
after:
截屏2024-03-24 16 47 29

disables the shadow and blur of a specific z-index window seems somewhat unusual

because it is the only "unique" property of a floating window

Therefore could we use the win field to decide whether to apply shadow and blur to that floating window?

Hmm how could a user configure that win field to say they don't want shadow from the specified plugin?

Note: the shadow is too distractive in light background, but in dark background it feels ok though(most users are using dark background).

@TwIStOy
Copy link
Contributor

TwIStOy commented Mar 24, 2024

because it is the only "unique" property of a floating window

However, z-index is not really the unique property. Currently, there are a lot of floating windows using the same z-index. Thus, I question whether using the z-index might inadvertently disable other windows that should have shadow and blur.

Hmm how could a user configure that win field to say they don't want shadow from the specified plugin?

Obviously, the buffers in those special windows corresponded to specific filetypes. So, I think a simple autocmd is enough.

@xzbdmw
Copy link
Author

xzbdmw commented Mar 24, 2024

Seems like this suggestion #2375 (comment)

but it is not possible to disable shadow for specified window

This also solves an issue with another plugin colorful-winsep

Could you show a working examples using autocmd? Because when a WinEnter or something happends, if you disable these shadows, the entire window will have no shadow, say, cmp's completion menue shadow.

Thus, I question whether using the z-index might inadvertently disable other windows that should have shadow and blur

Yes, but you can adjust the zindex through configuration to make it unique, as I said above, only a few plugins suffer from this, they all have zindex configurable, and plugin authors will make this kind of floating window a defualt zindex value lower than usual(50), so the chance of conflict is low. In case zindex is not configurable, I make flatten_floating_zindex a list of zindex.

@fredizzimo
Copy link
Member

We definitely want to allow disabling the shadows from specific windows, as I already mentioned in my previous message.

But I don't think that using the z-index is the best way, unless we can create a global standard that is followed across the Neovim community. But even then, it has limitations, like you might want to disable the shadows for some window outside that range.

I can think of two solutions, which are basically the same, but specified in different places. That would be to add a shadow_type configuration option to either the highlight group or the floating window configuration. A similar option could be added for the background blur. This also allows us to implement different types of shadows and blurs in the future and have everything absolutely configurable. Of course, it also requires some kind of standardization, but that could happen more incrementally, and mostly implemented in the color schemes, where this kind of stuff belongs IMO.

This needs support on the Neovim side though, but I'm planning to add a PR that adds support for arbitrary arguments to the highlight groups, which are passed to GUI implementations. This allows us to experiment without updating Neovim, and when things stabilize, standardize those arguments, and make them official. This would also allow us to fix the transparency issue we have, since we need more expressiveness than the default highlight groups can provide.

@TwIStOy
Copy link
Contributor

TwIStOy commented Mar 26, 2024

Could you show a working examples using autocmd? Because when a WinEnter or something happends, if you disable these shadows, the entire window will have no shadow, say, cmp's completion menue shadow.

Ohhh, It's my mistake. Upon reviewing the content of ui-events, I found that win in ui-events differs from win retrieved from vim.api. Thus, implement it using autocmd might not be feasible.

@Kethku
Copy link
Member

Kethku commented Apr 23, 2024

Im sorta inclined to close this. The "render same z-index together" PR solves a part of this problem by rendering sequential zindex floating windows together.

I fully agree that we need a way to configure how windows are rendered on a window by window basis but this PR doesn't really solve this in my mind in a way thats approachable to end users. My impression is that the Z-Index is not a thing that most users think about.

I'm going to close for now, but feel free to reopen if more discussion is needed

@Kethku Kethku closed this Apr 23, 2024
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

5 participants