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

Add optional argument to force fish_vi_cursor to allow a given $TERM #6968

Closed
wants to merge 4 commits into from

Conversation

LawAbidingCactus
Copy link
Contributor

@LawAbidingCactus LawAbidingCactus commented May 3, 2020

Description

This adds a check to fish_vi_cursor that compares $argv[1] against $TERM, enabling cursor shape setting if it matches. This is useful for people with nonstandard terminal emulation setups that support the relevant termcaps but aren't acknowledged by fish_vi_cursor (term-in-terms with tmux come to mind). This is vastly simpler than attempting to account for the enormous ecosystem of terminal emulation implementations and configurations, which quickly becomes untenable.

TODOs:

  • Changes to fish usage are reflected in user documentation/manpages.
  • Tests have been added for regressions fixed
  • User-visible changes noted in CHANGELOG.md

@faho
Copy link
Member

faho commented May 3, 2020

I don't think whitelisting a particular $TERM is all that helpful, given that it's not necessary or sufficient - usually it's "xterm-whatever".

So just adding a way to force it to skip checks would work better, and since it's often called from fish_vi_key_bindings an option won't work that well, so how about an environment variable called fish_vi_force_cursor or so.

If people want to check $TERM, they can then set that variable depending on $TERM.

@LawAbidingCactus
Copy link
Contributor Author

LawAbidingCactus commented May 3, 2020

Yeah, that makes sense-- done. I've also removed the flag for forcing cursor shapes on iterm, since that's redundant now (or perhaps it should just be deprecated? Though I didn't find anyone using it after a quick github search).

Copy link
Member

@krobelus krobelus left a comment

Choose a reason for hiding this comment

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

Intuitively, I'd prefer a name that starts with fish_vi_, not sure though.

CHANGELOG.md Outdated Show resolved Hide resolved
share/functions/fish_vi_cursor.fish Outdated Show resolved Hide resolved
@LawAbidingCactus
Copy link
Contributor Author

Makes sense; I don't have a particular preference here.

CHANGELOG.md Outdated Show resolved Hide resolved
@krobelus
Copy link
Member

I've also removed the flag for forcing cursor shapes on iterm, since that's redundant now (or perhaps it should just be deprecated? Though I didn't find anyone using it after a quick github search).

Could be fine since this is rarely used and only cosmetic a feature? @faho @zanchey

@zanchey
Copy link
Member

zanchey commented May 13, 2020

--force-iterm was never documented, although it seems like a cheap feature to keep working.

@LawAbidingCactus
Copy link
Contributor Author

LawAbidingCactus commented May 14, 2020

I've deprecated --force-iterm for the time being, then; I'll remove it in a future pull request (just so fish_vi_cursor.fish stays a bit cleaner).

@krobelus
Copy link
Member

Merged as f71737e, thanks and sorry for the delay.

@krobelus krobelus closed this May 14, 2020
@krobelus krobelus added this to the fish 3.2.0 milestone May 14, 2020
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 13, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants