GNOME Bugzilla – Bug 709165
refactor set_av_transport_uri_cb
Last modified: 2013-12-14 16:17:03 UTC
The nested callback stuff has made it quite large and ugly, we should try to split it up somehow
Created attachment 258840 [details] [review] renderer: Refactor SetAVTransportURI
FYI: I have a add-on patch to this one. The point of it is moving uri/metadata/track-uri/track-metadata/playlist state tracking to controller, where I think they belong: this paves way for being able to to more easily do SetNextAVTransportURI(). It's not 100% done, but I realised I hadn't mentioned I was playing with this... Anyway, let me know if this clashes with your plans. I'll upload the patch on wednesday.
Nope. Sounds good :)
I'll add my patches here if you don't mind (as they are based on yours). Also, your patch LGTM and works in my tests. So the general idea in the first two patches is to move related functionality to one place: uri, metadata and everything that is derived from those two (track_uri, track_metadata, n_tracks, track) is handled in PlayerController. The last two are minor improvements in the same area.
Created attachment 263333 [details] [review] renderer: handle playback state in PlayerController Player.playback_state was modified from both PlayerController and AVTransport. Start making all modifications through PlayerController.playback_state. This has the added benefit that we don't accidentally expose "EOS" state to AVTransport. Also rename some variables "player"->"controller" to match reality.
Created attachment 263334 [details] [review] renderer: Refactor AVTransport and PlayerController Move track_metadata and track_uri state tracking to controller, Stop modifying controller properties from many places in AVTransport. Add set_single_play_uri() and set_playlist_uri() as the only valid methods to set uri, metadata, etc. Also make property setters private if setting them from AVTransport makes no sense. The goal is to make both classes cleaner, no functional changes are intended.
Created attachment 263335 [details] [review] renderer: Avoid notifying track and n_tracks unnecessarily There were redundant values in LastChange events.
Created attachment 263336 [details] [review] renderer: Simplify PlayerController "EOS" handler
I realize now I didn't test my changes with playlists: I 'll do that tomorrow.
Review of attachment 263333 [details] [review]: +1
Review of attachment 263334 [details] [review]: +1 assuming the removal of the check was intended ::: src/librygel-renderer/rygel-av-transport.vala @@ +705,2 @@ } Oh, nice catch. ::: src/librygel-renderer/rygel-player-controller.vala @@ -59,3 @@ get { return this._playback_state; } set { this.player.playback_state = value; } - default = "NO_MEDIA_PRESENT"; While we're at it: Should we make this an enum? @@ -230,2 +297,1 @@ if (item.upnp_class.has_prefix ("object.item.image") && - this.collection != null && Are you removing this check for automatic transition to the "Next" Uri when SetNextUri was used after CurrentUri is an Image?
Review of attachment 263334 [details] [review]: +1 assuming the removal of the check was intended ::: src/librygel-renderer/rygel-av-transport.vala @@ +704,3 @@ action.return (); } Oh, nice catch. ::: src/librygel-renderer/rygel-player-controller.vala @@ -59,3 @@ get { return this._playback_state; } set { this.player.playback_state = value; } - default = "NO_MEDIA_PRESENT"; While we're at it: Should we make this an enum? @@ -230,2 +297,1 @@ if (item.upnp_class.has_prefix ("object.item.image") && - this.collection != null && Are you removing this check for automatic transition to the "Next" Uri when SetNextUri was used after CurrentUri is an Image?
Review of attachment 263335 [details] [review]: +1
Review of attachment 263336 [details] [review]: +1
Review of attachment 258840 [details] [review]: +1
Attachment 258840 [details] pushed as e7240f5 - renderer: Refactor SetAVTransportURI Attachment 263333 [details] pushed as 1db7452 - renderer: handle playback state in PlayerController Attachment 263334 [details] pushed as 7e7b9c2 - renderer: Refactor AVTransport and PlayerController Attachment 263335 [details] pushed as cc80bdd - renderer: Avoid notifying track and n_tracks unnecessarily Attachment 263336 [details] pushed as c2ae13f - renderer: Simplify PlayerController "EOS" handler