[vim/vim] enhance TermResponse to carry the attribute queried (PR #13829)
Danek Duvall
I wanted to be able to set the colorscheme based on the terminal colors (and adapt to terminal color changes), but there was no mechanism where v:termrbgresp was reliably correctly set. This enhances TermResponse to deliver the type of response it got so that a handler can pick it apart and make decisions based on it.
The only real problem with the concept is that if you want to make decisions based on both foreground and background, there's no way to know if both are set correctly. I'm not sure what a mechanism that provided that would even look like.
fgbg_event is probably wrong, but I needed something to check with has().
You can view, comment on, or merge this pull request online at:
https://github.com/vim/vim/pull/13829
Commit Summary
- a4938f3 enhance TermResponse to carry the attribute queried
File Changes
(6 files)
- M runtime/doc/autocmd.txt (15)
- M runtime/doc/eval.txt (26)
- M runtime/doc/todo.txt (3)
- M src/autocmd.c (6)
- M src/evalfunc.c (1)
- M src/term.c (10)
Patch Links:
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
Gary Johnson
> I wanted to be able to set the colorscheme based on the terminal colors (and
> adapt to terminal color changes), but there was no mechanism where
> v:termrbgresp was reliably correctly set. This enhances TermResponse to deliver
> the type of response it got so that a handler can pick it apart and make
> decisions based on it.
>
> The only real problem with the concept is that if you want to make decisions
> based on both foreground and background, there's no way to know if both are set
> correctly. I'm not sure what a mechanism that provided that would even look
> like.
>
> fgbg_event is probably wrong, but I needed something to check with has().
:autocmd TermResponse * ...
to wait for just the response to t_RV, I will now have to use
:autocmd TermResponse ver ...
If so, that may mess up a plugin expecting the former to respond
only to the version. Other than that, this seems like a nice
feature. Maybe change the event name?
Regards,
Gary
vim-dev ML
> adapt to terminal color changes), but there was no mechanism where
> v:termrbgresp was reliably correctly set. This enhances TermResponse to deliver
> the type of response it got so that a handler can pick it apart and make
> decisions based on it.
>
> The only real problem with the concept is that if you want to make decisions
> based on both foreground and background, there's no way to know if both are set
> correctly. I'm not sure what a mechanism that provided that would even look
> like.
>
> fgbg_event is probably wrong, but I needed something to check with has().
:autocmd TermResponse * ...
to wait for just the response to t_RV, I will now have to use
:autocmd TermResponse ver ...
If so, that may mess up a plugin expecting the former to respond
only to the version. Other than that, this seems like a nice
feature. Maybe change the event name?
Regards,
Gary
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
Danek Duvall
So, as I understand your change, where I now use :autocmd TermResponse * ... to wait for just the response to t_RV, I will now have to use :autocmd TermResponse ver ... If so, that may mess up a plugin expecting the former to respond only to the version. Other than that, this seems like a nice feature. Maybe change the event name?
Hm, I suppose. But I think the only real consequence is that its handler will now get called more often (at least if it's installed early enough; I'm not sure how often this will trigger under normal circumstances outside of echoraw(&t_RV) and the like), and given the wide variety of responses to t_RV there are, they would pretty much need to be programming defensively anyway (if it can't parse the response, ignore it), so I'm not sure I'd consider that to be a serious backwards-compatibility issue.
That said, it is a backwards compatibility issue, and if it's sufficiently important, then yeah, it probably needs a new name. I just can't think of a good one.
Maybe an alternative—if there were a way to check what pattern was used in the autocmd invocation, then it could special-case * to mean just the version, and introduce, say all, to actually mean any of the response types. But maybe the code necessary for that is just too grody.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you commented.
Christian Brabandt
So instead of re-using TermResponse, should we add a TermResponseAll event instead? That should be backwards compatible. Then you can test for its existence easily and don't need to use fgbg_event. Also, I find the patterns to be used slightly obscure. Can we make them more clear (e.g. ver -> terminalversion, style -> terminalstyle, rbg ->terminalbg, rfg -> terminalfg, etc)?
—
Reply to this email directly, view it on GitHub.
You are receiving this because you commented.
Danek Duvall
I find the patterns to be used slightly obscure. Can we make them more clear (e.g.
ver->terminalversion,style->terminalstyle,rbg->terminalbg,rfg->terminalfg, etc)?
Since they're all linked with the TermResponseAll event, do you think the terminal prefix is redundant? Would you be okay with version, cursorshape, bg (or background), fg, blink (or cursorblink) and ambiwidth (or ambiguouswidth)?
—
Reply to this email directly, view it on GitHub.
You are receiving this because you commented.
Danek Duvall
@dhduvall pushed 1 commit.
- 36f4f66 rename command & match strings, handle responses while blocked
—
View it on GitHub.
You are receiving this because you are subscribed to this thread.
Danek Duvall
@dhduvall pushed 1 commit.
- 8a132a5 remove unrelated change
—
View it on GitHub.
You are receiving this because you are subscribed to this thread.
Danek Duvall
@dhduvall pushed 1 commit.
- c15a54d update doc tags
—
View it on GitHub.
You are receiving this because you are subscribed to this thread.
Danek Duvall
I don't know how to test the lines that coverage is complaining about. It looks like the existing functionality wasn't covered, either, so I guess there isn't an existing test I can crib from. I'm happy to try to write the tests, but I think I'll need some assistance.
—
Reply to this email directly, view it on GitHub.
You are receiving this because you commented.
Christian Brabandt
thanks, that looks better. Let's just make it foreground instead of fg and background instead of bg.
—
Reply to this email directly, view it on GitHub.
You are receiving this because you commented.
Danek Duvall
@dhduvall pushed 1 commit.
- 10701e1 rename bg/fg to background/foreground
—
View it on GitHub.
You are receiving this because you are subscribed to this thread.
Christian Brabandt
—
Reply to this email directly, view it on GitHub.
You are receiving this because you commented.