-
Notifications
You must be signed in to change notification settings - Fork 70
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks.
In addition to my remake I'd appreciate if you could add this to CHANGELOG.md
.
I appreciate your suggestions but I am not convinced that this refactoring would have any benefit. In particular the TERM variable does not identify the terminal emulator per se; instead it simply denotes the terminal capabilities the terminal emulator would like to announce.
While some emulators do announce custom values and inject the corresponding entries into the term cap database many others simply announce a known value, mostly xterm 256, to maintain compatibility with existing programs.
Since the term cap database doesn't support any of the more recent extensions to terminal protocols (true colours, inline images, links, etc) I find the value of extending TERM with custom values questionable, and would rather rely on alternative means to identify the actual terminal emulator.
I understand that this position is debatable but unless the features supported by mdcat promote from proprietary extensions of terminal emulators to general standards with termcap entries which we can detect in a generic way through terminfo I see little value in making TERM checks a generic thing in mdcat.
Please do also check the Github actions error, and do fix it. Thanks |
Welcome! :-)
Will do.
Sorry! I didn't make it clear when I said "with slight modification". What I meant was to change that funcion to
then we could use it as
Of course, this is not necessary, as there are only few terminals atm which support the features mdcat wants. |
@MuhammedZakir Wrt to the actions failure I have removed a few needless borrows on main; please rebase onto main and the error should be gone 🙂 |
99e33b7
to
cd79f88
Compare
4761409
to
da58e5f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please also check why the rustfmt pipeline step complains.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I forgot to ask but could you add Wezterm to feature table in README as well?
Done. Btw, I wasn't trying to be lazy regarding doc comments. I haven't written much code, so am not sure what should be doc comments, and what should be code comments. So feel free to say if you need to change anything. Also, regarding punctuation, for single line comments, I have seen with and without ending dot. I have corrected it now. Is this a Rust convention? Anyway, I just wanna say, this is due to my inexperience, and not laziness. I hope I didn't create much trouble for you! :-) |
@MuhammedZakir Oh, I didn't mean to imply you were lazy. I'm sorry for this misunderstanding. You couldn't have known; these are just my personal preferences after all. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A minor last thing about the CHANGELOG.
And please do squash all commits into a single one (e.g. with git rebase -i
).
Wez's Terminal Emulator - https://wezfurlong.org/wezterm/ Uses iTerm image protocol for rendering images
I just found out that WezTerm supports "mark" using semantic prompts [1]. Unlike iTerm2, this one needs ending marker. Prompt mark start: What is the right way for implementing this? [1] https://gitlab.freedesktop.org/Per_Bothner/specifications/blob/master/proposals/semantic-prompts.md |
That's a different kind of mark which is of no use to mdcat 🙂 |
AFAIK, technically, they are. https://gitlab.freedesktop.org/Per_Bothner/specifications/blob/master/proposals/semantic-prompts.md:
https://iterm2.com/documentation-shell-integration.html:
This is the exact same thing mentioned in semantic-prompts proposal, except, iTerm2 sometimes mention "history location", and the proposal always mention "REPL". I understand what both of them "stressed" is different, but from what I can understand, adding markes using semantic-prompt is much easier as there is support for application identifier and prompt kinds [1]. IMHO, it's worth adding. After all, it's very useful. :-) Edit: [1] what I meant here is, we could have different keyboard shortcuts - one for shell prompts, and 1+ for everything else, making it much easier to navigate. |
@MuhammedZakir I am sorry but they are not; mdcat uses the proprietary iTerm escape code You're referring to the FTCS_PROMPT and FTCS_COMMAND_* escape codes which specifically mark the beginning and the end of the prompt, and only that. Modern terminal emulators rely on these escape codes to identify where the prompt is and whether a command is running in the terminal window; mdcat must not emit these codes lest it confuses the terminal emulator. |
Hmm... :-( |
@MuhammedZakir Does this change actually work for you? I tried a recent wezterm release, and it doesn't set |
You'll have to set it manually. |
@MuhammedZakir Okay, but why didn't you use |
I didn't know that. I thought it was only set it in macOS. Does (cross-platform?) terminals normally set that in Linux? P.S. I can send a PR to fix this. Do we need to check if either ( |
Just TERM_PROGRAM, just like for iTerm. I don't think it's a standard variable, but as far as I can see wezterm sets it always 🙂 Would be really cool if you could send a PR for this 🙏😇 |
See e.g. $ env | rg -i wezterm
GIO_LAUNCHED_DESKTOP_FILE=/usr/share/applications/org.wezfurlong.wezterm.desktop
TERM_PROGRAM=WezTerm
WEZTERM_UNIX_SOCKET=REDACTED
WEZTERM_EXECUTABLE_DIR=/usr/bin
WEZTERM_PANE=0
WEZTERM_CONFIG_FILE=REDACTED
WEZTERM_EXECUTABLE=/usr/bin/wezterm-gui
WEZTERM_CONFIG_DIR=REDACTED |
Wez's Terminal Emulator - https://wezfurlong.org/wezterm/
Uses iTerm image protocol for rendering images
IMO, terminal specific files in
src/terminal/
dir can be modified to make it as "protocol/features" specific code. And then, we could useis_term_env_var
function, with slight modification, for detecting other terminals (except VTE). What do you think?Btw, thanks for this cool app! :-)