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

MPD idle cleanup #1347

Merged
merged 10 commits into from Dec 5, 2015

Conversation

2 participants
@adamcik
Member

adamcik commented Dec 4, 2015

This combined with #1346 provides a much nicer experience when seeking as clients are far less likely to end up out of sync.

@adamcik adamcik added the A-mpd label Dec 4, 2015

@adamcik adamcik added this to the v1.2 - Gapless milestone Dec 4, 2015

@adamcik adamcik force-pushed the adamcik:feature/mpd-idle-cleanup branch from 47732bf to aa010e0 Dec 4, 2015

@jodal jodal changed the title from MPD idle clenaup to MPD idle cleanup Dec 4, 2015

- Idle events are now emitted on ``seekeded`` events. (Fixes: :issue:`1331`)
- Event handler for ``playlist_deleted`` has been unbroken. Likely fixes
unreported / diagnosed crashes.

This comment has been minimized.

@jodal

jodal Dec 4, 2015

Member

"unreported/undiagnosed"

@@ -57,9 +60,7 @@ def on_stop(self):
process.stop_actors_by_class(session.MpdSession)
def send_idle(self, subsystem):
listeners = pykka.ActorRegistry.get_by_class(session.MpdSession)
for listener in listeners:
getattr(listener.proxy(), 'on_idle')(subsystem)

This comment has been minimized.

@jodal

jodal Dec 4, 2015

Member

👍 for getting rid of the proxy creation here. Should save some cycles.

(['track_playback_ended', 'tl_track', 'time_position'], None),
(['playback_state_changed', 'old_state', 'new_state'], 'player'),
(['tracklist_changed'], 'playlist'),
(['playlists_loaded'], None),

This comment has been minimized.

@jodal

jodal Dec 4, 2015

Member

Should this trigger stored_playlist?

This comment has been minimized.

@adamcik

adamcik Dec 5, 2015

Member

Probably wouldn't hurt.

getattr(self, event)(**kwargs)
except Exception:
# Ensure we don't crash the actor due to "bad" events.
logger.exception('Triggering event failed: %s', event)

This comment has been minimized.

@jodal

jodal Dec 4, 2015

Member

Log kwargs too?

@jodal

This comment has been minimized.

Member

jodal commented Dec 4, 2015

LGTM, with comments :-)

@jodal jodal self-assigned this Dec 4, 2015

@@ -57,9 +60,7 @@ def on_stop(self):
process.stop_actors_by_class(session.MpdSession)

This comment has been minimized.

@adamcik

adamcik Dec 5, 2015

Member

I'm considering the following change to this handling as we don't really ever care about the kwargs we just need to know what event fired and map it. Thoughts?

_CORE_EVENTS_TO_IDLE_SUBSYSTEMS = {                                             
    'track_playback_paused': None,                                              
    'track_playback_resumed': None,                                             
    'track_playback_started': None,                                             
    'track_playback_ended': None,                                               
    'playback_state_changed': 'player',                                         
    'tracklist_changed': 'playlist',                                            
    'playlists_loaded': 'stored_playlist',                                      
    'playlist_changed': 'stored_playlist',                                      
    'playlist_deleted': 'stored_playlist',                                      
    'options_changed': 'options',                                               
    'volume_changed': 'mixer',                                                  
    'mute_changed': 'output',                                                   
    'seeked': 'player',                                                         
    'stream_title_changed': 'playlist',                                         
} 

....

class MpdFrontend(...):
    ....

   def on_event(self, event, **kwargs):                                          
        if event not in _CORE_EVENTS_TO_IDLE_SUBSYSTEMS:                          
            logger.warning(                                                       
                'Got unexpected event: %s(%s)', event, ', '.join(kwargs))         
        else:                                                                     
            self.send_idle(_CORE_EVENTS_TO_IDLE_SUBSYSTEMS[event]) 

This comment has been minimized.

@adamcik

adamcik Dec 5, 2015

Member

This probably doesn't make sense, reasoning being that we need to add playlists_loaded, playlist_changed, playlist_deleted handling which will update the URI mapper based on changes.

This comment has been minimized.

@jodal

jodal Dec 5, 2015

Member

I think this looks good, and I like that fact that it will complain when we add a new core event and forgets to update MPD.

As for URI mapper maintenance, I'm OK with a if/elif/elif block after the above code.

@adamcik

This comment has been minimized.

Member

adamcik commented Dec 5, 2015

Comments have been addressed, just my last suggestion / question left now.

@adamcik

This comment has been minimized.

Member

adamcik commented Dec 5, 2015

Oh, and the fix for playlist_deleted could probably be backported to a bugfix release if we want to.

@jodal

This comment has been minimized.

Member

jodal commented Dec 5, 2015

Feel free to cherry-pick the playlists_deleted commit to the release-1.1 branch. I'd like to make an 1.1.2 release soon.

@adamcik

This comment has been minimized.

Member

adamcik commented Dec 5, 2015

Done

jodal added a commit that referenced this pull request Dec 5, 2015

@jodal jodal merged commit 3e259f1 into mopidy:develop Dec 5, 2015

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@adamcik adamcik deleted the adamcik:feature/mpd-idle-cleanup branch Dec 5, 2015

@adamcik adamcik referenced this pull request Dec 5, 2015

Closed

Core events not mapped in MPD handler #1331

3 of 3 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment