[WIP] file watching #588
[WIP] file watching #588
Conversation
There was a problem hiding this comment.
Oh, there's a way to get rid of the thread and crossbeam_channel altogether:
https://github.com/notify-rs/notify/blob/main/examples/async_monitor.rs#L8-L20
Turn run into an async fn and use tokio channels: async fn run(notifier: NotifyActor, receiver: tokio::mpsc::Receiver<Message>)
Then spawn this via tokio::spawn instead of a thread. This is how we deal with LSP clients too.
helix-view/src/editor.rs
Outdated
| @@ -52,12 +54,14 @@ impl Default for Config { | |||
| pub struct Editor { | |||
| pub tree: Tree, | |||
| pub documents: SlotMap<DocumentId, Document>, | |||
| pub path_to_doc: FxHashMap<PathBuf, DocumentId>, | |||
There was a problem hiding this comment.
I'd call this documents_by_path and while I think it's useful, you should probably keep the original code which loops through the open documents. This new hashmap doesn't handle possible renames via set_path or set_path + write
helix-view/src/editor.rs
Outdated
| @@ -253,6 +256,11 @@ impl Editor { | |||
|
|
|||
| let id = self.documents.insert(doc); | |||
| self.documents[id].id = id; | |||
| self.watcher | |||
There was a problem hiding this comment.
Now that I think about it, this needs to deal with renaming too, :w somename.txt could set a name for an unnamed file or write an existing file to a new location
helix-view/src/file_watcher.rs
Outdated
|
|
||
| fn new(sender: Sender) -> NotifyActor { | ||
| let (tx, rx) = unbounded(); | ||
| let watcher: RecommendedWatcher = recommended_watcher(move |e| tx.send(e).unwrap()).unwrap(); |
There was a problem hiding this comment.
Return a result and propagate the error here.
There was a problem hiding this comment.
I also noticed that v4 supported debounce, but seems like that's removed in v5?
helix-view/src/file_watcher.rs
Outdated
| } | ||
| } | ||
|
|
||
| fn run(mut self, inbox: crossbeam_channel::Receiver<ActorMessage>) { |
There was a problem hiding this comment.
I think you could merge next_event and run, you construct an enum then immediately deconstruct it.
|
Do we care about the order of the events after they are coalesced? That is why I have |
|
I don't think so, since every event is distinct and handled in isolation. The ordering doesn't matter |
|
How do I reload files? |
|
There's a helix/helix-view/src/document.rs Line 594 in 7560af1 |
| @@ -38,6 +38,8 @@ log = "~0.4" | |||
|
|
|||
| which = "4.2" | |||
|
|
|||
| notify = "=5.0.0-pre.11" # check that it builds on NetBSD before upgrading | |||
There was a problem hiding this comment.
Is there a reason we need to use a pre-release version instead of current stable? (Which I believe is 4.x)
There was a problem hiding this comment.
Last time I looked at it, it seemed like they were supporting both versions and 5 seemed stable enough maybe
| ) -> anyhow::Result<Self> { | ||
| let language_servers = helix_lsp::Registry::new(); | ||
| let (watcher, watcher_receiver) = { | ||
| let (watcher_sender, watcher_receiver) = unbounded_channel(); | ||
| let watcher = NotifyActor::spawn(Box::new(move |e| watcher_sender.send(e).unwrap())) | ||
| .context("Failed to spawn watcher")?; | ||
| (Arc::new(watcher), watcher_receiver) | ||
| }; |
There was a problem hiding this comment.
Unless I'm misunderstanding something, it looks like this API makes it impossible to open an editor unless spawning the file watcher is successful. Presumably, though, we still want to succeed in opening the editor, and just report to the user that file watching isn't active, right? For example, I still want Helix to work even if I'm on a system that doesn't provide the expected notify capabilities.
|
Note: debouncing is going to be built in on v5 as well, it's just not implemented yet: notify-rs/notify#286 |
the-mikedavis
added
the
S-waiting-on-author
|
Superseded by #2653 |
Currently only the file watching actor is implemented. I am not sure where to put it, is
helix-viewthe right place? I also addedrustc_hashbecause it is faster but less secure, but I don't think we need the security here.TODO: