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 support for displaying emotes using the terminal graphics protocol #317

Merged
merged 6 commits into from
Apr 10, 2023

Conversation

Nogesma
Copy link
Contributor

@Nogesma Nogesma commented Mar 21, 2023

This PR adds support for displaying emote as discussed in #265.

I don't really know windows' equivalent of ~/.cache so I left a todo!() in the cache_path function.

I also managed to hit a few panics during my testing but they also seem to appear on your main branch so I left them as is.

@Xithrius
Copy link
Owner

Xithrius commented Mar 21, 2023

Panics are probably due to window size, they'll be fixed with the other currently open PR, #313.

@Nogesma
Copy link
Contributor Author

Nogesma commented Mar 21, 2023

I pushed an update to fix the broken lock file.

@Xithrius
Copy link
Owner

Perhaps the cache on windows can be stored in LOCALAPPDATA's path? Or was it APPDATA? Haven't used windows to program in a long time lol

@Xithrius
Copy link
Owner

Xithrius commented Mar 21, 2023

Looks like windows build is aliven't, hecc

@Nogesma
Copy link
Contributor Author

Nogesma commented Mar 21, 2023

Yeah, seems like it's because of the dependency on sixel-rs of pic, which I don't use anyway. I might put it behind a feature flag to make it build.
Another option would be to integrate the code I use from pic directly into this repo, as it's only a few functions, and I modified almost all of them from the base repo to suit my needs.

@Nogesma
Copy link
Contributor Author

Nogesma commented Mar 21, 2023

Perhaps the cache on windows can be stored in LOCALAPPDATA's path? Or was it APPDATA? Haven't used windows to program in a long time lol

I can make it a subdirectory of the config dir, like APPDATA/twt/cache. I never really used windows so I don't really know.

@Xithrius
Copy link
Owner

Yeah, seems like it's because of the dependency on sixel-rs of pic, which I don't use anyway. I might put it behind a feature flag to make it build. Another option would be to integrate the code I use from pic directly into this repo, as it's only a few functions, and I modified almost all of them from the base repo to suit my needs.

Putting it behind a feature flag would probably be a good idea, along with integration of your code from pic.

@Xithrius
Copy link
Owner

Perhaps the cache on windows can be stored in LOCALAPPDATA's path? Or was it APPDATA? Haven't used windows to program in a long time lol

I can make it a subdirectory of the config dir, like APPDATA/twt/cache. I never really used windows so I don't really know.

That'll probably do fine for now.

@Xithrius
Copy link
Owner

PR #313 was merged, so panics when window is too small should be solved now. If there are any other panics, please let me know.

@Nogesma
Copy link
Contributor Author

Nogesma commented Mar 23, 2023

PR #313 was merged, so panics when window is too small should be solved now. If there are any other panics, please let me know.

Okay I rebased my branch on top of your main. Didn't have any panics yet.

I integrated most of the modified code from pic into src/utils/kitty.rs.
I also added support for gifs. I still need to look into making animated webp/avif work.

@Xithrius
Copy link
Owner

Very nice.

@Nogesma Nogesma force-pushed the main branch 2 times, most recently from 54ab9cc to 2f4aa50 Compare March 23, 2023 13:34
@Nogesma
Copy link
Contributor Author

Nogesma commented Mar 23, 2023

Build failures should be fixed, I switched to using webp for 7tv emotes, as avif seems to require libdav1d C library which is not present in the workflows.

@Xithrius
Copy link
Owner

Xithrius commented Mar 23, 2023

Running actions now.

@Nogesma
Copy link
Contributor Author

Nogesma commented Mar 28, 2023

I've been using this for a few days and I noticed a few issues.

The first one is that spaces are trimmed when wrapping messages (mgeisler/textwrap#503). For now I keep a copy of the message and keep trimming it with the message part that's in the current span, to then get it's length. It's probably not optimal but I didn't really find a way to reliably get the number of characters trimmed, so that I can work with numbers instead of cloning strings.

I also replace the emote name when parsing with 'a' * emote_width, then replace that again after wrapping with spaces. This would probably not be needed too if spaces were not trimmed.

Another issue is that the image rust crate panics sometimes when decoding webp images/animations (image-rs/image#1856). Webp support is needed to enable 7tv emotes. I disabled webp images until this is fixed.

I now download the emotes in another thread, as it was making the app start quite slow.

I rebased onto main, and squashed my commits.

@Nogesma Nogesma force-pushed the main branch 5 times, most recently from c803c98 to 02dac28 Compare March 28, 2023 14:02
@Nogesma
Copy link
Contributor Author

Nogesma commented Mar 28, 2023

Fixed clippy errors.

@Xithrius
Copy link
Owner

Xithrius commented Mar 31, 2023

I shall begin my own testing of this PR within the next few days.

@Nogesma
Copy link
Contributor Author

Nogesma commented Apr 2, 2023

I pushed an update to now scale the emote automatically to the terminal row height.
I also had to move some code from draw_ui() into another function as clippy was complaining about the function being too complex.

I might look into converting the webp images from 7tv into gifs so that I can display them too, but it probably would have a dependency on imagemagick.

@Xithrius
Copy link
Owner

Xithrius commented Apr 4, 2023

Although I am yet to begin testing of this implementation of emotes, the code itself seems to be in good order.

@Xithrius
Copy link
Owner

Xithrius commented Apr 6, 2023

image

It works very well. However, it seems that using kitty with tmux doesn't work, as echo $TERM results in tmux, causing the panic at https://github.com/Nogesma/twitch-tui/blob/main/src/handlers/config.rs#L315 to trigger.

Copy link
Owner

@Xithrius Xithrius left a comment

Choose a reason for hiding this comment

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

I believe that implementing tmux + kitty is out of scope for this PR and probably isn't possible, so I think the current state of this implementation will do.

I'll have another person review before merging this PR, and if you don't have anything else on your mind for said PR.

@Nogesma
Copy link
Contributor Author

Nogesma commented Apr 6, 2023

I've checked and even if I remove the env check on tmux + kitty, tmux doesn't respond to the check at https://github.com/Nogesma/twitch-tui/blob/09a282cdaa876a17dbd791b0f114aeebafe1c0ec/src/emotes/kitty.rs#L69 to see if the terminal supports the graphics protocol, and doesn't respond either to the query for the terminal size in pixels at https://github.com/Nogesma/twitch-tui/blob/09a282cdaa876a17dbd791b0f114aeebafe1c0ec/src/emotes/kitty.rs#L46 (which cause an infinite loop). Kitty icat feature seems to work throught tmux, but only on kitty, and seems broken on other terminals which support the graphics protocol.

I could maybe make it work but it would require an TIOCGWINSZ ioctl syscall to get the terminal size in pixel, which would introduce unsafe code.

I have a last bug I need to fix. After a lot of emotes are loaded, if we resize the terminal it will take a long time to reload all emotes, and sometimes will crash. I'll try to find a way to either tell ratatui to not clear the images, or maybe reload the images in another thread.

EDIT: tmux does not seem to support the graphics protocol and it does not seem that it will change. They filter out most of the escape sequences used by the graphics protocol.

@Nogesma
Copy link
Contributor Author

Nogesma commented Apr 6, 2023

Okay so I pushed an update which should fix the long freeze when resizing the window. I now only reload the emotes when needed.
I also switched to using the crossterm queues to send the graphics protocol commands, so it shouldn't flush the stdout buffer every command, it'll probably increase performance a bit.

I can squash my commits if needed before the merge.

Once this gets merge, I will probably make a PR to add this repo to the list of programs that support the graphics protocol on kitty's website.

@Nogesma
Copy link
Contributor Author

Nogesma commented Apr 6, 2023

Seems like I need to add some fallback function for versions of windows before 10. I don't think kitty is even supported on windows, and wezterm only supports windows 10 or later. I might just put a panic!() macro inside thoses functions if that's okay with you?

@Xithrius
Copy link
Owner

Xithrius commented Apr 6, 2023

Yep, alright with me.

@Nogesma
Copy link
Contributor Author

Nogesma commented Apr 6, 2023

I didn't realize switching channels was a feature of twt. Switching channels should work with emotes now.

@Nogesma
Copy link
Contributor Author

Nogesma commented Apr 7, 2023

Okay I think everything should be done now, let me know if I've missed anything.

@balroggg
Copy link
Contributor

balroggg commented Apr 8, 2023

Just checked emotes support from this branch, I have foot terminal emulator

cargo r --release
Running `target/release/twt`
The application panicked (crashed).
Message:  called `Result::unwrap()` on an `Err` value: 
   0: Configuration error.
   1: This terminal does not support the graphics protocol, please use a terminal such as Kitty or Wezterm, or disable emotes.

Location:
   src/handlers/config.rs:315

Maybe check terminal support by env vars and skip this feature for unsupported terminals?
For example:

// Check if the terminal is Kitty or not
fn check_kitty_support() -> bool {
    if let Ok(term) = std::env::var("TERM") {
        term.contains("kitty")
    } else {
        false
    }
}

@Nogesma
Copy link
Contributor Author

Nogesma commented Apr 8, 2023

I'm doing exactly that here. Do you mean just disabling emotes silently if the terminal doesn't support it?

@balroggg
Copy link
Contributor

balroggg commented Apr 8, 2023

I'm doing exactly that here. Do you mean just disabling emotes silently if the terminal doesn't support it?

Sorry I didn't notice this. Something like this, maybe return some kind of soft error(log the error and work or quit). Panic is the last thing users want to see) Just a suggestion, I know env vars maybe just been wrong.

Copy link
Collaborator

@kosayoda kosayoda left a comment

Choose a reason for hiding this comment

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

Works well during manual testing, nice job.

default-config.toml Outdated Show resolved Hide resolved
src/utils/pathing.rs Show resolved Hide resolved
src/emotes/graphics_protocol.rs Show resolved Hide resolved
@Xithrius
Copy link
Owner

Xithrius commented Apr 9, 2023

Sorry for noticing this only now, but I believe that documentation describing how to set emotes up should be added to https://github.com/Xithrius/twitch-tui/blob/main/book/src/guide/configuration.md.

Copy link
Owner

@Xithrius Xithrius left a comment

Choose a reason for hiding this comment

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

Looks good, works perfectly with just the IRC token.

@Xithrius Xithrius requested a review from kosayoda April 9, 2023 22:01
Copy link
Collaborator

@kosayoda kosayoda left a comment

Choose a reason for hiding this comment

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

Should be good to merge, any issues can be fixed in a separate PR. Thanks!

@Xithrius
Copy link
Owner

Thank you so much for the PR! I'll get a release going soon.

@Xithrius Xithrius merged commit abd1200 into Xithrius:main Apr 10, 2023
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants