Add watch option #1548
Add watch option #1548
Conversation
Docs need to be regenerated, my pandoc version seems to be different than what generated the existing files cause there are a lot of spurious changes.
|
Thanks for working on this feature. There has actually been a few discussions in the past about using filesystem watching instead of polling via the This change is not trivial as it involves adding another library as a dependency, and even if it is hidden behind a I am thinking that it would be better to leave this PR here for a while, to give other users some time to test and provide feedback. I will probably have a look at the actual change in more detail when I can find some time. Regarding the documentation, don't worry about the automatically generated files. Just update |
|
I have tried this out now but it did not work as I expected.
(I think it works if the content is mounted to the new dir) |
Sounds good to me. I hadn't seen #1445, thanks for mentioning that. It looks like the change suggested there (checking all visible files) in combination with uncommenting the special case for
Great, thanks! |
|
Thanks for giving these changes a try @DusanLesan! I realized when you mentioned enabling things in your lfrc that I hadn't actually tested that, so I've fixed one related bug in d3f31d0. If you could pull those changes and try again that would be great! Also, the correct config line is One other thing I should mention, I'm not sure what filesystem you're using to mount your image file, but not all filesystems are supported by fsnotify: https://github.com/fsnotify/fsnotify/#why-dont-notifications-work-with-nfs-smb-fuse-proc-or-sys. If it turns out that your filesystem is one of the ones mentioned there, then I wouldn't expect it to work, and there's not really anything we could do to fix that (other than polling, which is what the |
|
I think I had wrong expectations for this feature. So I was expecting to see a directory update after mounting stuff into it. But fsnotify readme says that subdirectories are not watched so I do not think watch actually covers my use case |
There was a problem hiding this comment.
So regarding the use of filesystem watching via fsnotify, this has been brought up a few times before, and so far @gokcehan has been against the idea of adding it, see issues #73 and #627. Even if this feature is merged, we will still have to keep the existing period option, since fsnotify has some limitations and may not always be appropriate.
I am personally open to the idea of using fsnotify, but this change has to be justified before being merged. I would strongly suggest you to compile a list of scenarios where the current period option fails, and how fsnotify can solve the problem. Even then, we might choose to leave this PR open for a while for interested users to test changes - for instance the PR for Sixel support #1211 was open for a few months before finally being merged.
In the meantime I have left some comments on the code. Thanks once again for your changes, and also thanks to @DusanLesan for helping with the testing.
P.S. I hacked up my own branch when I was learning and playing around with fsnotify: https://github.com/joelim-work/lf/tree/fsnotify Some of the code was copied from your PR, but feel free to take any code from there.
| @@ -457,6 +462,18 @@ func (app *app) loop() { | |||
| app.nav.renew() | |||
| app.ui.loadFile(app, false) | |||
| app.ui.draw(app.nav) | |||
| case ev := <-app.watcherEvents: | |||
| switch ev.Op { | |||
There was a problem hiding this comment.
According to the documentation it is better to use Event.Has instead of direct comparison for checking event types, as it's possible for a single event to have multiple flags set.
| @@ -457,6 +462,18 @@ func (app *app) loop() { | |||
| app.nav.renew() | |||
| app.ui.loadFile(app, false) | |||
| app.ui.draw(app.nav) | |||
| case ev := <-app.watcherEvents: | |||
| switch ev.Op { | |||
| case fsnotify.Chmod, fsnotify.Write: | |||
There was a problem hiding this comment.
I would like to see Write events being handled as well before merging this, otherwise this feature will end up in a half-implemented state that may or may not ever get done. From my testing, I think it is fine to just do the following in response, and renew is not necessary if no files have been added/renamed/deleted:
app.ui.loadFile(app, false)
app.ui.draw(app.nav)| app.ui.draw(app.nav) | ||
| } | ||
| case err := <-app.watcherErrors: | ||
| app.ui.echoerrf("watcher: %s", err) |
There was a problem hiding this comment.
I think it would be better to restrict the use of echoerr/echoerrf to things like invalid input from the user, as this could end up being very annoying. Use log.Printf here instead, which can be viewed when logging is enabled, and same goes for all the other places where the watcher runs into an error.
| @@ -196,6 +196,8 @@ var ( | |||
| "wrapscroll!", | |||
| "findlen", | |||
| "period", | |||
| "watch", | |||
| "nowatch", | |||
There was a problem hiding this comment.
This is a bool option, so please follow the guidelines in #758 to maintain consistency with all the other existing options. You can copy and paste the code from some other option.
| @@ -823,6 +823,10 @@ Note that directories are already updated automatically in many cases. | |||
| This option can be useful when there is an external process changing the displayed directory and you are not doing anything in lf. | |||
| Periodic checks are disabled when the value of this option is set to zero. | |||
|
|
|||
| ## watch (bool) (default false) | |||
|
|
|||
| If enabled, watch visible directories for changes. | |||
There was a problem hiding this comment.
Please mention any limitations here, e.g. lack of support for certain types of filesystems, ignoring chmod events, etc..
Also I think this feature should be labelled as experimental for the time being, as it involves the use of another library. I have tested these changes and it works fine for me, but I am not sure about other platforms.
| break | ||
| } | ||
|
|
||
| watcher, err := fsnotify.NewWatcher() |
There was a problem hiding this comment.
I think the code for creating a new watcher should be moved into a separate function, which you'll probably want to do after implementing set watch!. The same goes for the code for cleaning up the existing watcher.
| @@ -1161,6 +1211,41 @@ func onSelect(app *app) { | |||
| if cmd, ok := gOpts.cmds["on-select"]; ok { | |||
| cmd.eval(app, nil) | |||
| } | |||
| if watcher := app.watcher; watcher != nil { | |||
| dirsSet := map[string]struct{}{} | |||
There was a problem hiding this comment.
Using bool as the value should suffice for sets, see https://go.dev/doc/effective_go#maps. I prefer to follow that recommendation if possible.
| @@ -1161,6 +1211,41 @@ func onSelect(app *app) { | |||
| if cmd, ok := gOpts.cmds["on-select"]; ok { | |||
| cmd.eval(app, nil) | |||
| } | |||
| if watcher := app.watcher; watcher != nil { | |||
There was a problem hiding this comment.
If I understand correctly, this entire block of code determines the desired set of paths to watch and then applies it to the watcher. I think it would be better to extract this into a separate function, which can be called during onSelect, but also when creating the watcher after the user enables the watch option.
| if _, ok := dirsSet[dPath]; ok { | ||
| delete(dirsSet, dPath) | ||
| continue | ||
| } |
There was a problem hiding this comment.
I think this part might not be needed since adding a watch that already exists is a no-op anyway according to the documentation. Though I don't mind too much either way, I also couldn't find a method to simply specify the exact set the paths to watch.
So I've done some testing with mounting stuff myself @DusanLesan. I think the issue is that inotify (the fsnotify backend for linux) doesn't notify about mount events, regardless of whether the directory is watched. In the section about "inotify events" of inotify's manpage, there is no mention of "mount" events of any kind. A possible solution for your case: if you're running a script within lf to trigger the mount, couldn't you just use What the fsnotify README means when it says that subdirectories aren't watched is that when you add a directory, say "foo", you will be notified of changes to "foo" itself or to entries within "foo", such as the creation of a file "foo/bar" or a directory "foo/baz/", but you won't be notified of changes within the subdirectory "foo/baz/", such as the creation of a file "foo/baz/quux". This is ideal for our use-case because we don't want to have to watch the entire filesystem when the user is looking at the root in lf. Instead, we specifically add each directory that is visible, then remove it when it becomes no longer visible, so we get as few events as possible without missing anything that should cause a visual change. So in your case, the directory into which you're mounting things is watched, but the mount action itself isn't something that notifications are sent for. If you created a file in the directory instead of mounting into it, you should see that update. |
Thanks for pointing out those issues, I missed them. After reading though those and thinking about it some more, I'm not super sure if introducing this dependency is really a good idea. The only two pros to fsnotify over period that I can see are:
As for cons of fsnotify:
One alternative I was thinking about was allowing sub-second period times, which would tip the comparison even more in favour of period. Lower period times would of course come at the cost of more CPU usage, but it's not unreasonable even with a patchdiff --git a/doc.md b/doc.md
index 6910c27..4c870b0 100644
--- a/doc.md
+++ b/doc.md
@@ -170,7 +170,7 @@ The following options can be used to customize the behavior of lf:
mouse bool (default false)
number bool (default false)
numberfmt string (default "\033[33m")
- period int (default 0)
+ period float64 (default 0.0)
preserve []string (default "mode")
preview bool (default true)
previewer string (default '')
@@ -820,7 +820,7 @@ When the `relativenumber` option is enabled, only the current line shows the abs
Format string of the position number for each line.
-## period (int) (default 0)
+## period (float64) (default 0.0)
Set the interval in seconds for periodic checks of directory updates.
This works by periodically calling the `load` command.
diff --git a/eval.go b/eval.go
index 3f338eb..df824e6 100644
--- a/eval.go
+++ b/eval.go
@@ -3,6 +3,7 @@ package main
import (
"io"
"log"
+ "math"
"os"
"path/filepath"
"strconv"
@@ -607,13 +608,13 @@ func (e *setExpr) eval(app *app, args []string) {
case "numberfmt":
gOpts.numberfmt = e.val
case "period":
- n, err := strconv.Atoi(e.val)
+ n, err := strconv.ParseFloat(e.val, 64)
if err != nil {
app.ui.echoerrf("period: %s", err)
return
}
- if n < 0 {
- app.ui.echoerr("period: value should be a non-negative number")
+ if !(n >= 0) || math.IsInf(n, 1) {
+ app.ui.echoerr("period: value should be a finite, non-negative number")
return
}
gOpts.period = n
@@ -621,7 +622,7 @@ func (e *setExpr) eval(app *app, args []string) {
app.ticker.Stop()
} else {
app.ticker.Stop()
- app.ticker = time.NewTicker(time.Duration(gOpts.period) * time.Second)
+ app.ticker = time.NewTicker(time.Duration(gOpts.period * float64(time.Second)))
}
case "preview":
if e.val == "" || e.val == "true" {
diff --git a/opts.go b/opts.go
index 6f8054b..142a174 100644
--- a/opts.go
+++ b/opts.go
@@ -73,7 +73,7 @@ var gOpts struct {
wrapscan bool
wrapscroll bool
findlen int
- period int
+ period float64
scrolloff int
tabstop int
errorfmt stringSorry that this reply has become so long... Given the above though, I'm leaning towards closing this PR. Thoughts? Are there any pros or cons I've missed? |
|
@mtoohey31 Regarding mounts, I think what happens is that Anyway, when I was talking about the kind of benefit that Regarding the PR itself, even if we merge this, we would probably have to mark it as experimental, and I don't know whether you'll be around afterwards to help fix any related issues if they come up. I guess it's up to you if you want to close the PR or keep it open, I think either way GitHub will keep the changes so they won't be lost. |
Good point, would it make sense to summarize this as watches being associated with inodes, not paths, at least on Linux? I don't have any ideas about how to handle this either. I don't think there's any way to check if a mount has happened other than repeatedly stat'ing a folder to check its inode, which is what we're trying to avoid with
In its current state, I don't think I've decided to close this for now, because if we want to actually fix any issues with this feature, this will have to be re-written with the alternative approach described above. I might come back to this at some point, but if someone else wants to give it a go, please do! |
|
Using fsnotify like proposed here is something I have been thinking about for quite a while as well and I just wanted to add some thoughts on this: There are quite a lot of cases where fsnotify does not work perfectly for a use case such as this. One important case I could add to those listed here is remote file systems. Personally I use lf on servers a lot and most file systems/partitions there are actually on other hosts, which means fsnotify will not work for those cases. In addition, using fsnotify would still require this additional dependency to provide an optional feature, still without catching all use cases. |
This merge request adds a
watchoption which leverages https://github.com/fsnotify/fsnotify to automatically update the UI when visible files change. It only tracksnav.dirsandnav.currFileif it's a directory so it should be pretty lightweight. fsnotify supports a wide variety of platforms, so we shouldn't have any compatibility issues, though I haven't tested this on all of them. The option is disabled by default.This PR doesn't handle chmod or write notifications. Adding support for those could be a future improvement. Doing so would require more than just removing the separate case for those events in
app.loopthough, causenav.renewdoesn't catch those changes, so we'd have to add extra logic to update file modes/sizes.As mentioned in the commit message, I haven't regenerated the other doc files aside from
doc.mdcause there seems to be something different about my local pandoc version that causes a bunch of extra changes. Not sure what's going on there...Maybe closes #1449? We might want to wait for chmod and write too before we close that though.