gruvbox-community / gruvbox

Retro groove color scheme for Vim - community maintained edition
MIT License
801 stars 62 forks source link

Add airline_term section #108

Closed efnull closed 5 years ago

efnull commented 5 years ago

Closes: #6

efnull commented 5 years ago

Actually, I just figured out that the changes on 9d94fcd conflicts with 58b226c.

I could introduce another s: variable to keep the behavior of 58b226c if that's preferable, but I think that would add an unnecessary complexity.

I'm using a macOS at home and have no experience with Urxvt terminal, so I'm not really sure if there is a work around for this. Do you have any suggestions @rbong?

rbong commented 5 years ago

How are the current color mappings broken?

efnull commented 5 years ago

Sorry I investigated the problem a little more and it turned out that the colour mappings weren't the problem.

On macOS, iterm2, Vim 8.1 with following .vim directory structure:

.vim/pack/a/start/gruvbox
.vim/pack/b/start/vim-airline

minimum vimrc below:

set termguicolors
set background=dark
colorscheme gruvbox

and only 9041122 of this pull request applied.

When I open vim and type in the command :terminal I get the following Screen Shot 2019-06-29 at 16 18 53

After <C-w>j, <C-w>k to move to the split below and then back to the terminal split Screen Shot 2019-06-29 at 16 18 58

The results were the same with 9d94fcd applied and not applied, so the colour mappings weren't wrong, but it's not showing correct colours until I de-select the terminal split once.

Perhaps this is a problem with airline itself? Maybe I'll have to take a deeper look into the vim-airline's code base to find out the cause.

efnull commented 5 years ago

I have found that commenting out all the assignment to <mode>_modified pallet like the following

let g:airline#themes#gruvbox#palette.normal_modified = { 'airline_c': modified_group }

fixes the issue.


The assignment to modified_group is done through following lines

  let M0 = airline#themes#get_highlight('Identifier')
  let modified_group = [M0[0], '', M0[2], '', '']

Result of the airline#themes#get_highlight('Identifier') call in my environment results in

let M0 = airline#themes#get_highlight('Identifier')
" M0 = ['#83a598', '#282828', '', '', '']

Which means the modified_group will be

let modified_group = [M0[0], '', M0[2], '', '']
" modified_group = ['#83a598', '', '', '', '']

Below is a snippet from airline's default dark theme which comments about modified palette's behavior.

" Here we define overrides for when the buffer is modified.  This will be
" applied after g:airline#themes#dark#palette.normal, hence why only certain keys are
" declared.
let g:airline#themes#dark#palette.normal_modified = {
      \ 'airline_c': [ '#ffffff' , '#5f005f' , 255     , 53      , ''     ] ,
      \ }

Missing values within the modified_group dictionary shouldn't matter, because the modified_group should only be overriding the default normal pallet if it has an assigned value. It shouldn't be causing any issues.

I have checked other airline themes which has terminal mode colours done properly, but none of them seems to map anything to <mode>_modified mappings.

I cannot find out why <mode>_modified mappings messes up airline_term mappings and I don't think I can investigate any further.

efnull commented 5 years ago

It seems that I was heading towards the wrong direction all along.

I simply had to add modified palette to each of the modes. Just like how airline_warning and airline_error sections are done.

I have tested on vim 8.1 and neovim 0.3.7 both with and without set termguicolors and they both work properly.

Here's the final result! ea0c91e

After :terminal<CR>: Screen Shot 2019-07-01 at 21 20 55

After <C-w>j: Screen Shot 2019-07-01 at 21 21 04

I have reverted all the wrong commits, and then added a new commit with the final fix ea0c91e. Sorry for the mess on the pull request's commit history.

efnull commented 5 years ago

I guess there's no interest to this, so I will close this pull request.

rbong commented 5 years ago

I'm sorry, work has been busy this past month. I'd like to remind everyone that I'm looking for additional maintainers.

efnull commented 5 years ago

Hey @rbong sorry I thought you were hoping for a different approach to solve this problem. If you are busy with your work, I'm happy to help. Just ping me if needed.

phddoom commented 5 years ago

@rbong Is there any testing or other help I can provide to get this fix merged or updated?

rbong commented 5 years ago

@phddoom @efnull I just need this pull request reopened or resubmitted, I can't merge it as-is so I haven't reviewed it yet.