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

Fix slint-viewer --auto-reload for editors which rename and replace files #5094

Merged
merged 1 commit into from
Apr 22, 2024

Conversation

0x6e
Copy link
Contributor

@0x6e 0x6e commented Apr 18, 2024

Some editors may backup a file by renaming it to append ~, then saving the new file contents to a new file. notify-rs/notify#166 explains that a non-recursive watcher on the parent directory is the recommended way to deal with this situation.

Closes: #3641

@CLAassistant
Copy link

CLAassistant commented Apr 18, 2024

CLA assistant check
All committers have signed the CLA.

@ogoffart
Copy link
Member

I think watching the parent directory is too much. If some random files is being created in that directory, we don't want to reload the view.
We only want to reload it if the created file is matching a name for which we depends.

@0x6e
Copy link
Contributor Author

0x6e commented Apr 18, 2024

Tested manually using the attached files.
tests.tar.gz

It wasn't clear to me how best to test this in Rust. I could provide my own fswatch thread, but that would be testing that, rather than the existing thread.

@0x6e
Copy link
Contributor Author

0x6e commented Apr 18, 2024

I think watching the parent directory is too much. If some random files is being created in that directory, we don't want to reload the view. We only want to reload it if the created file is matching a name for which we depends.

I had reservations about this too. I'll see what I can do.

@0x6e
Copy link
Contributor Author

0x6e commented Apr 18, 2024

Updated test scripts:
tests.tar.gz

Copy link
Member

@ogoffart ogoffart left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good.
I added a few comments.

tools/viewer/main.rs Outdated Show resolved Hide resolved
tools/viewer/main.rs Outdated Show resolved Hide resolved
Some editors, such as vim, rename (move) a file to a backup location,
then write the new contents to a new location when the user saves
their changes. notify stops watching the renamed file, and does not
automatically start watching the new file created. Additionally,
slint-viewer attempts to reload before the editor has written the new
file, which causes an error. The file is then never reloaded because
the watcher was lost.

This patch solves the problem by attempting to watch the file again,
if the previous watch failed due to a Generic or PathNotFound error.
Generic is required because this is error type we get on macOS for "No
such file or directory.". We delay the retry by a small timeout to
give the editor a chance to write the new file. Note that this still
results in an error being printed about the missing file.

Tested manually by editing both root .slint file, and .slint files
imported from sub-directories.

Closes: slint-ui#3641
@0x6e 0x6e force-pushed the slint-viewer-auto-reload branch from 9b329d7 to 67a720c Compare April 19, 2024 11:58
@0x6e
Copy link
Contributor Author

0x6e commented Apr 19, 2024

I found that watching the parent directory wasn't sufficient for the first edit, but would work fine for subsequent edits. My test scripts essentially hid this problem. In hind-sight it is possible that this is due to PENDING_EVENTS being greater than zero when the parent dir event was triggered.

However, the new approach to retrying the watch is simpler, and works for all edits I make to the file.

@ogoffart
Copy link
Member

I don't understand anymore how this works.
I wasn't aware that the watch command failled.

My understanding was that we were watching the file foo.slint
but the editor then renames foo.slint to foo.slint~ and since a rename is not a change in the file, the fswatcher don't see that change and continue watching the backup file instead of the new file.
I might have gotten details wrong. But i don't understand why this patch solves the problem.

@0x6e
Copy link
Contributor Author

0x6e commented Apr 19, 2024

My understanding is as follows:

  1. We watch the file foo.slint;
  2. The editor renames foo.slint to foo.slint.swp;
  3. notify sends a Modify(Name(Any)) event for the file, and removes the watcher (Explain how watch does not follow moves or renames notify-rs/notify#166 says removes explicitly, although the documentation is vague);
  4. we attempt to reload foo.slint, but it doesn't exist so ComponentCompiler::build_from_path returns None;
  5. The editor writes the new content to foo.slint;
  6. We don't get any more events because nothing has re-watched the file.

This patch works because thanks to the retry, the events continue as follows:
7. The timer triggers and we attempt to watch foo.slint;
8. notify sends a Modify(Metadata(Any)) event for the file;
9. fswatcher thread triggers a reload, which is successful because slint.foo now exists.

As an alternative to the single shot timer, we could recursively watch the parent directory of slint.foo. We would need to maintain a set of files we care about, and fswatcher needs to make sure it sends events when watched files are re-added.

@ogoffart
Copy link
Member

I see, thanks for the explanations.

@tronical , does it make sense to you?

@0x6e
Copy link
Contributor Author

0x6e commented Apr 19, 2024

The risks of this patch are:

  • the timeout is too short, in which case the file still won't exist and we never get a watch again;
  • we don't receive a Modify(Metadata(Any)) event after watching the file, in which case we wouldn't reload until the next change.

Recursively watching the parent directory would avoid these issues, however it is not necessarily fool proof either. Notify has a number of known problems including reaching the max-files watch limit and failure to receive all events when watching very large directories.

@tronical
Copy link
Member

Sounds like a reasonable workaround to me.

@ogoffart
Copy link
Member

I'm a bit skeptical, but if you say this solves the problem, let's merge it.

@ogoffart ogoffart merged commit 2e9a97b into slint-ui:master Apr 22, 2024
36 checks passed
@0x6e 0x6e deleted the slint-viewer-auto-reload branch April 22, 2024 14:45
@0x6e
Copy link
Contributor Author

0x6e commented Apr 22, 2024

@ogoffart Would you like to see the watch parent approach?

@ogoffart
Copy link
Member

I think it's fine as is, no need to overthink it. If the problem is solved this is good enough.

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.

slint-viewer --auto-reload does not work well with Neovim (and potentially other editors)
4 participants