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

Highlight makes entry invisible in the most right panel #1038

Closed
pbogut opened this issue Dec 4, 2022 · 16 comments · Fixed by #1086
Closed

Highlight makes entry invisible in the most right panel #1038

pbogut opened this issue Dec 4, 2022 · 16 comments · Fixed by #1086

Comments

@pbogut
Copy link

pbogut commented Dec 4, 2022

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
Copy link
Owner

gokcehan commented Dec 4, 2022

cc #924 @ilyagr

gokcehan added a commit that referenced this issue Dec 24, 2022
@gokcehan
Copy link
Owner

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
Copy link
Collaborator

ilyagr commented Dec 24, 2022

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
Copy link
Author

pbogut commented Dec 24, 2022

@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
Copy link
Collaborator

ilyagr commented Dec 24, 2022

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
Copy link
Owner

@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
Copy link
Collaborator

ilyagr commented Dec 24, 2022

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
Copy link
Owner

@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
Copy link
Contributor

@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
Copy link
Collaborator

ilyagr commented Dec 28, 2022

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 aa78b26, or (if that commit becomes old) clone lf and then do git revert b47cf6 to revert b47cf6d.

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
Copy link
Contributor

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
Copy link
Collaborator

ilyagr commented Dec 28, 2022

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
Copy link
Collaborator

ilyagr commented Dec 28, 2022

Underlining makes for a surprisingly decent possible default:
image

Should work on essentially all terminals, I think

@devnull09
Copy link
Contributor

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
Copy link
Collaborator

ilyagr commented Jan 2, 2023

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 #1072.

I'm happy to stop here, and I'm also happy to eventually implement the config we discussed above in #1038 (comment). 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
Copy link
Collaborator

ilyagr commented Jan 6, 2023

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 #1038 (comment).

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 it1.

The last option is the one I'm not really considering. Re-implementing b47cf6d 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 configurable2.

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.

Footnotes

  1. I closed Make preview cursor an underline #1072, but the code is still perfectly usable. Compiling lf with that 6-line patch is quite easy if you want to.

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

ilyagr added a commit to ilyagr/lf that referenced this issue Jan 19, 2023
…w panes

This allows using a different style for the normal and preview cursor. See
linked issues and PRs for the context.

The default behavior is an underline, same as in
gokcehan#1072, since it should not cause severe
problems and be visible on all terminals. It can now be easily changed.

Fixes gokcehan#1038

Follows up on:

gokcehan#1072
gokcehan#924
gokcehan@b47cf6d5a5
ilyagr added a commit to ilyagr/lf that referenced this issue Jan 19, 2023
This defines new `cursorfmt` and `crusorpreviewfmt` options to style cursor in normal/preview panes. This allows using a different style for the normal and preview cursor. The documentation includes several possible values these options can be set to.

The default behavior is an underline, same as in
gokcehan#1072, since it should not cause severe
problems and be visible on all terminals. It can now be easily changed, as explained in the docs. I also added an example to `etc/lfrc.example`.

See linked issues and PRs for the more context.

Fixes gokcehan#1038

Follows up on:

gokcehan#1072
gokcehan#924
gokcehan@b47cf6d5a5
ilyagr added a commit to ilyagr/lf that referenced this issue Jan 19, 2023
This defines new `cursorfmt` and `crusorpreviewfmt` options to style cursor in normal/preview panes. This allows using a different style for the normal and preview cursor. The documentation includes several possible values these options can be set to.

The default behavior is an underline, same as in
gokcehan#1072, since it should not cause severe
problems and be visible on all terminals. It can now be easily changed, as explained in the docs. I also added an example to `etc/lfrc.example`.

See linked issues and PRs for the more context.

Fixes gokcehan#1038

Follows up on:

gokcehan#1072
gokcehan#924
gokcehan@b47cf6d5a5
ilyagr added a commit to ilyagr/lf that referenced this issue Jan 19, 2023
This defines new `cursorfmt` and `crusorpreviewfmt` options to style cursor in normal/preview panes. This allows using a different style for the normal and preview cursor. The documentation includes several possible values these options can be set to.

The default behavior is an underline, same as in
gokcehan#1072, since it should not cause severe
problems and be visible on all terminals. It can now be easily changed, as explained in the docs. I also added an example to `etc/lfrc.example`.

See linked issues and PRs for the more context.

Fixes gokcehan#1038

Follows up on:

gokcehan#1072
gokcehan#924
gokcehan@b47cf6d5a5
ilyagr added a commit to ilyagr/lf that referenced this issue Jan 20, 2023
This defines new `cursorfmt` and `crusorpreviewfmt` options to style cursor in normal/preview panes. This allows using a different style for the normal and preview cursor. The documentation includes several possible values these options can be set to.

The default behavior is an underline, same as in
gokcehan#1072, since it should not cause severe
problems and be visible on all terminals. It can now be easily changed, as explained in the docs. I also added an example to `etc/lfrc.example`.

See linked issues and PRs for the more context.

Fixes gokcehan#1038

Follows up on:

gokcehan#1072
gokcehan#924
gokcehan@b47cf6d5a5
ilyagr added a commit to ilyagr/lf that referenced this issue Jan 28, 2023
This restores functionality from gokcehan#924
reverted in gokcehan@b47cf6d5a5, while
fixing gokcehan#1038.

Unlike before, the cursor in the preview column is now an underline
rather than a grey background. This seems to work on most terminals.
In no case should the visibility of the file name itself be affected.

For messages such as "empty" or "permission denied", the preview
cursor is the same as the active cursor for now.

There is no configuration, but there is nothing preventing adding
config as discussed in gokcehan#1038
in the future.

![Screenshot](https://user-images.githubusercontent.com/4123047/209767426-abcbddd4-965a-43a8-a5a8-6242681bbd98.png)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants