jeff-hughes / shellcaster

Terminal-based podcast manager built in Rust
GNU General Public License v3.0
199 stars 13 forks source link

Castero-inspired UI-improvements #23

Closed rien333 closed 3 years ago

rien333 commented 4 years ago

Cool little app! Somewhat more snappy and less resource heavy than castero.

Although there's nothing incredibly wrong with how this app looks by default (in fact, it arguably looked better for me than castero did by default), it would be nice if could change the UI colors to my liking from the config file. I'm particularity interested in a way to change the background color, as some ncurses apps set my background color wrongly. Looking at shellcaster's code, this seems to be because you use the color "black" as a background color, which in the nord color scheme is not the same as the user's background color. This results in some funky looking visual artifacts across different terminals: 2020-10-15-1602770963 (this is after some scrolling around; the gray stuff is what ncurses treats as the color black, the grayer stuff is my actual terminal background. Note due to my terminal, there's always a border with my actual background color around shellcaster)

It's also really cool that you support mouse events! However, clicking doesn't seem to work yet, and scrolling acts kind of weird on some of my devices (okay in termux on android, weird on linux). Perhaps shellcaster assumes some kind of smooth scrolling that doesn't really translate to all devices?

Have you also considered interfacing with libmpv and libvlc to get statusinfo (e.g. playback position)? This is what castero does.

jeff-hughes commented 4 years ago

Hey, thanks for the ideas! I am a little curious about the colour issue you're running into. I did test on several different terminal emulators and didn't have this problem, but it's entirely plausible that I'm missing some of the nuances of how terminals set the colours. What terminal emulator are you using? I will try to replicate the issue and see if I can play around with it.

I'll be honest -- I spent zero time or effort looking into or implementing mouse events. That's either the pancurses crate adding that, or ncurses itself adding it. It seems like it just translates scroll events into the equivalent of up and down arrows. So yeah, I know ncurses/pancurses provide some support for finding mouse positions and identifying clicks, etc. I just never bothered to look into it. I might add support for that at some point, but to be honest, it's pretty low on my list. The goal was always for keyboard commands :)

Yeah, I considered adding some sort of library for media playback in there; I agree it's a nice feature of castero. However, it does end up adding a bunch of complexity in terms of interfacing with an external library, handling info about playback, handling user input to play/pause/seek, and so on. It also essentially sort of limits the programs the user can use for playback -- that's not a critical issue, but if someone has a player they particularly like, or wants to change the "play" command to add to a playlist or something, that's something I wanted to allow for. So in the interest of a bit more simplicity and flexibility, I kind of wanted to avoid any sort of playback handling. Quite frankly, I'd rather that shellcaster do one thing (managing podcasts) well, rather than trying to do multiple things and maybe end up not being good at any of them. Supporting an arbitrary command lets me be completely agnostic to whatever program you use to play files -- as long as it can handle a filepath and/or URL, we're good.

Anyway, please do let me know what terminal emulator you're using, because I'd like to try to nail down the colour issue there. Thanks again for your comments and ideas!

rien333 commented 4 years ago

All right, thanks for the detailed response!

It seems like [pancurses/ncurses] just translates scroll events into the equivalent of up and down arrows.

That's interesting. Normally I wouldn't care and would be fine with keyboard controls; I just happend to be using this app over ssh on an android device. Being able to use a touchscreen to navigate around TUI apps works surprisingly well (nnn and cmus are actually p nice to use on android), but I completely understand if this use case is rather obscure.

In the interest of a bit more simplicity and flexibility, I kind of wanted to avoid any sort of playback handling. Quite frankly, I'd rather that shellcaster do one thing (managing podcasts) well, rather than trying to do multiple things and maybe end up not being good at any of them.

Makes total sense, actually. The thing is that mpv (either per my configuration, or by default) doesn't always launch a window, and an invisible mpv instance is fairly hard to control and get information on. I completely solved this issue, though, by just using play_command = "mpv --force-window=immediate %s". mpv already handles UI stuff well, so there's indeed no need to replicate what it does.

Anyway, please do let me know what terminal emulator you're using, because I'd like to try to nail down the colour issue there. Thanks again for your comments and ideas!

I'm using kitty, though the issue is also present on st. I think you should be able to reproduce it fairly well if you configure kitty to use the same color scheme as I do.

# put this in kitty's config (~/.config/kitty/kitty.conf)

# Nord theme
foreground            #D8DEE9
background            #2E3440
selection_foreground  #2E3440
selection_background  #D8DEE9
url_color             #0087BD
cursor                #81A1C1

# black
color0   #3B4252
color8   #6d7a96

# red
color1   #BF616A
color9   #BF616A

# green
color2   #A3BE8C
color10  #A3BE8C

# yellow
color3   #EBCB8B
color11  #EBCB8B

# blue
color4  #81A1C1
color12 #81A1C1

# magenta
color5   #B48EAD
color13  #B48EAD

# cyan
color6   #88C0D0
color14  #8FBCBB

# white
color7   #E5E9F0
color15  #ECEFF4

I had similar issues with castero, which the author then solved for me. I think the difference here is that castero request the user's "foreground" and "background" from ncurses (see https://github.com/xgi/castero/blob/master/castero/display.py#L118), whereas shellcaster asks for "black" and "white". As can be seen in my configuration above, the colors I've specified for "black" and "background" are slightly different (which is normal for nord terminal themes I think, see for example st's nord theme).

jeff-hughes commented 4 years ago

Hey, sorry about the delayed response. I just built a new PC, so I've been spending a lot of time setting everything up again, that's taken some time! But thanks for the details; I will definitely look into it and see if I can fix the colour issues. It's on my to-do list, it's just gotten....delayed.

jeff-hughes commented 3 years ago

Hi again, @rien333, I finally got around to testing things out. I set up kitty, added the colour scheme you provided, but I'm unable to replicate the same issue you were having: image

As you can see, it shows the custom gray colour as the background, without any of the odd colour artifacts in your screenshot.

By specifying COLOR_BLACK in the code, it should be pulling whatever colour your terminal specifies as "black". It's not requesting a specific RGB value, only the colour represented as "0". Similarly, in castero, it's not requesting "foreground" and "background", what it's doing is pulling those values from the config file (see here). And at least the defaults for those seem to be "yellow" and "black", which are then translated into the ncurses colours (here).

In taking a look through ncurses documentation, though, I realized there is a method to set window backgrounds, and I'm wondering if that may be the issue. I've added a new branch to try this out, but since I don't seem to be able to reproduce the issue on my end, I'm hoping you may be able to test it out for me. It would require cloning the git repo, switching to the set_background branch, and compiling from there. Let me know if that's something you're able to do -- if not, that's totally fine! You could also let me know what Linux distro you're using, and I can try spinning that up a VM to see if I have better luck that way. Either way, I'm hoping we can nail the issue down.

rien333 commented 3 years ago

As you can see, it shows the custom gray colour as the background, without any of the odd colour artifacts in your screenshot.

These "artifacts" appear only when you scroll, btw. I should've been clearer, sorry. Initially, the whole window is indeed printed in a gray-ish color (but see below), but the background beneath will appear if you scroll down a bunch (more than a full page). Also, I'm actually mostly bugging you because I don't like to have one terminal window in a different background color than others. Do with that what you will, but, as shown in the screenschot you provided, the gray you're speaking of is not the terminal background color (which is much darker).

One further reason I don't like TUI programs that set their own custom background color, is because I've window padding set in kitty. This extra padding area is also shown in the suckless terminal, FWIW. This creates a background color discrepancy, as shown here:

2020-11-11-1605081814

Sorry for forgetting about this line earlier, but to reproduce add this to the config lines posted above:

window_padding_width 8 # or make it bigger to make it more pronounced 
jeff-hughes commented 3 years ago

Yes, you're right that the gray in the background is the "black" colour and not the "background" colour. I see what you mean. Despite scrolling all over the place, I'm unable to replicate the issue you were having, unfortunately. I'm on Arch Linux, which gets the latest updates, so it's possible it's something that's been fixed in kitty recently, or it's a difference between ncurses libraries, or....who knows. Again, if you can tell me what distro you're running, I can play around with it in a VM and see if I can replicate the issue.

As far as programs using their own background colour...I can certainly appreciate the annoyance. However, I am a little hesitant to rely on the foreground/background colours people have set on their terminals, just because of the variety of colours people use. Most people are probably using some sort of white/light text on a black/dark background, but that's not a guarantee. The default xterm colours are black text on white, for example. And there are plenty of people that go for the sort of "retro" look of green or yellow text on black. If I start letting the app use these colours, we're going to run into issues where text is illegible (white text on a white background) or just plain ugly (green text with a yellow selection cursor). At least if I use the colors set as "black" and "white", I can be reasonably confident that they're somewhat close to black and white. Your theme uses grays instead of black and white, but it's close enough to be reasonable.

So, the only way to really allow for using "foreground" and "background" is to make all the colours fully customizable in the config file. And that's fine, I definitely like that idea -- and I know it's what you suggested in the first place! But it's something that will take some work to implement. However, this issue you're having where the "black" background colour isn't being properly applied to the whole window is a separate concern -- and it's something I would need to nail down even if I do go with some sort of customizable colours. No point in having custom colours if the app still looks bad because half of the terminal background is showing through. So I'll put custom colours on my list of possible new features. But in the meantime, if you can let me know what distro you're running, I'll see if I can nail down the bug. Thanks!

rien333 commented 3 years ago

Arch linux, so that won't really help. I've been using kitty-git, FWIW. I can post a screencast in a bit, the problem seems to be that not the whole background is being redrawn in every case.

Oh and I just noticed that you made a branch that tries to deal with this issue, I'll try that one out!

This issue you're having where the "black" background colour isn't being properly applied to the whole window is a separate concern

It's closely related, actually. It's a direct effect of terminal emulators adding a background-colored padding area, which for whatever reason cannot be drawn to. Hence, all TUI apps that set a custom background color have this exact problem, not just shellcaster. However, most TUI apps (except a few older ones) don't set their own custom background color, so normally one never notices. In that sense, you should be able to reproduce this specific issue fairly easily if you add window_padding_width 30 to your config (actually, I can see the issue in the screenshot you provided above).

rien333 commented 3 years ago

Oh and I just noticed that you made a branch that tries to deal with this issue, I'll try that one out!

Yeah, your set_background branch got rid of the issue where shellcaster didn't fully redraw its background. Thanks! As for fixing the remainder of my issues (color preferences and consistency and the like), I think the easiest way to go would indeed be to implement reading the user's color preferences from a config file.

jeff-hughes commented 3 years ago

Awesome, thanks for testing that out! I've merged the bug fix into the develop branch, so you should be able to either use the shellcaster-git package from the AUR, or you can wait for the next release, which I think should be coming soon.

I've opened another issue for the option for custom colors, just to keep things tidy. So I'm going to close this issue, but if you have more to say about that feature, feel free to discuss in #25.

jeff-hughes commented 3 years ago

Hey, just following up here to note that the next release of shellcaster, v1.2.0, will provide customizable colors! This will include the ability to use the terminal's default background color, so you can have a transparent background in the app. You'll also be able to use the named terminal colors (white, black, red, and so on), or set a completely custom color with hex codes or rgb() values.

The new version should be released within the next day or two -- I'm hoping to have some time tonight to put the finishing touches on it and release it.