use tmux passthrough for kitty graphics protocol #3086
Conversation
There was a problem hiding this comment.
Hey, thanks for the keeping the pr alive and the update, it's cleaner now!
Should we separate the commit in two ? Something like :
- support for virtual placement using unicode placeholder
- support for tmux allow-passthrough mode
Moreover some minor changes below.
Updated Ethan PR for current master Co-authored-by: Ethan <milonthan@gmail.com>
Updated Ethan PR for current master Co-authored-by: Ethan <milonthan@gmail.com>
3608676 to
3ac971f
Compare
|
Do the resolve comments work in comments or should they be part of commit messages? |
|
Hi,
From what I read, the resolve don't work in comment, so they should be part of the commit message. I have commented them so we don't have to search them later.
Ok, the PR seems good for me. |
5e06dd4 to
66c1488
Compare
153c40b to
62c8d89
Compare
|
Hi, |
|
|
||
| try: | ||
| with open( | ||
| self.fm.relpath("rowcolumn-diacritics.txt"), |
There was a problem hiding this comment.
| self.fm.relpath("rowcolumn-diacritics.txt"), | |
| self.fm.relpath(“data”, “rowcolumn-diacritics.txt"), | 10000
@Ethsan you placed file in data folder, so the path is incorrect and throws error.
also why file named txt while it’s treated like csv? 🤔
There was a problem hiding this comment.
Hi,
@Ethsan you placed file in data folder, so the path is incorrect and throws error.
Yeah sorry about that, I move it will trying ot fix linting issues and forgot to update path.
also why file named txt while it’s treated like csv? 🤔
I have simply reused the name given upstream rowcolumn-diacritics.txt
There was a problem hiding this comment.
FWIW in the future I'd prefer if people tried to PR to the PR branch of the original PR author, possibly linking to the downstream PR in our PR comments. It's inconvenient to keep track of things if people keep opening new PRs that are really the same thing. Rebasing on master is usually quite trivial so you can rest assured that that is not what holds back a PR from getting merged.
In this case Ethsan seems perfectly happy with the situation but it can come across as trying to take credit for someone else's work. All of this is easily avoided by opening the PR against their PR branch.
There was a problem hiding this comment.
I was kinda hoping to read this from wherever Kitty or Ghostty install it on the system but I guess this makes it compatible with other terminals too.
There was a problem hiding this comment.
I agree it would have been cleaner.
For example, icat from the kitty repo use an array with the unicode character, like in the original idea.
| newline="", | ||
| encoding="utf-8", | ||
| ) as file: | ||
| reader = csv.reader(file, delimiter=";") |
There was a problem hiding this comment.
Is csv.reader more performant than rolling our own? It's just some hex digits from the starts of lines, right? I'd expect parsing the entire lines to take a little bit longer.
| except CalledProcessError: | ||
| tmux_allow_passthrough = b'off' | ||
| if tmux_allow_passthrough == b'off': | ||
| raise ImageDisplayError("allow-passthrough no set in Tmux!") |
There was a problem hiding this comment.
| raise ImageDisplayError("allow-passthrough no set in Tmux!") | |
| raise ImageDisplayError("allow-passthrough is not set in Tmux!") |
| # convert hex code to int and then to unicode char | ||
| self.diacritics = [chr(int(hex, 16)) for hex in hex_list] | ||
| except Exception as err: | ||
| raise ImageDisplayError("Error while loading diacritics" + str(err)) |
There was a problem hiding this comment.
| raise ImageDisplayError("Error while loading diacritics" + str(err)) | |
| raise ImageDisplayError("Error while loading diacritics: " + str(err)) |
| answer_start = protocol_start | ||
| answer_end = protocol_end |
There was a problem hiding this comment.
This part is honestly a bit confusing. Why are we initializing two sets of these? Do we need answer_start after changing protocol_start while also not being able to use it instead of protocol_start before changing it?
| # check if we inside tmux | ||
| # if true then we use the tmux escape sequence |
There was a problem hiding this comment.
| # check if we inside tmux | |
| # if true then we use the tmux escape sequence | |
| # Use Tmux escape sequence if appropriate. |
| # check if tmux allows passthrough | ||
| # use tmux escape sequence if it's enabled | ||
| # throw error if it's disabled |
There was a problem hiding this comment.
| # check if tmux allows passthrough | |
| # use tmux escape sequence if it's enabled | |
| # throw error if it's disabled | |
| # Check whether allow-passthrough is enabled |
| # catch kitty answer before the escape codes corrupt the console | ||
| resp = b'' | ||
| while resp[-2:] != self.protocol_end: | ||
| while resp[-2:] != self.answer_end: |
Updated Ethan PR for current master
ISSUE TYPE
RUNTIME ENVIRONMENT
CHECKLIST
CONTRIBUTINGdocument has been read [REQUIRED]DESCRIPTION
Updated PR #2856 to be compatible with current master branch.
MOTIVATION AND CONTEXT
Want to finally see images inside tmux on my mac and outside of iterm/kitty terminal 😆
Found old Ethan PR and decided to resolve conflicts and update it for current master
TESTING
make testexecuted successfullyIMAGES / VIDEOS
cleanshot.mp4