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

Add statfmt option #1288

Merged
merged 7 commits into from Jun 9, 2023
Merged

Add statfmt option #1288

merged 7 commits into from Jun 9, 2023

Conversation

joelim-work
Copy link
Collaborator

@joelim-work joelim-work commented Jun 5, 2023

Add a new fileinfofmt statfmt option to customize the file info in the bottom left corner.

In addition, I changed the default behaviour so that the file permissions is colored cyan to match the behaviour or ranger, this is a breaking change. Let me know if you don't want this and I'm happy to undo the color change.


Example: set statfmt "\033[7;31m %p \033[32m %c \033[33m %u %g \033[34m %s \033[35m %t \033[36m %l \033[0m"

image

doc.go Outdated Show resolved Hide resolved
os.go Show resolved Hide resolved
os.go Outdated Show resolved Hide resolved
Copy link
Collaborator

@ilyagr ilyagr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm still on the fence about changing the default, but I guess we can just undo it if people complain.

Thanks!

@gokcehan
Copy link
Owner

gokcehan commented Jun 8, 2023

Thanks @joelim-work for the patch and thanks @ilyagr for the feedback. A few points below:

  1. I'm not sure about the option name fileinfofmt. So far, we have been using the name "info" to refer to the information at the right side of file names (i.e. info, infotimefmtnew, infotimefmtold). I think I have been internally calling the bottom left part "stat" myself, because the information shown here mostly comes from the stat call. For example, I had considered renaming timefmt to stattimefmt when infotimefmtnew/old options were added but I did not want to make a breaking change. What do you feel about the name statfmt for this option? An alternative might also be statusfmt as I think it is a common name in other programs as well (e.g. statusline for vim). There is a little confusion since I was also calling the line at the bottom statline or statusline throughout the documentation when it is used for other purposes (e.g. messages or prompts), though I think we can update these words if necessary. To sum up, I feel like we should have a consensus on the terminology in general before naming this option.
  2. Do we have a standard way to decide whether options should be added as field options or format strings? I feel like, we are currently building up some inconsistency within the options. For example promptfmt and this option is added as a format string but ruler is a field option. Sometimes there is a reason for using a field option. For example, info is a field option because we don't want any color customization as we want to make use of the color of the corresponding file. I can't say if there was a reason to use a field option for ruler (e.g. maybe it should have been rulerfmt). This discussion also somewhat related to Improve API for parsing escape sequences #1264 . I feel like the name "fmt" or "format" suggests some kind of replacement as in the printf-family functions so maybe the additions of short-hand forms was already a mistake in the first place.
  3. For the documentation formatting, so far I have been simply writing such format expansions as regular text (e.g. see promptfmt). I think it is better to save code blocks for the names of commands/options so they would look distinctive in the online version of the document. Is there a reason why we can't use bullet lists for these expansions? (e.g. like numbered lists in "Colors" section of the documentation but with - instead of numbers with a space before and after it). I'm also not happy with some of the things in our documentation system, though I agree it belongs to a separate discussion elsewhere.
  4. I'm slightly worried that there wasn't any further feedback from @Lassebq after the proposal as I'm not sure if the current version of this patch is in line with the expectations. Do we have an actual potential user of this option at this point?

@Lassebq
Copy link

Lassebq commented Jun 8, 2023

Thanks @joelim-work for the patch and thanks @ilyagr for the feedback. A few points below:

  1. I'm not sure about the option name fileinfofmt. So far, we have been using the name "info" to refer to the information at the right side of file names (i.e. info, infotimefmtnew, infotimefmtold). I think I have been internally calling the bottom left part "stat" myself, because the information shown here mostly comes from the stat call. For example, I had considered renaming timefmt to stattimefmt when infotimefmtnew/old options were added but I did not want to make a breaking change. What do you feel about the name statfmt for this option? An alternative might also be statusfmt as I think it is a common name in other programs as well (e.g. statusline for vim). There is a little confusion since I was also calling the line at the bottom statline or statusline throughout the documentation when it is used for other purposes (e.g. messages or prompts), though I think we can update these words if necessary. To sum up, I feel like we should have a consensus on the terminology in general before naming this option.
  2. Do we have a standard way to decide whether options should be added as field options or format strings? I feel like, we are currently building up some inconsistency within the options. For example promptfmt and this option is added as a format string but ruler is a field option. Sometimes there is a reason for using a field option. For example, info is a field option because we don't want any color customization as we want to make use of the color of the corresponding file. I can't say if there was a reason to use a field option for ruler (e.g. maybe it should have been rulerfmt). This discussion also somewhat related to Improve API for parsing escape sequences #1264 . I feel like the name "fmt" or "format" suggests some kind of replacement as in the printf-family functions so maybe the additions of short-hand forms was already a mistake in the first place.
  3. For the documentation formatting, so far I have been simply writing such format expansions as regular text (e.g. see promptfmt). I think it is better to save code blocks for the names of commands/options so they would look distinctive in the online version of the document. Is there a reason why we can't use bullet lists for these expansions? (e.g. like numbered lists in "Colors" section of the documentation but with - instead of numbers with a space before and after it). I'm also not happy with some of the things in our documentation system, though I agree it belongs to a separate discussion elsewhere.
  4. I'm slightly worried that there wasn't any further feedback from @Lassebq after the proposal as I'm not sure if the current version of this patch is in line with the expectations. Do we have an actual potential user of this option at this point?

Yes, I'm perfectly fine with this approach, although initially I wanted it to be displayed the same way as :shell-pipe works, i. e. the info string is set to the output of a shell script. I'm not sure if there were any issues with this approach, but whatever works best, any coloring is good to have.

@gokcehan
Copy link
Owner

gokcehan commented Jun 8, 2023

Yes, I'm perfectly fine with this approach, although initially I wanted it to be displayed the same way as :shell-pipe works, i. e. the info string is set to the output of a shell script. I'm not sure if there were any issues with this approach, but whatever works best, any coloring is good to have.

@Lassebq This might already be possible with the use of on-select command. For example, the following comes up with a visual close to the original example (except for the link part which should also be possible with some additions):

cmd on-select %{{
    ls=$(ls -l $f)
    p=$(echo $ls | awk '{ print $1 }')
    c=$(echo $ls | awk '{ print $2 }')
    u=$(echo $ls | awk '{ print $3 }')
    g=$(echo $ls | awk '{ print $4 }')
    s=$(echo $ls | awk '{ print $5 }')
    t=$(echo $ls | awk '{ print $6,$7,$8 }')
    printf "\033[7;31m $p \033[32m $c \033[33m $u $g \033[34m $s \033[35m $t \033[0m"
}}

Though I don't think this is usable in practice as it flickers at each change. This is why you usually want to have these things as builtins.

@joelim-work
Copy link
Collaborator Author

joelim-work commented Jun 9, 2023

@gokcehan To answer some of your points:

  1. I have changed the option name to statfmt as you have suggested. I do think the names are a bit confusing in the code, but I think its less of a concern because it's internal and can be easily changed. It's more important to establish good names for the configuration options as they are public, and changing them would be significantly more difficult if not impossible.
  2. The ruler option was added in Customizable statusline, df #1168 and I didn't involve myself in it. Maybe I should have intervened and advocated for a format string style option (i.e. rulerfmt), but at the time I was still a relatively new contributor and I didn't want to dictate other people's PRs. I'm not 100% satisfied with the current implementation of ruler, because if statfmt gets added, I can easily that predict users will request the option of styling ruler components, which is currently not supported. Regarding the shorthand forms discussed in Improve API for parsing escape sequences #1264, I think they make sense for UI components where only a style definition is required and not a format string (e.g. tagfmt/numberfmt/borderfmt), but I believe this should not have been applied to errorfmt (here is an example of a user performing slightly more complex formatting on the error message). I think the discussion of shorthand forms isn't relevant to this particular PR, but I can reopen the other PR if you wish to discuss it further.
  3. I have changed the documentation to use plain text formatting - I pretty much copied off the promptfmt option. The problem with lists (both ordered and unordered) is that they appear as code blocks in the man pages. I am also not 100% satisfied with the documentation system (for example the Arch Linux website now hosts man pages and the entry for lf is quite, well, interesting), but I don't know if there's any easy solution since you want the same document to be formatted reasonably on several different platforms. And I think this PR is not the right place to discuss this matter - maybe Documentation is inconsistent with what comes first: command or explanation #1018 would be a better place.

For the purposes of this PR, I have made the changes you requested, please let me know if there is anything else.

@joelim-work joelim-work changed the title Add fileinfofmt option Add statmt option Jun 9, 2023
@joelim-work joelim-work changed the title Add statmt option Add statfmt option Jun 9, 2023
@gokcehan
Copy link
Owner

gokcehan commented Jun 9, 2023

@joelim-work It looks good, thanks again for the patch.

@gokcehan gokcehan merged commit 2ab4b14 into gokcehan:master Jun 9, 2023
3 checks passed
@joelim-work joelim-work deleted the fileinfofmt branch June 10, 2023 01:16
gokcehan pushed a commit that referenced this pull request Jul 3, 2023
…-arm64 (#1298)

* Create CI job to run `gen/xbuild.sh`, adjust `xbuild.sh` to make it possible

This reworks the gen/xbuild.sh script to exit with a failure if any of the targets don't compile. It also adds a CI job to try cross-compiling for every platform on every pull request.

The new check is a little slow and compute-expensive, but I am hoping it's well within the quota.

See #1299 for a demo of how the CI would have failed after #1288 and before #1295.

Screenshot: https://github.com/gokcehan/lf/assets/4123047/3d7a6b7c-8642-4d4c-a98a-c8b886c0b184

Thanks to @joelim-work for numerous improvements and an alternate
implementation I heavily borrowed from
(https://github.com/joelim-work/lf/blob/ci/gen/xbuild.sh)

* Refactor xbuild.sh some more to decrease repetition

I can remove this commit if people prefer.

* Generate darwin-arm64 binaries as we have users using it

Arm Macs are becoming popular. We have at least one
user: #1294. It
seems lf works fine there.
@gokcehan
Copy link
Owner

@joelim-work I realized with the new statfmt option, there is a slight change in the widths of humanized sizes. Before, humanized form always used to be replaced as a string with a width of 4 so different stats used to show up as follows:

-rw-r--r-- 1 gokcehan gokcehan 2.3K Wed Jul  5 18:25:50 2023
-rw-r--r-- 1 gokcehan gokcehan  12K Sun Aug 27 19:07:38 2023
-rw-r--r-- 1 gokcehan gokcehan 360B Sat May 13 13:29:51 2023
...

With the new statfmt default, it shows up as follows:

-rw-r--r-- 1 gokcehan gokcehan 2.3K Wed Jul  5 18:25:50 2023
-rw-r--r-- 1 gokcehan gokcehan 12K Sun Aug 27 19:07:38 2023
-rw-r--r-- 1 gokcehan gokcehan 360B Sat May 13 13:29:51 2023
...

So it results in a slight misalignment for the size and time. It's not too important, but I think it would be better to keep the alignment. Sometimes it can be useful when you are looking for a difference (e.g. year) by juggling through the file list. I'm aware, it would still not be a perfect alignment since hardlink counts and user/group infos can also be different, though such differences are often rare in practice within the same directory.

If you agree, an easy fix is to do this directly in humanize function by using %3d instead of %d as follows:

diff --git a/misc.go b/misc.go
index 8199697..f5b0ae5 100644
--- a/misc.go
+++ b/misc.go
@@ -223,7 +223,7 @@ func readPairs(r io.Reader) ([][]string, error) {
 // This should be fine for most human beings.
 func humanize(size int64) string {
        if size < 1000 {
-               return fmt.Sprintf("%dB", size)
+               return fmt.Sprintf("%3dB", size)
        }

        suffix := []string{
@@ -242,7 +242,7 @@ func humanize(size int64) string {
                if curr < 10 {
                        return fmt.Sprintf("%.1f%s", curr-0.0499, s)
                } else if curr < 1000 {
-                       return fmt.Sprintf("%d%s", int(curr), s)
+                       return fmt.Sprintf("%3d%s", int(curr), s)
                }
                curr /= 1000
        }

@joelim-work
Copy link
Collaborator Author

Hi @gokcehan changing the size to have a width of 4 sounds to maintain alignment sounds fine to me. However I would suggest not changing the humanize function and instead make the change at the UI level:

diff --git a/ui.go b/ui.go
index 7abe1a7..0520d48 100644
--- a/ui.go
+++ b/ui.go
@@ -742,7 +742,7 @@ func (ui *ui) loadFileInfo(nav *nav) {
 	replace("%c", linkCount(curr))
 	replace("%u", userName(curr))
 	replace("%g", groupName(curr))
-	replace("%s", humanize(curr.Size()))
+	replace("%s", fmt.Sprintf("%4s", humanize(curr.Size())))
 	replace("%t", curr.ModTime().Format(gOpts.timefmt))
 	replace("%l", curr.linkTarget)

Do you think it's better to apply this change to the %s placeholder, or create a new placeholder (e.g. %S) that does the same but with the width of 4? Either way I'm happy to make width of 4 the default since that was the original behavior before statfmt was added.

@gokcehan
Copy link
Owner

@joelim-work Changing it at the UI level sounds perfectly fine. I also don't mind to see a separate placeholder for this. I agree the default should be the same as before.

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

Successfully merging this pull request may close these issues.

default color bottom line / file info
4 participants