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

feat: draw block characters with background opacity #2478

Merged
merged 1 commit into from
Apr 18, 2024

Conversation

Theaninova
Copy link
Contributor

@Theaninova Theaninova commented Apr 10, 2024

What kind of change does this PR introduce?

This fixes #2275 by letting box drawing, block elements, geometric shapes as well as the Nerdfont Powerline (+Extras) inherit the background opacity instead.

  • (Fix?)
  • Feature

Did this PR introduce a breaking change?

A breaking change includes anything that breaks backwards compatibility either at compile or run time.

  • No
Main This PR
image image

@Theaninova Theaninova force-pushed the fix-block-drawing-transparency branch 2 times, most recently from 865f494 to e20b245 Compare April 10, 2024 13:35
@fredizzimo
Copy link
Member

fredizzimo commented Apr 15, 2024

Thank you!

I think this is good for a quick workaround before we have a long-term proper solution like this or an alternative:

But it could also cause problems for other UI elements, there might be some UI element that is supposed to be opaque, like a border, which now becomes transparent with these changes. So, it's a two-edged sword.

But in any case, would you mind, rebasing on top of main, so that we get proper CI checks for this? After that I will discuss this with the other maintainers and decide what to do.

@Theaninova Theaninova force-pushed the fix-block-drawing-transparency branch from e20b245 to 1d66107 Compare April 15, 2024 19:08
@fredizzimo
Copy link
Member

Some of the clippy warnings will be fixed by:

But the

RangeInclusive::contains implementation is caused by these changes.

As a sidenote, we need to improve the reporting of the clippy warnings:
they are not very readable currently, but you should get a better message locally if you run cargo clippy

@Theaninova Theaninova force-pushed the fix-block-drawing-transparency branch from 1d66107 to 3914c53 Compare April 15, 2024 20:57
@Theaninova Theaninova force-pushed the fix-block-drawing-transparency branch from 3914c53 to cfc9b09 Compare April 16, 2024 12:24
@Theaninova
Copy link
Contributor Author

Sorry about that, forgot to include rustfmt in my shell.nix

Copy link

Test Results

  6 files  ±0    6 suites  ±0   19s ⏱️ -1s
107 tests ±0  107 ✅ ±0  0 💤 ±0  0 ❌ ±0 
630 runs  ±0  630 ✅ ±0  0 💤 ±0  0 ❌ ±0 

Results for commit cfc9b09. ± Comparison against base commit 41622dc.

@Kethku Kethku merged commit f9848f0 into neovide:main Apr 18, 2024
12 checks passed
@Kethku
Copy link
Member

Kethku commented Apr 18, 2024

This looks great! Thanks for the fix. @fredizzimo this is a small enough change that I'm merging it for now and we can revert it if and when the longer term fix gets implemented. In the meantime, I will do some testing to see how rough the issues you mentioned might be

falcucci added a commit that referenced this pull request Apr 18, 2024
- remove the `is_box_drawing_character` function and its usage
- refactor the paint blending logic in the `gridrenderer` struct
@falcucci
Copy link
Member

@Theaninova first of all, thank you for the contribution!

I had to revert this change in specific due to some issues to render the floating window borders. I'll share an example where the border disappear at top and the bottom:

CleanShot 2024-04-19 at 01 01 06@2x

and this one it's how it should be rendered:

CleanShot 2024-04-19 at 00 47 18@2x

I would be happy to help you with any further information to bring the fix.

@Theaninova
Copy link
Contributor Author

I was debating in my head if it would be reasonable to include the box drawing set

Box Drawing
  0 1 2 3 4 5 6 7 8 9 A B C D E F
U+250x
U+251x
U+252x
U+253x
U+254x
U+255x
U+256x
U+257x

In hindsight, that was probably a bad idea (who could have guessed). Removing that range should do the trick.

I also just realized that I messed up the ranges when fixing the clippy warnings, all of them should be inclusive

fn is_box_drawing_character(c: char) -> bool {
    // block elements
    ('\u{2580}'..='\u{259F}').contains(&c)
        // geometric shapes
        || ('\u{25A0}'..='\u{25FF}').contains(&c)
        // nerdfont powerline
        || ('\u{e0a0}'..='\u{e0a2}').contains(&c)
        || ('\u{e0b0}'..='\u{e0b3}').contains(&c)
        // nerdfont powerline extras
        || (c == '\u{e0a3}')
        || ('\u{e0b4}'..='\u{e0c8}').contains(&c)
        || (c == '\u{e0ca}')
        || ('\u{e0cc}'..='\u{e0d7}').contains(&c)
}

@Kethku
Copy link
Member

Kethku commented Apr 19, 2024

This work is pretty related to #2491 which was opened just recently.

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.

Lualine background transparency
4 participants