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

Add File Watcher #8871

Merged
merged 7 commits into from
Feb 26, 2024
Merged

Add File Watcher #8871

merged 7 commits into from
Feb 26, 2024

Conversation

kapitanluffy
Copy link
Contributor

  • I'm the package's author and/or maintainer.
  • I have have read the docs.
  • I have tagged a release with a semver version number.
  • My package repo has a description and a README describing what it's for and how to use it.
  • My package doesn't add context menu entries. *
  • My package doesn't add key bindings. **
  • Any commands are available via the command palette.
  • Preferences and keybindings (if any) are listed in the menu and the command palette, and open in split view.
  • If my package is a syntax it doesn't also add a color scheme. ***
  • If my package is a syntax it is named after the language it supports (without suffixes like "syntax" or "highlighting").
  • I use .gitattributes to exclude files from the package: images, test files, sublime-project/workspace.

My package is File Watcher

There are no packages like it in Package Control.

@kapitanluffy kapitanluffy marked this pull request as draft February 13, 2024 18:50
@kapitanluffy kapitanluffy marked this pull request as ready for review February 13, 2024 18:50
@kapitanluffy kapitanluffy reopened this Feb 13, 2024
Copy link
Collaborator

@packagecontrol-bot packagecontrol-bot left a comment

Choose a reason for hiding this comment

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

Automated testing result: SUCCESS

Repo link: File Watcher

Packages added:
  - File Watcher

Processing package "File Watcher"
  - All checks passed

@rchl
Copy link
Contributor

rchl commented Feb 13, 2024

You are missing "install.txt" file.

@FichteFoll
Copy link
Collaborator

FichteFoll commented Feb 14, 2024

A few quick things I happened to notice while taking a look:

  1. As @rchl mentioned, you reference install.txt in your messages.json, but it doesn't exist.
  2. Your package won't reload cleanly after an update because submodules are not cleared from the module cache automatically and thus not reloaded. You can either do something like PackageDev and manually reload them or you can add a notice in your update message that strongly suggests to restart ST to properly use the updated code.
  3. I suspect your FileEventListener class is supposed to serve as something consumers of the package can subclass to get their on_create etc event hooks called? It's currently not documented like that in the readme. In fact, the way that you use window commands to broadcast events seems more like an implementation detail (that you should mention in case someone decides to override the method, but otherwise seems irrelevant).
    In that case, I strongly suggest to use an importable name for your package, i.e. FileWatcher instead of File Watcher as that allows using the import statement.
  4. Besides, since it is imported into the main plugin module, this example class will also be loaded by ST but not actually have an effect.
  5. You should mention in the readme (i.e. somewhere not inside your source code) that changes to projects are not automatically detected.
  6. The Python 3.3 host does not have the typing module by default. You need to declare it as a required dependencylibrary.
  7. There are quite a few odd pass statements at the end of function bodies. That's obviously not a bad thing since they do nothing, but their presence is weird.

Copy link
Collaborator

@packagecontrol-bot packagecontrol-bot left a comment

Choose a reason for hiding this comment

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

Automated testing result: SUCCESS

Repo link: FileWatcher

Packages added:
  - FileWatcher

Processing package "FileWatcher"
  - All checks passed

@kapitanluffy
Copy link
Contributor Author

@FichteFoll

As @rchl mentioned, you reference install.txt in your messages.json, but it doesn't exist.

Used README.md, would that work?

Your package won't reload cleanly after an update because submodules are not cleared from the module cache automatically and thus not reloaded. You can either do something like PackageDev and manually reload them or you can add a notice in your update message that strongly suggests to restart ST to properly use the updated code.

Copied it. I wonder how it works with subfolders though. Should I do this to my other packages as well

In that case, I strongly suggest to use an importable name for your package, i.e. FileWatcher instead of File Watcher as that allows using the import statement.

The Python 3.3 host does not have the typing module by default. You need to declare it as a required dependencylibrary.

There are quite a few odd pass statements at the end of function bodies. That's obviously not a bad thing since they do nothing, but their presence is weird.

👍

I suspect your FileEventListener class is supposed to serve as something consumers of the package can subclass...

Besides, since it is imported into the main plugin module, this example class will also be loaded by ST but not actually have an effect.

Removed it. Subclassing doesn't work anyway

You should mention in the readme (i.e. somewhere not inside your source code) that changes to projects are not automatically detected.

Updated the README

@rchl
Copy link
Contributor

rchl commented Feb 14, 2024

You are mentioning that it requires LSP-file-watcher-chokidar but it also requires LSP since the former imports from the latter.

@rchl
Copy link
Contributor

rchl commented Feb 14, 2024

Used README.md, would that work?

The contents will be inserted inside plain text document so don't expect markdown syntax highlighting but otherwise it's fine.

@kapitanluffy
Copy link
Contributor Author

The context will be inserted inside plain text document so don't expect markdown syntax highlighting but otherwise it's fine.

Yeah that should be fine

You are mentioning that it requires LSP-file-watcher-chokidar but it also requires LSP since the former imports from the latter.

Updated it 👍

@rchl
Copy link
Contributor

rchl commented Feb 15, 2024

Actually there is something pretty bad about design of this package. Once you have it installed, it will set up watcher for every project you open, even if nothing actually needs or listens to those events.

Creating watchers consumes file descriptors and also causes extra CPU and memory usage.

@kapitanluffy
Copy link
Contributor Author

kapitanluffy commented Feb 16, 2024

Actually there is something pretty bad about design of this package. Once you have it installed, it will set up watcher for every project you open, even if nothing actually needs or listens to those events.

Creating watchers consumes file descriptors and also causes extra CPU and memory usage.

Is there a way to pass multiple folders to chokidar so that it only creates 1 watcher? I just depend on what I can see from watcher.py.

I wonder how chokidar handles it though since from what I can see here, if I open multiple projects (ProjectA and ProjectB), won't it do the same thing (have watchers for each)? Or does it only register 1 watcher at a time and terminate it when focusing on a different project/window?

Anyway, I fully understand. If performance will be a concern, I do not mind closing the PR. I just wanted to share it since it might help other plugin developers too. So far, I am just using it in a feature in CompassNavigator

@rchl
Copy link
Contributor

rchl commented Feb 16, 2024

Is there a way to pass multiple folders to chokidar so that it only creates 1 watcher? I just depend on what I can see from watcher.py.

It always creates a single watcher process at most but it matters how many directories are watched (how big the tree is). Watching your whole home directory will consume a lot more resources than watching 10 small directories.

LSP servers can decide which files/directories to watch using a pattern. Some only watch specific configuration files, some watch whole projects. But still this applies only to project in which that server runs, not to every random project/folder that is opened.

Anyway, I fully understand. If performance will be a concern, I do not mind closing the PR. I just wanted to share it since it might help other plugin developers too. So far, I am just using it in a feature in CompassNavigator

It might be useful to some but it should be done in an opt-in way so that whatever is using it has to define the pattern/directory to watch. In other words, there should probably be a programmatic way of setting up the watcher that other packages could leverage. Though I'm not sure if there would then be a benefit in this package if they could do it directly through LSP-file-watcher-chokidar instead.

Maybe you should better explain the use case you think this could solve so that we can understand how it should be designed?

@kapitanluffy
Copy link
Contributor Author

It always creates a single watcher process at most but it matters how many directories are watched (how big the tree is). Watching your whole home directory will consume a lot more resources than watching 10 small directories.

I see. So for example if I have a project that has 2 root directories, it will still use 1 process (given that it creates 1 process at most). The only problem is if the directory is big.

Maybe you should better explain the use case you think this could solve so that we can understand how it should be designed?

The whole idea is to provide a way for plugins to hook into file events and use that to do something. For example, in CompassNavigator, a cache is built from the MRU stack and the existing files so that it won't recalculate everything everytime you open it. If a new file is created, there is no way to signal the package that it needs to rebuild the cache. There are probably other use-cases but this is one of the things I can currently think of on the top of my head.

It might be useful to some but it should be done in an opt-in way so that whatever is using it has to define the pattern/directory to watch. In other words, there should probably be a programmatic way of setting up the watcher that other packages could leverage. Though I'm not sure if there would then be a benefit in this package if they could do it directly through LSP-file-watcher-chokidar instead.

Are you saying that instead of "subscribing" to events, it should be the package's responsibility to create/register its own watcher based on its own conditions? Let's say if there are 2 packages and 2 folders, then there would be 4 watchers? Will that be more optimal?

As for opting in. Right now, it will track folders in the sidebar I could probably make it work only for projects to make it at least "opt in". or maybe, I can make it decide to not run if the folder is too big and provide the user an option to enable it.

@rchl
Copy link
Contributor

rchl commented Feb 16, 2024

For cases like CompassNavigator it might not make any difference to do what I'm suggesting since it sounds like you want to watch all project folders, always. Unless you want to only listen to file creations/deletions then you could optimize by only setting up watcher for those events.

There might be other use cases where someone would want to watch only specific files or extensions in which case it would be much more efficient to just set up watcher for those specifically. Setting up watcher for the whole directory and always running a window command to broadcast those (even if nothing is listening) just sounds wasteful and potentially would even cause noticeable delays (but that's just my assumption).

@FichteFoll
Copy link
Collaborator

I agree with @rchl's concerns here: Always setting up watchers for all projects when no other plugin asked for them seems a bit wasteful. I understand that the idea of this package is to unify watchers so that multiple packages can share the same watcher(s) for a project, but since there is currently no way to declare that a package would like to have a watcher (i.e. register it) it must simply watch everything, which may be taxing on resources. I could also imagine a case where a package would only be interested in updates for certain files or projects and most likely not for any gitignored or folders excluded from the project (which I don't remember whether it's respecting that already).

Since the package is supposed to work for both plugin hosts, a way of communication using commands is preferred, so you might want to implement a command API to start watching for certain projects when requested by a package. Then the watcher package needs to keep track of which packages requested a watcher and also allow unregistering their watchers, etc. Of course, this will quickly become more demanding of the developer to use this package, but the concerns raised by @rchl are real and will likely occur in the long run.

In my opinion, it doesn't rule out the addition of the package in its current state, but you should be aware of the limitations of this approach and ideally also document them in your README.

@FichteFoll FichteFoll self-assigned this Feb 21, 2024
@kapitanluffy
Copy link
Contributor Author

I understand the concerns and I have been weighing it. I feel like having the developer manage the watchers (registering/unregistering) by themselves will be too demanding to use in contrast to just listening to a command. Hooking directly into chokidar would be a much better option.

I guess updating the README and explaining the limitations and the caveats tied to using this package is a better idea.

In the future, I am planning to create a way for the projects to determine which directories to watch and a package setting that would make it ignore common directories; that should lessen the resources used.
I will also look into "a way of communication using commands" (something like sublime.run_command("register_watcher", { package_name }) maybe?). I will probably ping you on Discord about it if that's not much of a hassle.

Anyway, thank you for the feedback guys. Let me know what else should I still do.

@FichteFoll
Copy link
Collaborator

Let's roll with this for now.

@FichteFoll FichteFoll merged commit 21aedb1 into wbond:master Feb 26, 2024
2 checks passed
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.

4 participants