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

Modify watch_filesystem to take in a list of folders to watch #3325

Closed
leakec opened this issue May 4, 2024 · 2 comments
Closed

Modify watch_filesystem to take in a list of folders to watch #3325

leakec opened this issue May 4, 2024 · 2 comments

Comments

@leakec
Copy link

leakec commented May 4, 2024

It would be nice to modify watch_filesystem to take in a list of folders to watch. The reasons for doing this are:

  1. Avoid doing unnecessary work watching files we don't care about.
  2. Currently, this watches the /host folder and recurses through all child folders. As a result, if the user doesn't have access to a file in the /host folder or child folders, this results in an error. Restricting the folders watched to the minimum required should limit this happening.

To make this change, watch_filesystem could be modified from this function signature

pub fn watch_filesystem()

to

pub fn watch_filesystem(files: &HashMap<PathBuf, bool>)

If watch_filesystem is called with an empty HashMap, we can fill it with /host and true to get the previous behavior.

The downside of this is the function signature of watch_filesystem will change, which means any plugins using it will need to update this call. This doesn't seem to be used much in the zellij core. There was only one instance of it in the core, and it was for a test. multitask is another plugin I know that uses this, and I will update said plugin if this change is made.

I've got this implemented and tested on a local fork. Would be happy to submit a PR if this is accepted (or modify and submit a PR based on any discussion here).

@leakec
Copy link
Author

leakec commented May 22, 2024

The PR I have ready for this is located here (should have added this when I first opened the issue).

EDIT: Created PR at #3377.

@leakec
Copy link
Author

leakec commented Nov 5, 2024

Closing this as the PR has been closed. There is a better way to fix this, which is with new plugin APIs available in 0.41! :)

@leakec leakec closed this as completed Nov 5, 2024
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

No branches or pull requests

1 participant