sghpjuikit / player

Audio player and management application.
22 stars 2 forks source link

Library source + watcher #92

Closed sghpjuikit closed 5 years ago

sghpjuikit commented 6 years ago

It is very common functionality to allow user customize source directories for the audio library. Optionally, have these directories monitored for changes and update library automatically.

I propose a plugin which is configurable

Simple functionality, but there are some complications:

sghpjuikit commented 6 years ago

I have a working prototype, roughly in 100 lines.

The biggest problems are threading and file shadowing (we must not monitor same directory multiple times if its parent is already monitored).

xeruf commented 6 years ago

This is essential, I have all my music in one recursive folder! So it'd be a core plugin right? What I think is easiest if you don't want permanent monitoring of the folders (especially on non-windows) is an option to automatically do a full scan at startup, that's how many players do it.

sghpjuikit commented 6 years ago

I also am in a situation like yours.

Yes the plugin is internal one, at least until external plugins are supported, then we will see. It will probably be enabled by default, with directory monitoring disabled when unsupported. I want to stress this again: recursive directory monitoring only works on Windows as Unix based systems do not support such feature. This is quite a problem as simulating recursive watching in Java gets complicated fast and there are also issues with performance. Needless to say we have no such solution in this project as of this moment.

Yes, the startup scan is also a good idea (definitely will be done). I have not started to implement this yet as it also is not fully trivial. There is lot to think about running heavy actions while application starts, for instance:

So perhaps the toughest part will be to integrate the feature seamlessly into the application. On the other hand, we are talking 200 lines of code (for the entire plugin!) in the worst case I think.

Right now, I have a functional prototype, at about 140 lines, which include the configuration and monitoring functionality. I'm not happy with the code however as I feel there is way too many Observables everywhere.

The file monitoring feature on/off switch is completely crazy:

    private val fileMonitoring = when {
        fileMonitoringSupported -> Subscribed {
            Subscription.multi(
                    fileMonitoringEnabled sync { sourceDirsChangeHandler.subscribe(it) },
                    Subscription { sourceDirsChangeHandler.subscribe(false) }
            )
        }
        else -> Subscribed { Subscription { } }
    }

Thats like a 4-level Subscribtion going on there... Good luck reasoning about that.

So, I will work on compactifying this a bit before I push the code so you can give me some feedback and ideas you may have.

sghpjuikit commented 6 years ago

There is another issue the plugin needs to be aware of: when application edits the song files (potentially a lot of them) the watcher should behave with caution. Application can potentially use safe write strategy (i.e. creating temporary file duplicates) or edit certain metadata. The watcher must not edit the database with incorrect or duplicate values. Optimally, watcher would not cause useless insertion-removal as library refreshing is about the most expensive UI call we have. It may be necessary to increase file change event time period and call file.exists() for each of the changed files.

xeruf commented 6 years ago

I think you could either accumulate change-events until there's a second or so with no changes, or tell the Watcher to temporarily ignore changes while editing the song files, or both.

sghpjuikit commented 5 years ago

Implemented. I'm sure there is more work to be done, like integrating menus and so on, but one at a time...

xeruf commented 5 years ago

Implemented in https://github.com/sghpjuikit/player/pull/108 and merged in https://github.com/sghpjuikit/player/commit/56aa53f5efae342a125bf81b57d42d07c8b0a71e