-
Notifications
You must be signed in to change notification settings - Fork 39
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
Listen for when a resource changes #2669
Comments
One use case: sublimelsp/LSP#516 |
Could you summarize the use case here? It isn't clear to me without reading the LSP spec to know what the requirements would be. |
I can't summarize more than it is, but I'll try:
/**
* The file event type.
*/
export namespace FileChangeType {
/**
* The file got created.
*/
export const Created = 1;
/**
* The file got changed.
*/
export const Changed = 2;
/**
* The file got deleted.
*/
export const Deleted = 3;
}
export namespace WatchKind {
/**
* Interested in create events.
*/
export const Create = 1;
/**
* Interested in change events
*/
export const Change = 2;
/**
* Interested in delete events
*/
export const Delete = 4;
} It is worth checking the workspace/didChangeWatchedFiles section of the spec for more details. |
|
The proposed I propose it should receive a list of these small objects: class FileSystemEvent:
__slots__ = ('kind', 'path', 'prev_path')
def __init__(self, kind: int, path: str, prev_path: Optional[str] = None) -> None:
self.kind = kind # sublime.FS_CREATED / FS_DELETED / FS_MODIFIED / FS_RENAMED
self.path = path
self.prev_path = prev_path # in case of FS_RENAMED, the old path before the rename |
Reading first message, it pretty much describes use case of watching package resources while for LSP it's rather about watching workspace folders instead. Just wanted to clarify, since potential implementation would most likely handle both cases anyway. |
The two use cases are similar, but there are some important differences.
|
It has to, otherwise it cannot update the side bar or it wouldn't ask the user to reload the view once the underlying file was changed due to an external FS event. |
We watch both packages and workspace directories, and there are a lot of edge cases we handle that would take time to get right in an external implementation. I've discussed this with Jon before, and the blocker has always been the performance implication of serializing, potentially, thousands of FS events (specifically paths) over the plugin_host SHM segment, and the performance of Python finding things that are important to it, and doing something about it. My understanding is we'd have to try and come up with some way to deal with things like switching branches in Git repositories in a sane way such that it didn't bog the plugin interface down. |
I suppose that could be mitigated via subscriptions so that plugins explicitly describe notifications they need (e.g. In X workspace, ending in |
I've also thought about collecting FS events, and then batching then into tree structures to try and reduce the raw amount of data being sent. |
Note that we have an issue about files in the project/workspace at #626. |
Some practical examples with LSP servers:
All of those don't really want to watch So in those cases, a "subscription" model appears better suited and likely more performant.
Sorry, but I'll keep discussing it here since I think it wouldn't make sense for developers to handle those separately. Both should be taken into consideration when designing the system and likely both would have to be implemented simultaneously. |
👋 just wanted to chime in with yet another example, also LSP related. The official language server for Go is gopls. Today I stumbled upon a problem related to adding new files into a package namespace. Let's say I have the following directory tree: .
└── main.go
0 directories, 1 files The package main
import "fmt"
func main() {
cards := []string{"Ace of Spades", "Five of Diamonds"}
} I introduce a new file, .
├── deck.go
└── main.go
0 directories, 2 files The package main
type deck []string If I now try to swap cards := []string{"Ace of Spades", "Five of Diamonds"}` in cards := deck{"Ace of Spades", "Five of Diamonds"}` it will error out, saying The usage of This is a fairly big roadblock in the usefulness of |
This exact scenario also happens when using the Python language server with sublime-lsp, Pyright. Any time I create a new python file and want to import anything from it in other files, I have to restart the language server manually. It's pretty annoying. |
FYI: there is interim solution for LSP: https://github.com/sublimelsp/LSP-file-watcher-chokidar |
Problem description
It's common for packages to want to take some kind of action when a resource changes. There is no simple, reliable way to do this. (Several workarounds are listed under Alternatives.)
Example use cases:
ruamel.yaml
.)Preferred solution
A new method
EventListener.on_resource_changed_async(name)
, to be invoked whenever:(I presume that Sublime already monitors the resource directories. That is, all of the work of monitoring the directories across three platforms is already done, so this feature is “merely” to notify the user code. If my assumption is incorrect, then please let me know.)
Example:
Package operations (such as (un)installing, (un)ignoring) may still cause a large number of events. Instead of calling the listener with the name of a single resource, it may make more sense to call the listener with an iterable of names. (As appropriate, the event could be debounced in addition to this.)
Some events will inevitably be spurious. For instance, most applications won't care if a resource is overridden by an identical copy. But it's probably better to leave this to the listener code to sort out rather than to try to do something clever.
Alternatives
An explicit subscription (like
Settings.add_on_change()
) could work as well. The downside is that the user would have to manually manage the subscription, e.g. remembering to remove it inplugin_unloaded
. (Sublime automatically managesEventListener
instances, eliminating this failure mode.) An explicit method could also take a parameter describing which resources to monitor, but it would be more flexible for the user code to handle this directly.There are several workarounds that packages might use instead of an event:
on_post_save_async
Packages can run a callback when the user manually saves a file. This may work well enough most of the time, but it will fail if a resource is modified by other means, such as Dropbox sync or a build system.
It may also fail if a resource is in a symlinked directory — the user would have to add code to manually check whether the directory is a symlink and get its resource name from there. This is rarely done in practice, and doing it correctly is surprisingly difficult and potentially inefficient. (AutomaticPackageReloader tries to do this, but it's currently buggy.)
Manual modification checks
Rather than waiting for a notification, a package can manually check to see whether a file has been modified. This can be accomplished by loading the entire resource and comparing it to a previous version (or a hash). This has several drawbacks. First, you have to read the entire resource and either compare it byte-by-byte or hash it. Second, in order to have a baseline for comparison you would need to pre-load all of the relevant resources, which could cause problems if many exist but few are expected to change. Third, in order to use this to monitor for changes (as opposed to checking manually as needed), you would have to poll the relevant files at intervals.
Checking modificaton time
As above, but instead of reading the file, check the file modification time. For unpacked resources, this is a filesystem operation; for packed resources, you can check the
ZipInfo.date_time
. It isn't necessary to read in the entire resource or pre-load resources for comparison, but there are several new brand-drawbacks. For one thing, you have to keep track of every physical file location and figure out the override logic yourself. For another, file modification times are inherently unreliable (for one thing, it's not necessarily monotonic) andZipInfo.date_time
even more so.inotify
(and company)A package could use
inotify
to monitor the resource directories. This is in some sense the “correct” way to do it. The package would have to use alternative implementations on OS X and Windows. Python packages to do this are available, but not (yet) as a dependency. Such a dependency would also have to reimplement the resource override logic — in effect, it would be doing most or all of the work that Sublime does to monitor resources (taking up its own set of resources to boot). It is unlikely that such an implementation would behave identically to Sublime's, though it might be good enough for most practical purposes. Another problem is that while this would remove the need for polling, I don't think there's any way to guarantee whether the package or Sublime itself would receive the first notification, which raises the specter of race conditions (especially if it were usually Sublime).The text was updated successfully, but these errors were encountered: