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

Support optional fields for statfmt option #1337

Merged
merged 3 commits into from
Jul 17, 2023

Conversation

joelim-work
Copy link
Collaborator

@joelim-work joelim-work commented Jul 8, 2023

For the statfmt option, I finally found a way to support display text depending on whether a field exists or not. The idea is to divide the format string into sections, and if any section contains a failed expansion (blank string), then the section is discarded and not shown.

Benefits:

  • There is no need to have separate placeholders for the link target (%l), and the link target with an arrow (%L).
  • There is no need to have a separate default for Windows just because some fields don't exist.
  • Formatting/spacing can be 'tied' to a field, and if it doesn't exist then it won't show up in the final result.

In the example given in #1288, the \033[36m %l \033[0m part for displaying the link target would show as two cyan spaces if there wasn't one, which is not ideal. With optional fields supported, this can now work:

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

There are a couple of limitations now, though I don't think it will matter in practice:

  • The | character cannot appear in the format string, as it now has a special meaning.
  • Results of expansions now cannot contain a null byte (0x00) or a unit separator byte (0x1f) as they now have a special meaning (failed expansion, and field delimiter, respectively).

@joelim-work joelim-work marked this pull request as ready for review July 8, 2023 14:11
@gokcehan
Copy link
Owner

@joelim-work I don't have a strong opinion about this as I don't use this myself so feel free to merge this if you like. However, my two cents is that I'm not sure about the | character as the separator. Isn't this a common character to separate fields in status lines? (e.g. i3status) Have you considered other alternatives? On the top of my head, maybe : (to be more consistent with the rest of our options) or \t (similar to tsv files) could be used instead?

@joelim-work
Copy link
Collaborator Author

joelim-work commented Jul 12, 2023

@gokcehan I guess your concern is fair, the irony is that I chose | as a separator because it is commonly used for separating fields. In particular I wanted to have the statfmt option remain fairly readable. I don't mind changing the separator to something else though, below are some alternatives for comparison:

set statfmt "\033[36m%p\033[0m| %c| %u| %g| %s| %t| -> %l"

set statfmt "\033[36m%p\033[0m|| %c|| %u|| %g|| %s|| %t|| -> %l"

set statfmt "\033[36m%p\033[0m: %c: %u: %g: %s: %t: -> %l"

set statfmt "\033[36m%p\033[0m\t %c\t %u\t %g\t %s\t %t\t -> %l"

Another alternative is to provide an escape character, for example %| to represent a literal |.


I'm not sure how useful the character | is for visually separating fields, in my opinion have spaces and different colors/styles do a much better job. So my personal preference for the implementation is:

  1. Use | as a separator, and provide %| to encode a literal | for users that require it
  2. Use || as a separator, as it is doesn't hurt readability by much, and is less likely to be required by users
  3. Use another character like : or \t as a separator

I'm not sure of how to get feedback on this, we could either merge this and see what happens, or post this as a discussion first.

@gokcehan
Copy link
Owner

@joelim-work I guess I agree it wouldn't be an issue most of the time. So feel free to merge this whenever you feel like. You don't even need to add the literal escape (i.e. %|) at this point unless someone explicitly asks for it.

@joelim-work
Copy link
Collaborator Author

@gokcehan Thank, I wanted to get this change in before statfmt was officially released, otherwise it would be more difficult to change later on.

I agree that it would be better to just keep the current implementation for now, and only add %| if it becomes necessary. For reference, the implementation would look like this (just have to add a couple more replace calls):

	statfmt := gOpts.statfmt
	statfmt = strings.ReplaceAll(statfmt, "%|", "\x1a")
	statfmt = strings.ReplaceAll(statfmt, "|", "\x1f")
	replace := func(s string, val string) {
		if val == "" {
			val = "\x00"
		}
		statfmt = strings.ReplaceAll(statfmt, s, val)
	}
	replace("%p", curr.Mode().String())
	replace("%c", linkCount(curr))
	replace("%u", userName(curr))
	replace("%g", groupName(curr))
	replace("%s", humanize(curr.Size()))
	replace("%t", curr.ModTime().Format(gOpts.timefmt))
	replace("%l", curr.linkTarget)

	fileInfo := ""
	for _, section := range strings.Split(statfmt, "\x1f") {
		if !strings.Contains(section, "\x00") {
			fileInfo += section
		}
	}
	fileInfo = strings.ReplaceAll(fileInfo, "\x1a", "|")

	ui.echo(fileInfo)

@joelim-work joelim-work merged commit 23418ad into gokcehan:master Jul 17, 2023
4 checks passed
@joelim-work joelim-work deleted the statfmt-optional-fields branch July 17, 2023 01:07
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.

None yet

2 participants