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

Main Column Line Numbers Only #1751

Closed
jbytes1027 opened this issue Jun 19, 2024 · 8 comments · Fixed by #1789
Closed

Main Column Line Numbers Only #1751

jbytes1027 opened this issue Jun 19, 2024 · 8 comments · Fixed by #1789

Comments

@jbytes1027
Copy link

When enabled, line numbers are in the left, center, and right columns:
image

However, I only use the main column line numbers to jump around with vim keybindings quicker. The other two columns of line numbers are unused, add visual clutter to parse, and shrink the usable width. An option is needed to only enable line numbers for the main column.

As an aside, this is in line with how ranger handles line numbers:
image

@joelim-work
Copy link
Collaborator

Line numbers was requested in #130 and implemented in #133. I couldn't find any mention in the discussions about limiting the display to the main window, so I guess it was never considered at all.

Actually this isn't hard to achieve, it should just be a one line change:

diff --git a/ui.go b/ui.go
index f7684ec..7b9d3cb 100644
--- a/ui.go
+++ b/ui.go
@@ -385,21 +385,21 @@ func (win *win) printDir(ui *ui, dir *dir, context *dirContext, dirStyle *dirSty
 
 	beg := max(dir.ind-dir.pos, 0)
 	end := min(beg+win.h, len(dir.files))
 
 	if beg > end {
 		return
 	}
 
 	var lnwidth int
 
-	if gOpts.number || gOpts.relativenumber {
+	if (gOpts.number || gOpts.relativenumber) && dirStyle.role == Active {
 		lnwidth = 1
 		if gOpts.number && gOpts.relativenumber {
 			lnwidth++
 		}
 		for j := 10; j <= len(dir.files); j *= 10 {
 			lnwidth++
 		}
 	}
 
 	for i, f := range dir.files[beg:end] {

The main problem is deciding on how this can be configured by users. Which one of the following do you prefer?

Solution Description
Change number/relativenumber to display line numbers on the main window only This is a breaking change, and some users may not like this. However since I don't really use this feature much, I don't know which one is more preferred and I am probably indifferent about this anyway.
Change number from a boolean option to a string (e.g. all/active/off) This may end up being slightly unintuitive because the corresponding option in Vim is boolean. It is also a breaking change since users who have set number true will have the change it to set number all to maintain the existing behavior.
Add another boolean option to control whether line numbers are displayed on all windows or the main window only This won't result in a breaking change, but would introduce another option and there are a lot of them already.

I'm leaning somewhere between the first and third options. Perhaps it's worth leaving this issue open so others can provide their opinions, but in the meantime you can use the above patch if you're willing to build from source.

@jbytes1027
Copy link
Author

Thanks for the patch and detailed response.

My vote would be option 1. I don't understand the need for line numbers in columns 1 or 3. Maybe for visual consistency? Option 3 works but feels oddly hyper specific of an option.

@joelim-work
Copy link
Collaborator

After thinking about this a bit more, I sort of want to avoid adding too many new options (especially for something small like this) since lf has a lot of options already. The problem with UI configuration in general is that every user has their own preference for how things should be displayed and it's practically impossible to satisfy everyone 100% without adding a heap of options.

I think what we can do is go with the first option, and then revisit this if there are users out there who want line numbers for every window.

For reference, I tried joshuto (set the line_number_style option) and it also displays line numbers on the main window only.

@joelim-work
Copy link
Collaborator

I've submitted a PR for this #1752

@jbytes1027
Copy link
Author

Fixed via PR #1752

@joelim-work joelim-work reopened this Aug 9, 2024
@joelim-work
Copy link
Collaborator

So just an update on this, the PR #1752 was closed because I deleted my fork to test something unrelated.

Although I rarely use line numbers myself, I have so far been reluctant to make any breaking changes without providing a way to keep the original behavior. Especially for anything relating to the UI display, which tends to be very subjective and based on user preferences. I am still a bit concerned that making this kind of change will just lead to someone else raising an issue to request it being reverted.

At the same time I am not sure whether it is a good idea to add more configuration options for minor things like this. I looked over the documentation just now and found that there are already a large number of options purely for configuring the display:

  • borderfmt
  • copyfmt
  • cursoractivefmt
  • cursorparentfmt
  • cursorpreviewfmt
  • cutfmt
  • drawbox
  • errorfmt
  • icons
  • info
  • infotimefmtnew
  • infotimefmtold
  • numberfmt
  • preview
  • promptfmt
  • ratios
  • roundbox
  • rulerfmt
  • selectfmt
  • statfmt
  • tagfmt
  • timefmt
  • truncatechar
  • truncatepct

At some point it becomes overwhelming for both users and maintainers. Going off on a tangent, I can see a possibility that someone in future will request the ability to separately color different types of line numbers (i.e. CursorLineNr, LineNrAbove and LineNrBelow in Vim).


Anyway to address this particular issue, since multiple users have now requested this, I think it is probably better to proceed with this change. However if there is negative feedback from other users, then I will have to revert it until there is a better solution.

@masroof-maindak
Copy link

I for one discovered this option by complete accident while going through the manpage, and my first thought was wanting the numbers in only the central column - partially because the others are fairly useless, and partially for the visual clutter.

Of course, if, as you suggested, some negative feedback does arise, then we could revert and reconsider our approach.

@joelim-work
Copy link
Collaborator

I have just merged the change, I think it's the only way we can get any sort of feedback. Let's wait and see what happens.

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