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

refactor(scanner): make the watcher a little easier to reason about #394

Merged
merged 2 commits into from
Oct 24, 2023

Conversation

sentriz
Copy link
Owner

@sentriz sentriz commented Oct 19, 2023

hiya @brian-doherty , i was reading the scanner code again today, and since it had been a while it took me a little while to grasp it again. i'm wondering what you think about these changes that i've found to help a me a bit

  • pass only an "absPath" arg to scanCallback and watchCallback instead of "absPath" and "dir" (musicDir). the callbacks can deduce the root dir themselves, and the two args have always been confusing, even for the original scanCallback
  • delete the watch stuff from the main scanner struct. it seems we don't need watchMap given above^ and the others can be centralised to ExecuteWatch
  • update some names so that its clear we're doing some event batching

thoughts? thanks!

@sentriz sentriz force-pushed the scan-watch-proposal branch from d7abfe2 to 4cc977b Compare October 19, 2023 01:08
@sentriz sentriz changed the title make the scan watcher a little easier to reason about refactor(scanner): make the watcher a little easier to reason about Oct 19, 2023
@sentriz sentriz force-pushed the scan-watch-proposal branch 2 times, most recently from f648578 to 5ba2d8c Compare October 19, 2023 01:18
@brian-doherty
Copy link
Contributor

This mostly makes sense. My concern about dropping the passing of the root dir regards handling of sym links -- you can't deduce the root dir if you've followed a sym link to somewhere else in the file system. I don't use sym links in my library but you've got handling for them so they need to work properly.

@sentriz
Copy link
Owner Author

sentriz commented Oct 20, 2023

oh yeah you're right 👍

@sentriz sentriz force-pushed the scan-watch-proposal branch from 5ba2d8c to aac5785 Compare October 24, 2023 19:10
@sentriz
Copy link
Owner Author

sentriz commented Oct 24, 2023

actually @brian-doherty it seems to be fine, have a look at this

image

when we encounter a symlink, we evaluate it and replace the prefix of the originally scanned path before evaluating

so if we scan a
/mnt/music/artist (sym to external fs)
we evaluate to
/mnt/external/artist
and carry on as if there was never a symlink in the first place
/mnt/music/artist/album

so the reverse mapping of music dirs works fine, as far as i can tell / test

@brian-doherty
Copy link
Contributor

Makes sense, as long as you tested it and it works.

@sentriz sentriz merged commit c947404 into master Oct 24, 2023
1 check passed
@sentriz sentriz deleted the scan-watch-proposal branch October 24, 2023 22:57
@sentriz
Copy link
Owner Author

sentriz commented Oct 24, 2023

did some more testing of symlinks seems to be fine 👍

thanks for your input, and the watcher itself 👍 👍

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.

2 participants