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

linux: Replace current fs::watch with PollWatcher. #11749

Conversation

CharlesChen0823
Copy link
Contributor

@CharlesChen0823 CharlesChen0823 commented May 13, 2024

Follow with #11595, and comment.

  1. Currently, in linux platform, using notify::recommanded_watcher for watch config.json file modify.
  2. When using ctrl-shift-p toggle vim mode, will using NamedTempFile to rename with exsit file, notify will emit Modify and Delete event.
  3. In current notify default impl, Delete event will triggle watcher remove watch file anymore. Only restart Zed will work for config.json modify.
  4. replace with PollWatcher will not cause this problem.

But, there also leave another problem, because linux platform active_window not implement, after modify vim mode, need manual click other tab active vim mode work.

I don't know if this is accept modify.

Release Notes:

  • N/A

@cla-bot cla-bot bot added the cla-signed The user has signed the Contributor License Agreement label May 13, 2024
@CharlesChen0823 CharlesChen0823 marked this pull request as ready for review May 13, 2024 13:07
@maxdeviant maxdeviant changed the title Linux: replace current fs::watch with PollWatcher. Linux: Replace current fs::watch with PollWatcher. May 13, 2024
@maxdeviant maxdeviant changed the title Linux: Replace current fs::watch with PollWatcher. linux: Replace current fs::watch with PollWatcher. May 13, 2024
@ConradIrwin
Copy link
Collaborator

@CharlesChen0823 thanks for investigating this!

I don't think we want to use a poll-watcher if fsnotify is available. Can we need to restart the fsnotify if we observe a delete on the settings file instead?

cc @mikayla-maki our linux guru

@mikayla-maki mikayla-maki self-assigned this May 13, 2024
@mikayla-maki
Copy link
Contributor

mikayla-maki commented May 13, 2024

@ConradIrwin, that's what the code should be doing already! Sounds like there's a bug we need to fix 🙃

@CharlesChen0823, I don't think we want to use a PollWatcher, as it'll be much slower than inotify. It looks like we're already trying to recreate the file watcher here: https://github.com/zed-industries/zed/pull/11749/files#diff-e13638ad3885e643f14ba8cde1f20fb4955f2cf025ee6b4e7bdb33a0c3f00e50R446-R451, how is the existing solution failing in this case?

@mikayla-maki
Copy link
Contributor

Also, going to close this PR to keep things organized. Feel free to make an issue, or another PR, as we'd like to fix this bug :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla-signed The user has signed the Contributor License Agreement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants