gokcehan / lf

Terminal file manager
MIT License
7.73k stars 332 forks source link

Highlight makes entry invisible in the most right panel #1038

Closed pbogut closed 1 year ago

pbogut commented 1 year ago

Not sure when it started, I'm pretty sure it used to be visible correctly. First entry (or last selected entry) in right panel is invisible for me, is there a way to change the color of that highlight in lfrc?

Examples: First item invisible: screenshot_from_2022-12-04_10-39-32 Select second one and go back: screenshot_from_2022-12-04_10-39-17 Previously selected is invisible: screenshot_from_2022-12-04_10-39-26

Edit:

I'm using alacritty with solarized-dark theme. It seams like lf is using bright black color for that selection, but it's the same color as the background so it makes it invisible. I have same issue with urxvt with solarized theme.

gokcehan commented 1 year ago

cc #924 @ilyagr

gokcehan commented 1 year ago

The way I understand it, we can not rely on gray to be different than other colors so I have reverted this change. I'll make a new release today.

ilyagr commented 1 year ago

Yes, I misunderstood what Gray is when I made that PR. On a 16-color terminal, it does seem to be the same as brightblack. I noticed this eventually, but I hoped that this wouldn't cause a problem on most themes. My recommendation for these cases would be to change the theme so that brightblack and background colors are different (that is, make brightblack slightly darker or slightly brighter).

@gokcehan, I would obviously prefer a better solution than reverting this. Let me know if you'd be interested in that. At the moment, I can't think of anything better than a config option at the moment, though. If you aren't interested, this will become just one more personal customization for my lf.

Also, sorry I missed this three weeks ago. It was a busy time for me.

Happy holidays!

pbogut commented 1 year ago

@ilyagr for now since it happened I did change the colour in my theme so it is visible. I'ts good as a workaround but I don't think its reasonable to ask people to change their themes when its not uncommon to have this color be the same as background.

Configuration option would be the best solution. Either to enable/disable this change or better yet, let me pick the color in the config.

ilyagr commented 1 year ago

Yes, the fact that there are popular themes that have background identical to this "Gray" a.k.a brightblack color is important information.

I'm happy to make a config option (either a boolean, or a choice between various colors, or a normal/dim/invisible cursor with the last option not showing which file is highlighted in the preview) if that makes sense to @gokcehan . I can also understand if it doesn't.

gokcehan commented 1 year ago

@ilyagr I was delaying the new release for quite a while so I simply reverted this. I guess I would be open for a configuration option but boolean or color choice sounds unusual compared to our current color configurations for different things (e.g. promptfmt, errorfmt, tagfmt). Maybe an option with similar raw escape codes could also work for this as well?

ilyagr commented 1 year ago

So, you're suggesting something like cursorfmt and previewcursorfmt options with escape sequences as values. I guess lf would always print the escape sequence to go to normal font after it prints the cursor.

That sounds doable.

One positive aspect of this is that you might then be able to make previewcursor (or even cursor) just make the text bold. I need to look more carefully into how escape sequences work to be sure.

@gokcehan, I had no idea you were waiting on me to do a release. If I missed any other messages from you, please ping them to me again.

gokcehan commented 1 year ago

@gokcehan, I had no idea you were waiting on me to do a release. If I missed any other messages from you, please ping them to me again.

No that's fine. I didn't mean that I was waiting for you. I was just trying to find some energy to prepare the changelog and today finally I got it and this was one of the things I needed to figure out before the new release.

devnull09 commented 1 year ago

@ilyagr for now since it happened I did change the colour in my theme so it is visible. I'ts good as a workaround but I don't think its reasonable to ask people to change their themes when its not uncommon to have this color be the same as background.

Configuration option would be the best solution. Either to enable/disable this change or better yet, let me pick the color in the config.

Could you please share the configuration that can be used to implement this for the time being?

The greyed preview is one of the most useful features for me to keep my orientation and i really hope it will be reimplemented. I also think it should be default.

ilyagr commented 1 year ago

Could you please share the configuration that can be used to implement this for the time being?

You'll need to either compile lf at https://github.com/gokcehan/lf/commit/aa78b26e44574f3fef7cfdd4e6496dac405ba52f, or (if that commit becomes old) clone lf and then do git revert b47cf6 to revert https://github.com/gokcehan/lf/commit/b47cf6d5a525c39db268c2f7b77e2b7497843b17.

The greyed preview is one of the most useful features for me to keep my orientation...

😀

I also think it should be default.

I'm thinking that, after implementing @gokcehan's suggestion, perhaps we could make the default be no visible cursor in the preview pane at all. That should be a decent experience in all terminals. I'd add a few examples of other options for previewcursorfmt in the docs and one of them in lfrc.example.

I'm not sure when I'll have time to write the docs for this feature, so it might not happen for a few weeks.

p-ouellette commented 1 year ago

One alternative to a hardcoded preview selection color is the "dim" attribute (SGR 2). Here's what it looks like in Alacritty: 2022-12-27T21:59:19,252727760-06:00 2022-12-27T22:00:00,383976831-06:00 I like this because you can still tell if the file is a directory/regular file/executable/etc. The dim attribute also works in at least xterm and VTE-based terminals, but it's not very well defined in the standard so not all terminals implement it the same. In Kitty it just dims the foreground color, even with the reverse attribute set.

A likely more reliable approach would be to query the terminal foreground and background colors and blend them to get a dimmed version. Tcell doesn't currently support querying terminal colors, but I think most terminals support the escape sequences to do so.

ilyagr commented 1 year ago

The dimmed attribute is interesting. I filed a request for Kitty.

Although, it looked to me that xterm did the same thing as kitty, and the dimming wasn't very visible (though it was there). It does look good in xterm.js.

ilyagr commented 1 year ago

Underlining makes for a surprisingly decent possible default: image

Should work on essentially all terminals, I think

devnull09 commented 1 year ago

One alternative to a hardcoded preview selection color is the "dim" attribute (SGR 2). Here's what it looks like in Alacritty:

This looks pretty neat as well.

How did you configure the colors in alacritty to look exactly like that? So far I have only been using the lf color configuration.

ilyagr commented 1 year ago

I've decided that the underline should be a good enough solution for the short term, and possibly for the long term as well. It's visible in all terminals I tried and, more importantly, I can't imagine a case where it will harm readability. So, I made https://github.com/gokcehan/lf/pull/1072.

I'm happy to stop here, and I'm also happy to eventually implement the config we discussed above in https://github.com/gokcehan/lf/issues/1038#issuecomment-1364586156. That would allow people to use the "dim" attribute @p-ouellette suggested if they use a terminal where it's visible enough, or something else. It's a shame it's not prominent in many terminals and, at least according to [one expert opinion[(https://github.com/kovidgoyal/kitty/issues/5831) that is what the standard demands.

ilyagr commented 1 year ago

It seems that #1072 doesn't sit well with a significant number of people. Now, I'm thinking of implementing the PR with configuration and making the default preview cursor be the one with the dim attribute set, as suggested by @p-ouellette above in https://github.com/gokcehan/lf/issues/1038#issuecomment-1366364695.

My reasoning is as follows. While the dim attribute is not super-visible in all terminals, it is quite visible in some terminal, and it should not hide any vital information on any terminal or be very upsetting to anyone. The users that want it to be visible will at least know this feature exists, and will be somewhat likely to actually discover the configuration option in the docs.


The other alternatives I'm seriously considering is making the default to be the same as the normal cursor (as it is now and was in r27) or making the default be the lack of any cursor in the preview pane. I don't like these because they will mean most users will never discover either the possibility of having a preview cursor (which they might consider a bug if they're used to what ranger looks like) or the possibility of dimming it.

Personally, I'm using the underline cursor from #1072 in the preview panel.I still think it works great and looks great after a short period of getting used to it[^underline].

[^underline]: I closed #1072, but the code is still perfectly usable. Compiling lf with that 6-line patch is quite easy if you want to.

The last option is the one I'm not really considering. Re-implementing https://github.com/gokcehan/lf/commit/b47cf6d5a525c39db268c2f7b77e2b7497843b17 with the grey preview cursor color is not really an option unless we can find an issue to avoid the bug this thread is about. Even if it affects relatively few people, I don't want them to feel that lf is buggy. So, it doesn't seem reasonable to make the grey background the default even if it's configurable[^grey].

[^grey]: On the other hand, compiling lf with the grey preview cursor is even easier than compiling it with #1072. See https://github.com/gokcehan/lf/issues/1038#issuecomment-1366330648).

All of these options should be achievable in the end by setting a config option. As ever, let me know if you have any more thoughts.