Added statcmd option to run external commands for file stats #1347
Conversation
There was a problem hiding this comment.
The potential lag from running external commands synchronously every time the UI needs to be updated is a concern. This has actually been discussed before when working on customizing the statfmt (#1288) and ruler (#1257), and in both cases we ended up deciding against providing a hook for an external command.
Just out of interest, does using the on-select hook not work sufficiently for you? Adapted from #1288 (comment):
set statfmt ''
cmd on-select %{{
exa -ld --color=always "$f"
}}
If not, then we could keep this PR under consideration, but I am sort of hesitant to add a feature that could potentially be problematic, at least not without marking it as experimental.
|
@joelim-work Thanks for your feedback. Yes the potential lag was also one of my concerns, but I figured the same concern persists with almost every shell based solution. (Like your Thank you for your suggestion with the The problem with your proposed solution is, that I was however able to improve your solution to the following (based on the This works very well in my case and without any problems. (Maybe something like this should be added to the wiki, where it can be found more easily by other people?) I'm fine with this solution, and will therfor close this PR. |
|
@SPFabGerman If there's anything you find unclear about the documentation, you're more than welcome to fix it up and submit a PR. Anyway I have now added your example config as an integration in the wiki, and hopefully other users will be able to benefit from it. Thanks for your help on this! |
|
I have nothing useful to add, though I can't help but leave a comment here. I should say that it was kind of sad to see this discussion. I thought the main purpose of adding @SPFabGerman I'm glad to see you found a solution that works for you, and I hope you don't take offense in me saying this, but I personally find this solution horrifying. Juggling through the list of files could mean spawning hundreds of processes with this solution. At this point, I don't even know why we added a hook for Anyway, it is just me rambling again as I feel more alienated towards |
|
@gokcehan I don't exactly understand what about the solution you find horrifying. Every "feature" that wants to excess external information (information not provided by lf, but instead by an external command) has to rely on spawning shell processes, in some way or another. Therefor every "feature" that wants some external information for potentially every file will spawn a lot of processes, simply by it's very nature. The amount of spawned processes is not really a nature of the specific solution, but inherent to the problem I wanted to solve. The solution I use now is also not really more expansive than my orignal PR (3 vs 2 spawned processes per file). Also there are a lot of other things in lf that could act as mini-forkbombs, if configured incorrectly. For example the preview script or (in parts) the Also a note on performance: It sounds to me as if you think lf is getting more imperformant. I don't think that is the case. The base configuration of lf (so without additional user configuration) is still very fast and in all my time I have used lf, I never had a performance problem. Even with a lot of user configuration, lf remains very fast. One would have to seriously mess up the configuration to notice a performance impact on lf. We have to keep in mind that lf is a GUI / TUI application, and fast enough that the user does not notice an impact in everyday use (so the programm responds immediatly) is generally good enough performance. Any more improvements are more or less unnecessary, as the user will not notice them anyway. |
|
@SPFabGerman I think there is a big conceptual difference between But then again, if there is no problem with spawning a new process to show the status line, why do we bother with I'm aware that the behavior with |
|
@gokcehan I don't think it makes sense to invent a mini-language (as mentioned in #1282 (reply in thread)) just to customize I wouldn't mind revisiting this as it's my fault Please let me know what you would prefer, I would like to address this issue before too many people find out about |
|
BTW I was playing around with this a bit more, a naive implementation of func (ui *ui) loadFileInfo(nav *nav) {
if !nav.init {
return
}
if gOpts.statcmd != "" {
nav.exportFiles()
ui.exportSizes()
exportOpts()
cmd := shellCommand(gOpts.statcmd, nil)
go func() {
out, err := cmd.Output()
if err != nil {
ui.echoerrf("statcmd: %s", err)
return
}
ui.exprChan <- &callExpr{"echo", []string{string(out)}, 1}
}()
return
}
// default implementation below
}This appears to work fine in practice, but theoretically there's a race condition when previewing multiple files in succession, where the thread that finishes last will have its output prevail. The We might have to instead implement this as a separate thread listening over a channel for updates, similar to what is done for custom previews. |
|
OK so I wrote a basic implementation for a Click to show diffdiff --git a/app.go b/app.go
index 57936b4..72e620b 100644
--- a/app.go
+++ b/app.go
@@ -243,6 +243,7 @@ func (app *app) writeHistory() error {
func (app *app) loop() {
go app.nav.previewLoop(app.ui)
go app.nav.dirPreviewLoop(app.ui)
+ go app.ui.statLoop()
var serverChan <-chan expr
if !gSingleMode {
diff --git a/complete.go b/complete.go
index 024da3a..a57fcdf 100644
--- a/complete.go
+++ b/complete.go
@@ -211,6 +211,7 @@ var (
"shellflag",
"shellopts",
"sortby",
+ "statcmd",
"timefmt",
"tempmarks",
"tagfmt",
diff --git a/eval.go b/eval.go
index 863d4f0..cdddda1 100644
--- a/eval.go
+++ b/eval.go
@@ -801,6 +801,8 @@ func (e *setExpr) eval(app *app, args []string) {
}
app.nav.sort()
app.ui.sort()
+ case "statcmd":
+ gOpts.statcmd = e.val
case "tabstop":
n, err := strconv.Atoi(e.val)
if err != nil {
diff --git a/opts.go b/opts.go
index 58c1532..f092de2 100644
--- a/opts.go
+++ b/opts.go
@@ -67,6 +67,7 @@ var gOpts struct {
selmode string
shell string
shellflag string
+ statcmd string
timefmt string
infotimefmtnew string
infotimefmtold string
@@ -128,6 +129,7 @@ func init() {
gOpts.selmode = "all"
gOpts.shell = gDefaultShell
gOpts.shellflag = gDefaultShellFlag
+ gOpts.statcmd = ""
gOpts.timefmt = time.ANSIC
gOpts.infotimefmtnew = "Jan _2 15:04"
gOpts.infotimefmtold = "Jan _2 2006"
diff --git a/ui.go b/ui.go
index 1f8af2c..6299fe3 100644
--- a/ui.go
+++ b/ui.go
@@ -5,6 +5,7 @@ import (
"fmt"
"log"
"os"
+ "os/exec"
"path/filepath"
"sort"
"strconv"
@@ -553,6 +554,11 @@ func getWins(screen tcell.Screen) []*win {
return wins
}
+type statChanMsg struct {
+ cmd string
+ path string
+}
+
type ui struct {
screen tcell.Screen
polling bool
@@ -567,6 +573,7 @@ type ui struct {
keyChan chan string
tevChan chan tcell.Event
evChan chan tcell.Event
+ statChan chan statChanMsg
menuBuf *bytes.Buffer
cmdPrefix string
cmdAccLeft []rune
@@ -594,6 +601,7 @@ func newUI(screen tcell.Screen) *ui {
keyChan: make(chan string, 1000),
tevChan: make(chan tcell.Event, 1000),
evChan: make(chan tcell.Event, 1000),
+ statChan: make(chan statChanMsg, 1000),
styles: parseStyles(),
icons: parseIcons(),
currentFile: "",
@@ -722,6 +730,11 @@ func (ui *ui) loadFileInfo(nav *nav) {
return
}
+ if gOpts.statcmd != "" {
+ ui.statChan <- statChanMsg{gOpts.statcmd, curr.path}
+ return
+ }
+
var linkTarget string
if curr.linkTarget != "" {
linkTarget = " -> " + curr.linkTarget
@@ -737,6 +750,25 @@ func (ui *ui) loadFileInfo(nav *nav) {
linkTarget)
}
+func (ui *ui) statLoop() {
+ for msg := range ui.statChan {
+ loop:
+ for {
+ select {
+ case msg = <-ui.statChan:
+ default:
+ break loop
+ }
+ }
+ out, err := exec.Command(msg.cmd, msg.path).Output()
+ if err != nil {
+ ui.echoerrf("statcmd: %s", err)
+ continue
+ }
+ ui.exprChan <- &callExpr{"echo", []string{string(out)}, 1}
+ }
+}
+
func (ui *ui) drawPromptLine(nav *nav) {
st := tcell.StyleDefault
#!/bin/sh
exa -ld --color=always "$1"It looks like I will also re-open this PR for the time being as this discussion is still in progress. |
|
@joelim-work Sorry for pinging you with this and thanks for having a look at it again. I tried your patch and it seems to work well without any performance issues. There could be a race as you mention but I haven't encountered myself. Maybe the proper way to handle this could be to implement something like the previewer mechanism though it might be an overkill for this. It would also require dealing with the outdated information and I expect there will still be complaints no matter what mechanism we have (e.g. flicker with a loading or empty message vs show outdated information). To be honest, I was just rambling for no reason so don't take me too seriously. Feel free to decide what to do about this yourself. The best option might just be to leave this as it is since there is no real issue here. In retrospect, I should have hold myself to drop a comment since it does not effect me in any way. At this point, I haven't really worked on anything for months so I don't deserve giving feedback. There is certainly not a "fault" in here as you expressed it and we all appreciate your involvement in the project. |
|
@gokcehan On the contrary, I appreciate your feedback very much - even if the feedback is negative, provided that it is truthful and constructive, I would prefer it over feedback that is positive but skates over potential problems. Actually, the approach of not commenting just because it "does not affect me in any way" can lead to degrading the quality of the project in the long term, especially if contributors continue to work on features for their own benefit without considering how it fits into the overall design. In particular with this PR, the suggestion of providing an additional You are correct that the intention of On the topic of race conditions, if you press j and scroll through the files, a process will be spawned for each file. Not only is this inefficient, it also means that whichever process finishes last will have its output prevail. This can be simulated very easily by adding a random cmd on-select &{{
sleep $((RANDOM % 5))
lf -remote "send $id echo \"$(ls -ld --color=always "$f")\""
}}You are correct that it would be better to implement this in a way similar to the previewer, which is what I have already done in #1347 (comment). Given that #!/bin/sh
if target=$(readlink "$1"); then
set "$target"
target="-> $target"
fi
size=$(numfmt --to=si --round=down "$(stat -c '%s' "$1")")
time=$(date -d @"$(stat -c '%Y' "$1")" '+%a %b %e %X %Y')
stat -c "%A %h %U %G $size $time $target" "$1"Delegating the customization to an external command would also solve other issues like #1098 (use binary sizes instead of metric sizes), and even render the @SPFabGerman What are your thoughts on this? I'm not sure who else to ping regarding this, but the more feedback I get the better. |
|
@joelim-work I have taken a look at the patch again and I understand how it works now. Everything seems to be ordered properly using the channel so I don't see any race here. Not only that, but I also see how it avoids running redundant processes, which is very nice. 👏 Just to ping a few more people in case they are interested in the discussion: @Lassebq as the most recent proposer of this feature in #1282 @tex and @og900aero as the participants of discussion in #1330 (If I'm not mistaken, @og900aero 's final comment led to the optional field patch in #1337). |
|
@joelim-work Sorry for the late reply, but I was somewhat busy today. I have tried out your suggested patch and have also noticed no drawbacks. As far as I can tell, it works very much like my own original PR, just utilising background processes instead. I was very supprised that I did not notice any kind of flickering. (That concern was the main reason I originally used a syncronous approach, rather than an asyncronous one.) The main concern that I have with removing the Though I have to admit I don't really see the practical benefit for your solution compared to the |
|
on-select doesn't work on last file in list... |
|
@SPFabGerman You do raise a good point about
It's also possible to have a hybrid design (e.g. To be clear, I don't have a strong opinion on any of the solutions. To me they all have pros and cons, and I think it's not possible to satisfy everyone including both users and maintainers. Regarding Because of the differences in timing between It is also good that you mentioned I was also surprised that there hasn't been much requests for customizing the Either way, I'm not sure whether it's worth considering the design of |
|
Thanks for mentioning the ruler. I am 100% satisfied with user variable and rule with lf_ |
Its not bad, but doesnt good for directory. |
That is on purpose. I often browse a network folders (nfs or sshfs) and I do not want to be slowed down just because I move cursor over some folder (deep folder, many files, it would take ages to compute the sizes). It is kind of balance between usability and slowness. (but I found now that du supports maxdepth so I could avoid use of awk and numfmt) |
Of course. U can use maxdepth with du. But anyway, the fact is that I only use the code I entered when I select a folder or file, because anyway I see the size of the file by default during browsing |
@joelim-work I think maybe you meant |
I prefer to run
exafor the file stats, since I find it's output more visually appealing than anything I could get with thestatfmtoption. Until now however, lf had no built in support to do something like that. (I used a workaround by mapping my movement keys to a %-shell command, which would then display the output fromexain the status line. This however did not work flawlessly in all cases and produced some screen flickering in the status line.)To fix this, I added a new expansion character
%Cto thestatfmtoption, that runs the command in the newstatcmdoption and displays it's output. Therefor I can achieve my desired behaviour by settingstatfmtto%Candstatcmdto my desiredexacommand. But thisstatcmdhas much more use cases, for example to display version control info of the current file.The only downside is, that lf waits for the command to finish, so the user has to ensure that the command finished relatively quickly, as otherwise lf becomes very laggy.