Architecture concerns

[dan-dr]

Hi,
First of all thanks for the library. simplified a lot of things for me.
But I think it's on it's way to become a spaghetti monster.
Overall a lot of things are working good, but trying to extend, or work around is very difficult.

My first concern is about the controls: Right now there's 3(!) different ways to control the video view - Default controls, Callbacks AND EventBus. This is too much. It makes following the code of the library extremely confusing.

Now about the Default controls being embedded inside the view - it looks like a very bad design. There's no way for them to work outside of the EMVIdeoView, so while they work perfectly inside your code, extracting them outside and trying to build custom controls is very tiresome. Of course it's not meant to be outside, but there's not reason to put it all over the VideoView when they can perfectly control it from the outside - the same way we would implement it - callbacks OR events.

Now as a consequence of embedding the default controls in the video view - it now has a lot of method delegation to the controls - e.g. setPlayPauseImages instead of having a #getDefaultControls().setPlayPauseImages

Next, there's so many events and callbacks, it's hard to tell what is coming from where. when dealing with these view-controllers, there should be a distinction of event emitters - meaning EMMediaPlayPauseEvent should only be triggered by one of the mediaplayer/videoview OR the controls. What happens now is a mix & match - some events are triggered by the EMVideoView, some by the controls - and the don't sync!
for example in DefaultControls.java:

    protected void onPlayPauseClick() {
        if (callback != null && callback.onPlayPauseClicked()) {
            return;
        }

        if (bus != null) {
            bus.post(new EMMediaPlayPauseEvent()); // OLD DATA
            if (busPostHandlesEvent) {
                return;
            }
        }

        //toggles the playback
        boolean playing = videoView.isPlaying();
        if (playing) {
            videoView.pause();
        } else {
            videoView.start();
        }
    }

When I register to a EMMediaPlayPauseEvent and it triggers, I expect that a video was paused or played - but this is a controls click event, before we called videoView.pause(). so to act upon this event I HAVE TO handle the event myself using setFinishOnBusEvents, thus having to handle ALL the events myself.

There's actually more examples but this is too of a long rant, and I'm sure you know all this already!

Just trying to start a discussion around this since fixing even some of these is 100% breaking changes, and that should be handled (not written, just handled) by the library author. I want to see if there's support for this from the author since I don't have much time to write these by myself, and want to make sure if this happens it could be in sync with the core features and not fall behind, since I do need the improvements which aren't related to UI and structure.

Thanks again!

[brianwernick]

I completely agree with all of these concerns (and what I'm assuming are others). I've had some new architecture designs drawn up for a while which fix all of these issues, but I've been short on time lately (I should have time next week). Additionally, the EventBus support was supposed to be a test that ended up sticking around way too long (it will be removed in 3.0).

I'm excited that you took the time to bring up your concerns, especially in such an example rich manner. That being said, my first goal will be to fix the other bugs before going down this path. My hope is that I will be able to get 3.0 out around the start of next year (maybe end of this year). As I work on migrating this, I will be committing to a 3.0 brach, so if you have time I would like it if you could look it over occasionally to see if I have missed anything. (The branch doesn't exist yet)

[dan-dr]

awesome!
excited to see whats in store :)

[brianwernick]

Just an update, I have started work on the 3.0 branch

[brianwernick]

UPDATE:

In case you haven't noticed --or for anyone else who happens upon this question-- I've decided to extract the playlist support from this library for two main reasons.

1. I felt like ExoMedia was performing two functions:
   • Wrapping the ExoPlayer in to an AudioPlayer and VideoView with simplified APIs
   • Playing lists of media with support for Notifications and RemoteViews (like Android Wear, lock screens, and bluetooth devices).
2. We were forcing everyone who wanted the EMAudioPlayer and EMVideoView to get the playlist support whether they wanted it or not.

ExoMedia will still provide the EMAudioPlayer and EMVideoView mentioned above however the playlist support has been extracted in to its own library called PlaylistCore.

If you are worried about migrating from the current ExoMedia (2.5.x) to ExoMedia 3.0.x and PlaylistCore, I will be adding a full example to the Demo app and creating a migration guide on the wiki

[TonyTangAndroid]

Hello there, may I inquiry what's status of 3.0.0 milestone?

[brianwernick]

@TonyTangAndroid Because my schedule is pretty busy at the moment I can't give any specific time frame. My best guess would be 1-2 months.

[brianwernick]

I'm going to close this as any architecture changes should be addressed in the 3.0 branch now. If you notice any oddities or issues please let me know.

[brianwernick]

I forgot to mention that there is now a preview release of 3.0.0 (compile "com.devbrackets.android:exomedia:3.0.0-preview1")