Add Modified highlight group

[evanpurkhiser]

This is similar to the change made in #36 however the requirement of the the indicators option being enabled is not present in this change, and the precedence is different.

I think this should line up more with @ap's thinking on how this should work.

[ap]

Hi, thanks for taking the time to do this. The reason #36 never went anywhere is that @wmonk never replied about whether he had a specific reason to pick the precedence order he chose (and if so, what it was). I still wish I knew, but I guess no answer can be expected now.

I think this should line up more with @ap's thinking on how this should work.

I’m afraid not… sorry. 😕 It just limits the Modified highlight so a buffer will never be highlighted that way unless it’s the Current buffer. So it is sort of the inverse of #36: in order to get a colour indication of whether a buffer is modified, you have to first navigate to it. Is that in actual fact how you want it to work…? I’m trying to understand. So far out of two patches I’ve gotten for this, both of them seem obviously bizarre to me (in completely opposite ways)… am I the weird one?

Maybe Current/Active/Hidden should stay as they are and instead be augmented by a set of ModifiedCurrent/ModifiedActive/ModifiedHidden highlight groups? But of course then I start to wonder about the combinatorial explosion that would result from trying to support other buffer states in this way. And then of course I wonder if my concern is unnecessary – after all, indicators has never been expanded to display any other state than the modified flag.

Hmm.

[evanpurkhiser]

I’m afraid not… sorry

That's okay! Hopefully I can explain my reasoning.

in order to get a colour indication of whether a buffer is modified, you have to first navigate to it. Is that in actual fact how you want it to work…?

Yes, this is indeed the functionality that I would prefer. Like you said previously, I want to know what buffers are active in a window over which ones are modified. I can enable the indicators to see that it is modified if it's not the current, but when I am focusing on a window, I would like a more distinct visual indication that I've made changes.

This is also how the buffer line extension in vim-airline works.

combinatorial explosion that would result from trying to support other buffer states in this way

Personally I think it would be good to simply support the simplest version of this functionality first, and if users ask for more configuration then consider this.

[ap]

Personally I think it would be good to simply support the simplest version of this functionality first, and if users ask for more configuration then consider this.

The thing is… effectively users already have. You explained that you do, in fact, want it working the way it works according to your patch. But what you want is substantially different from what @wmonk apparently wants… and, as I said, both of these are substantially different from how I would expect the feature to work. That seems like a strong first-approximation indicator that there will be several distinct camps among users of this feature. So I’m not feeling inclined to implement an approach that will work only for one of us. (I’m not happy that this means I’m currently implementing nothing, which works for none of the potential users… but…)

[evanpurkhiser]

The thing is… effectively users already have

In that case, someone may submit a patch that supports this case and another case, where the review for an elegant design can happen then right? For now I would argue that this feature is good enough. Especially considering the vim-airline buffer line extension does the same thing.

[ap]

someone may submit a patch that supports this case and another case, where the review for an elegant design can happen then right?

Feel free to do the honours! Start from the patch in #36 and make a change to it to support the way you want this feature to work. If you want to accommodate how I thought it should work, then even better. 😊 Then we can take a look at how well it works.

Personally I consider it a failure on my part to leave single-feature requestors to figure out how to integrate the feature they want with all the other features the code base already implements. Particular if every feature is designed in isolation. Inevitably their own patch is not going to accommodate anything much more than the one particular thing they needed. Leaving the next requestor to figure out how to fit their feature into that combination. And so on…

For now I would argue that this feature is good enough.

Which one? Yours, #36, my initial proposal, or my revised unified proposal? If “good enough” is the bar then it’ll be my good enough, i.e. my initial proposal. Will that good enough work for you?

Especially considering the vim-airline buffer line extension does the same thing.

Huh. Let’s see…

https://github.com/vim-airline/vim-airline/blob/a0298263b7fd55827839862ffd3a8d5b2a787a5c/autoload/airline/extensions/tabline.vim#L184-L202

Interesting. Looks like it implements my revised proposal: there is airline_tabmod and airline_tabsel for the current buffer, and airline_tabmod_unsel vs airline_tab for non-current buffers, i.e. there is a highlight group for every combination of selection-state × modified-state of a buffer. (Except that Airline apparently doesn’t have a “visible in some window but not the focussed window” selection state.)

Thanks for the pointer! That at least reassures me that if it’s a mistake to multiply out the highlight groups like that, at least I won’t be the only one who made that mistake. 😊 (Also, another thing I retrospectively find Airline having beaten me to… grumble grumble. 😊)

[evanpurkhiser]

then it’ll be my good enough, i.e. my initial proposal. Will that good enough work for you?

In that case I think I will continue to use my fork. Unfortunately I don't have too much spare time to work on this right now.

I understand though. Thank you for your time @ap

[ap]

No worries. I will be implementing the revised / same-as-Airline proposal soon, which will be possible to configure like you prefer, at which point you’ll be able to revert to upstream instead of maintaining a fork (if you choose to).