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

[RFC] vim-patch:8.0.0142 #7335

Merged
merged 3 commits into from
Oct 7, 2017
Merged

[RFC] vim-patch:8.0.0142 #7335

merged 3 commits into from
Oct 7, 2017

Conversation

ckelsel
Copy link
Contributor

@ckelsel ckelsel commented Sep 28, 2017

Problem: Normal colors are wrong with 'termguicolors'.
Solution: Initialize to INVALCOLOR instead of zero. (Ben Jackson, closes
vim/vim#1344)

vim/vim@0cdb72a

Problem:    Normal colors are wrong with 'termguicolors'.
Solution:   Initialize to INVALCOLOR instead of zero. (Ben Jackson, closes
            vim/vim#1344)

vim/vim@0cdb72a
@marvim marvim added the RDY label Sep 28, 2017
@justinmk
Copy link
Member

Can someone test this?

@KillTheMule
Copy link
Contributor

Steps are laid out in vim/vim#1343, I'd give it a shot if no one else wants to.

@teto
Copy link
Member

teto commented Sep 29, 2017

I've had a bunch of similar error I fixed in my hilights-related PR ; #7082 and subsequent. It might or not fix this already.

@KillTheMule
Copy link
Contributor

KillTheMule commented Sep 29, 2017

I can't reproduce what's described in vim/vim#1343 (comment), neither in vim (I downloaded 8.0.0141 for the test), nor in nvim. Also the corresponding test does not show a change when run on master vs. run with this PR. You can see the test at KillTheMule@01fcdbd.

Not sure how to proceed when I can't reproduce the issue.

(e) Ok I could reproduce with vim 8.0.0134, but still not with nvim.

@jamessan
Copy link
Member

Ok I could reproduce with vim 8.0.0134, but still not with nvim.

Yes, nvim behaves differently than vim. It behaves like gvim does in this case. However, IMHO, both nvim and gvim are wrong. They show (bold?) white text on a white background. The background should be black.

@KillTheMule
Copy link
Contributor

KillTheMule commented Sep 29, 2017

It's not black, it's grey right? That's the cursorline, if I'm not mistaken. It's a different grey than in vim though.

@jamessan
Copy link
Member

It's not black, it's grey right?

Ah, you're right. nvim and gvim set guibg to Grey90 for CursorLine, while vim sets it to Grey40.

@jamessan
Copy link
Member

nvim and gvim set guibg to Grey90 for CursorLine, while vim sets it to Grey40.

Which is due to differences in the value for 'background'. background=dark sets Grey40, background=light sets Grey90. gvim, and apparently nvim, default to light and vim defaults to dark.

@@ -6827,6 +6827,9 @@ int hl_combine_attr(int char_attr, int prim_attr)
new_en = *char_aep;
} else {
memset(&new_en, 0, sizeof(new_en));
new_en.rgb_fg_color = -1;
Copy link
Member

Choose a reason for hiding this comment

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

Alternatively, maybe this entire branch can be removed? new_en already has the correct ATTRENTRY_INIT value before the memset().

@justinmk justinmk changed the title [RDY] vim-patch:8.0.0142 [RFC] vim-patch:8.0.0142 Oct 7, 2017
@justinmk justinmk added needs:response waiting for reply from the author and removed needs:response waiting for reply from the author labels Oct 7, 2017
Copy link
Member

@justinmk justinmk left a comment

Choose a reason for hiding this comment

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

see comments

@marvim marvim added RFC and removed RDY labels Oct 7, 2017
Problem:    Normal colors are wrong with 'termguicolors'.
Solution:   Initialize to INVALCOLOR instead of zero. (Ben Jackson, closes
            vim/vim#1344)

vim/vim@0cdb72a
@ckelsel
Copy link
Contributor Author

ckelsel commented Oct 7, 2017

Fixed.

@justinmk justinmk merged commit e3ca1e6 into neovim:master Oct 7, 2017
@justinmk justinmk removed the RFC label Oct 7, 2017
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.

7 participants