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

Allow separate styles for cursor in normal and preview panes, simplify tagfmt and errorfmt #1086

Merged
merged 2 commits into from
Feb 11, 2023

Conversation

ilyagr
Copy link
Collaborator

@ilyagr ilyagr commented Jan 19, 2023

This PR contains two commits. The first one introduces the new feature. The second one simplifies the configuration of both the new and existing features.

New feature: Allow separeate styles for cursor in normal and preview panes

This defines new cursorfmt and crusorpreviewfmt options to style cursor in normal/preview panes, as discussed in #1038 (comment). (I renamed the options so that they sort next to each other). 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 inverted normal cursor (as before) and an underline in preview, same as in
#1072. The underline 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.

I don't expect many people to change the normal cursor style, but some may want some fancy color and it's simpler to have both settings for consistency. For another example, a bigger fan of underlines than myself might make their normal cursor an underline and have no preview cursor at all.

See linked issues and PRs for the more context.

Fixes #1038
Fixes #1102

Follows up on:

#1072
#924
b47cf6d5a5

Simplification commit

This simplifies the configuration of cursorfmt, cursorpreviewfmt, tagfmt,
and errorfmt options.

In almost all usecases, the value of these options had to end with %s\033[0m
to print the contents of filename/tag/error and reset the terminal style. Now,
if %s is not part of the option's value, %s\033[0m is appended
automatically. This simplifies configuration.

I retained the same behavior as before when %s is part of the string for
backwards compatibility. I think it should be considered deprecated, but
preserving it causes no harm.

@ilyagr ilyagr force-pushed the cursorfmt branch 3 times, most recently from 9c883e8 to ac8b1fd Compare January 20, 2023 05:08
@ilyagr ilyagr changed the title Allow separeate styles for cursor in normal and preview panes, simplify tagfmt and errorfmt Allow separate styles for cursor in normal and preview panes, simplify tagfmt and errorfmt 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
This simplifies the configuration of `cursorfmt`, `cursorpreviewfmt`, `tagfmt`,
and `errorfmt` options.

In almost all usecases, the value of these options had to end with `%s\033[0m`
to print the contents of filename/tag/error and reset the terminal style. Now,
if `%s` is not part of the option's value, `%s\033[0m` is appended
automatically. This simplifies configuration.

I retained the same behavior as before when `%s` is part of the string for
backwards compatibility. I think it should be considered deprecated, but it
also causes no harm.
@laktak
Copy link
Contributor

laktak commented Jan 28, 2023

Thanks for working on this!

Could you maybe change the name of the feature though? Calling the highlight a cursor is a bit confusing.

@ilyagr
Copy link
Collaborator Author

ilyagr commented Jan 28, 2023

A "cursor" is the best name I could think of, and it makes a lot of sense to me. I think of each dir having a cursor in it, one of which is active. I could rename cursorfmt to cursoractivefmt if that makes things clearer (this would probably also mean creating cursorparentfmt, since the cursors in parent directories are not active. I was thinking of doing that either way, though they would probably default to being the same as the regular cursor).

When I hear "highlight", I think of highlighting text (tag highlight, "select" highlight, yank highlight, mark highlight, ...). So, something like previewhighlightfmt sounds confusing to me. (Also, I want these options to sort next to each other, but that's less important). I'm not sure if you had a better idea than that in mind.

I don't have a very strong opinion here. I guess I'm not convinced my version is particularly confusing, at least among the options I can think of, but if people have better suggestions, please share them!

I also looked into what different parts of lf call this.

It's called a cursor, for example, in 84cf8e1.

In doc.go, it's called the "selection". However, this is a bit confusing, since the select command creates something doc.go also calls a selection, but is completely different. IMO, it would make sense to rename the "selection" to "cursor" in the docs when it's referring to what I would call a cursor, to distinguish it from the selection created with select and <space>.

@laktak
Copy link
Contributor

laktak commented Feb 6, 2023

Then it might be better to invent a new term (target, selector, ...) than to use something that might be confusing.

A cursor in a terminal used to insert text.

@gokcehan gokcehan merged commit 530ab2c into gokcehan:master Feb 11, 2023
@gokcehan
Copy link
Owner

@ilyagr Thanks for the patch. I'm merging the patch as it is but I agree with @laktak that the name cursor can be confusing. I can't think of anything better at the moment though. Feel free to suggest alternative names here or drop a PR for the name change later on.

@ilyagr
Copy link
Collaborator Author

ilyagr commented Feb 11, 2023

Yeah, I'm at a loss. In addition to @laktak 's suggestions, I can think of "point", but all of these seem worse than "cursor" to me. Of these three, "selector" sounds best to me, but it is such an unusual term.

P.S. Also, @gokcehan , thanks for merging this and the other PRs.

@ilyagr ilyagr deleted the cursorfmt branch February 12, 2023 06:39
ilyagr added a commit to ilyagr/lf that referenced this pull request Feb 12, 2023
This follows up on gokcehan#1086.

The option defaults to the same value as `cursorfmt`, so the default look of
`lf` is unchanged. However, I find it useful to make it different, since I seem
to have so much trouble figuring out which of my cursors is the active one. :)
@laktak
Copy link
Contributor

laktak commented Feb 12, 2023

selector sounds great! 👍

@ilyagr
Copy link
Collaborator Author

ilyagr commented Feb 14, 2023

On second thought, selector has a problem: "selection" already means something different from the cursor. So, "selector" would probably be the little magenta tag that marks the selection (as opposed to the thing that marks where the cursor is).

Also, do I understand correctly that the problem with "cursor" is that it can be mixed up with the cursor in the command line (after pressing : and the like)? Maybe sometimes calling them "file cursor" vs "command line cursor" would be good enough to disambiguate?

Finally, another thought is "current position", shortened to "curpos" in option names. It's quite awkward, but might be workable, I'm not sure.

@ilyagr
Copy link
Collaborator Author

ilyagr commented May 13, 2023

In #1234, @joelim-work had the idea of using focus instead of cursor. I'm not 100% sure I like it better for the purpose of this discussion (does focuspreviewfmt make sense? should it be focusedpreviewfilefmt? Is it worth changing the name at this point?), but I think it's an inspired choice worth pondering.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants