Page MenuHome

File browser: Initial integration of file system monitoring
AbandonedPublic

Authored by Sergey Sharybin (sergey) on Nov 12 2015, 7:22 PM.

Details

Summary

This is a proof-of-concept patch which integrates file system monitoring
into file space. The idea is to perform automatic file list refresh when
creating / deleting files outside of Blender. Currently only covers Linux
with the following TODOs and limitations:

  • Naming is a bit weird, it's nothing to do with X11, it's using INotify from Linux so some classes are to be renamed.
  • Only CMake is hoked up
  • It is actually forced ON in CMake, need to become a build system option.
  • Only single file browser works reliably

    This is due to inotify limitation which doesn't let to monitor directories recursively. So each file space would need to tell GHOST that it wants to monitor some directory. Doable, but not for initial patch.
  • File space is not refreshed immediately, Blender window needs to gain focus back in order to perform list refresh.

All in all, before moving forward let's gather some feedback about whether
we want this and if we do, whether we can do something different here.

Diff Detail

Repository
rB Blender
Branch
inotify

Event Timeline

Sergey Sharybin (sergey) retitled this revision from to File browser: Initial integration file system monitoring.Nov 12 2015, 7:22 PM
Sergey Sharybin (sergey) updated this object.
Sergey Sharybin (sergey) updated this revision to Diff 5423.
Sergey Sharybin (sergey) retitled this revision from File browser: Initial integration file system monitoring to File browser: Initial integration of file system monitoring.Nov 12 2015, 7:23 PM

File space is not refreshed immediately, Blender window needs to gain focus back in order to perform list refresh.

I think this is true for a lot of other program with features such as this. Also will bpy have access so people can make addons that can go through a .blend and reload the datablocks. Also will in work for linked datablock from other .blend files?

hewi added a subscriber: hewi.Nov 15 2015, 4:02 PM

I’m not much convinced we need that feature really… Currently, filebrowser is typically never kept open, you just open it for a specific action and close immediately after. Even if with future asset project this may change, imho just having the 'refresh' button is good enough here, not a real big burden to hit it when you want to refresh your listing.

Another issue is forcing listing of heavy data - imagine you are at level 3 of recursivity over a dir containing a bunch of .blend file (in append/link mode), refreshing every time something changes in that dir could become an issue. Same goes with network stuff (provided this inotify is network-aware?).

Other thing I’m not much happy with is using GHOST for that - it adds yet another per-platform piece of code to maintain. And it’s not really flexible, we cannot easily adapt to which changes we want to monitor (e.g. when listing files we typically do not care about their content, except if we show thumbnails, or are listing .blends' content; also, when to do recursive monitoring, etc.). Not to mention future asset engines.

If we really want some kind of automatic update, I would rather implement that at the filebrowser level (as a light background task, like previews), a simple check called periodically, with option for the user to enable it or not (per opened browser).

Also not really keen on this feature.
On one hand its *nice* to support, on the other hand its means we end up having to support file watching on 3x platforms,
when Blender isn't often used as some persistent file manager (file selector is most often a popup, briefly used and closed).

It adds a bit of complexity for the fileselector too, that there is the potential for operators to have their state changed (causing rare but possible bugs).

Of course if this is really needed we can just accept the extra overhead (which isn't huge), and make sure corner cases are working.
We could postpone this functionality until there is important use case shows up (possibly asset manager engine could make use of it).

@Bastien Montagne (mont29), you might easily want to keep it opened when working with clips collection in VSE and refresh buttons are rather really annoying, those are to be gone (at least from the default visible interface).

There's surely no network support (server will not send any information to the clients when contents changes). Also statment "refresh when something changes" is not really correct either. IN_MODIFY flag was added by accident as a result of some other experiments and without this flag refresh only happening when new files appears/disappears from the viewing folder. Additionally, it's possble to bypass refresh if new/deleted file does not match current filtering.

Listening in the link/append mode is also not really an issue, currently inotify will not register for that path because it'll match file, not a directory.

Also not really sure about this "extra per-platform code maintain" notes. We do have enough platform-specific code (drag-n-drop, file operations, even platform-cpecific things in Cycles). How often we do anything to maintain it? Simple example -- xdnd integration is 3 years old now and it was just a single fix applied 2 years ago, other that that it's all "code cleanup" commits. If that causes an issue let's rather stop doing cleanups?

@Campbell Barton (campbellbarton), The only complication to file selected is an extra notification listener, which doesn't seem so bad. How would automatic refresh cause some operator state changes and manual refresh will not? Can you give exact example of such thing happening? To me it'll seem more like an operator bug, it's state should not change due to interface refresh.

@Campbell Barton (campbellbarton), The only complication to file selected is an extra notification listener, which doesn't seem so bad. How would automatic refresh cause some operator state changes and manual refresh will not? Can you give exact example of such thing happening? To me it'll seem more like an operator bug, it's state should not change due to interface refresh.

Not to overstate the complications am sure some are trivial to resolve and since this is a proof of concept, expect you're aware of them too,
just to note they exist and some should be resolved.

  • file rename is interrupted (currently canceled).
  • refresh happens even when the changes to the file system are filtered out. (possibly un-avoidable, from user POV it could not visibly refresh though).
  • not working with recursive view.
  • selection is lost.
  • deleting a directory which is watched will bump you out of the current directory. (corner case)
  • currently only activates when the area's active, (probably you want it to refresh when visible). you may have file-list in its own window on a second monitor.
  • if your encoding a video or writing into the directory preview will keep running (can be managed smarter by code - or user can disable preview).

Also not really sure about this "extra per-platform code maintain" notes. We do have enough platform-specific code (drag-n-drop, file operations, even platform-cpecific things in Cycles). How often we do anything to maintain it? Simple example -- xdnd integration is 3 years old now and it was just a single fix applied 2 years ago, other that that it's all "code cleanup" commits. If that causes an issue let's rather stop doing cleanups?

Right, the inotify code its self (the ghost part), isn't a problem to maintain.
The issue is we have to support new functionality on Blender side (investigate user reports if it fails - on various platforms).

This is more comparable to changes in wm_dragdrop.c (which had a bunch of issues/fixes), though its still comparing apples and oranges.
Whatever the case, if you add and are happy to support - investigate bugs etc... I'm not so much against it, patch seems reasonable though I only did checked over briefly.

In that case @Bastien Montagne (mont29) has more to do with this since he's been working here.

intern/ghost/CMakeLists.txt
247

This files missing from the patch

intern/ghost/intern/GHOST_FileSystemManagerX11.cpp
65–73

Users may have have 2+ file selectors open.

Tested and behavior when 2x file selectors are open is to refresh all, based on one of their directories.

Probably this should use the active file-selector? (if the aim is to watch one directory).

source/blender/editors/space_file/filelist.c
1381

Getting a crash here when a directory is being watched, then opening a file selector (Ctrl-O)

#0 0xba5177 in filelist_reset /src/blender/source/blender/editors/space_file/filelist.c:1381
#1 0xbc32c1 in file_listener /src/blender/source/blender/editors/space_file/space_file.c:319
#2 0x1d4149b in ED_area_do_listen /src/blender/source/blender/editors/screen/area.c:140
#3 0xaa9a48 in wm_event_do_notifiers /src/blender/source/blender/windowmanager/intern/wm_event_system.c:367
#4 0xa8d224 in WM_main /src/blender/source/blender/windowmanager/intern/wm.c:492
#5 0xa89bc7 in main /src/blender/source/creator/creator.c:2001
#6 0x7fa9484c260f in __libc_start_main (/usr/lib/libc.so.6+0x2060f)
#7 0xa81438 in _start (/src/cmake_debug/bin/blender+0xa81438)
intern/ghost/CMakeLists.txt
247

Not sure what do you mean by this, the files are in the patch and you can see them here. You sure you're applying patch correctly?

Or did you mean intern/GHOST_EventFileSystem.h is missing? This file is to be gone actually.

intern/ghost/intern/GHOST_FileSystemManagerX11.cpp
65–73

Please refer to the TODO #4 from the patch description.

source/blender/editors/space_file/filelist.c
1381

Couldn't really reproduce the issue, but guess it's because sfile->files could be NULL. This is being checked in another listener. Will add it to this one.

Sergey Sharybin (sergey) updated this revision to Diff 5428.

Some typo fixes and hopefully crash fix

I do not have anything really against this patch (as long as I do not have to maintain GHOST side of it ;) ), changes in filebrowser itself are very limited and OK afaikt.

I’m just not convinced at all we need that feature - and find it too much limited and not enough flexible compared to what a complete 'auto-update' feature would have to do in current (and future asset-aware) filebrowser, that’s all.

@Bastien Montagne (mont29), What exact limitations you're talking about? There are two things mentioned in the original description:

  • Multiple file selectors support
  • Refresh without waiting focus to be returned back to Blender

Those are to be addressed and they aren't so difficult in fact. Just didn't make it priority before gathering general opinion on the feature and mainly was working on inotify docs reading and such so far.

@Sergey Sharybin (sergey): Already said it (shortly) in my first comment, to get this auto-update feature complete here are all the points I see:

  • Update on add/remove/rename of files & dirs.
  • Update on file change itself when using previews and/or listing internals of .blend files (link/append modes) - afaik monitoring a single file is not possible with inotify?.
    • (Probably totally anecdotal, but since we also show file size, in fact we should probably always update on file change? Though for file size & previews we could avoid re-listing the whole dir…)
  • Support multiple filebrowsers (with a way to avoid relisting all of them when only one dir changes).
  • Support recursive mode (that is, monitoring several directories for a same filebrowser).

Etc. Not to mention potential future AssetEngine update needs. That’s why I said I would rather see this done by Blender itself, using a light background task, instead of relying on some limited OS-level system (so that we can precisely control what we monitor, and how). My main point is that our filebrowser is both less and more than a 'real' OS filebrowser, and it aims at being even more different in future. OS-level file handling is only a part of its goals, and a rather limited one.

Part of my work last year in filebrowser was to precisely make it much more independent from low-level internal filesystem module, so not totally happy at the idea of adding back something that is intrinsically totally bound to local filesystem only. But again, since it currently does not touch filebrowser much, I’m not really against it either. Just have the feeling it’s adding more complexity and OS-dependent code to maintain, for a really narrow usecase.

Taking assumption that because of code simplicity currently only one browser is supported:

  • Update on add/remove/rename is already working for the local file systems. Remote file systems is more tricky, but there we can't do much really.
  • You can in theory set a watch to individual file. In practice i don't think we should do this. Instead we should add watch to all current directories and then handle inotify events. We can avoid full re-listing all together, because we know which file triggered an event and what happened to that file. This all is coming from struct inotify_event which content is currently not being checked, but it's something we can totally consider to do.

Recursive is somewhat same as several file browsers support actually.

instead of relying on some limited OS-level system (so that we can precisely control what we monitor, and how)

inotify is a kernel subsystem and apparently, it's the kernel which knows for fact what operations are being done with each and all of file system elements. If you feel like implementing own system for tihs instead, good luck with that and i'll just abandon this revision.

Forgot to mention. I don't really see anything wrong in using OS-related stuff to help dealing with real file system. Event handling in file browser could be flexible enough to support events coming from whatever-asset-engine-is-in-use, some own implementation of whatever-watcher-we-want and also handle events coming from GHOST/whatever-OS-level-system.

Even though am not too concerned about using inotify, its worth checking existing multi-platform file-watching wrappers, See: QFileSystemWatcher [0],
since we probably will end up with code like this - eventually.


Once we have asset-engines, its reasonable they might want to hook into file-watcher,
only supporting a single directory is a bit limiting in that case (where having 2x file selectors is still limiting - in practice for typical Blender usage, we could accept the limit).

I think it would be better to expose a file-watching API in a more generic way (like QFileSystemWatcher). Then after asset-management is in.
Then allow add-ons and asset engines to take advantage of it.

Otherwise we have 3x multi-platform file-watcher wrappers in GHOST,
then re-write them when we want to use them in other parts of Blender.

[0] https://github.com/qtproject/qtbase/tree/33a43cdbf258a3f426719d1e74c117ee4b68a9b9/src/corelib/io (qfilesystemwatcher_*.*)

Am not really motivated for working on it any further. It's just concerns about complexity in the code I don't ask to maintain and requests to look into yet another libraries which are subjectively better and can't be used directly anyway. And it was orooven that implementing algorithms instead of copy-paste them from external libs leads to a cleaner code which is easier to maintain.