Skip to content
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.

Sign up for GitHub

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

Make class(es) more self-contained, reduce globals #146

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

Make class(es) more self-contained, reduce globals #146

Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
bc474c6
Instantiate logger at top of module
ferdnyc Dec 1, 2021
ccd8394
Pass loop instance to NotifyWrapper
ferdnyc Dec 1, 2021
3f4d8c4
Pass wrapper instance to MPRISInterface()
ferdnyc Dec 1, 2021
6e71969
Set up notifications from inside MPDWrapper()
ferdnyc Dec 1, 2021
a57828a
Merge branch 'master' into repl-usability
ferdnyc Dec 2, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
103 changes: 54 additions & 49 deletions src/mpDris2.in.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,9 @@

_ = gettext.gettext

logger = logging.getLogger('mpDris2')
logger.propagate = False

identity = "Music Player Daemon"

params = {
Expand Down Expand Up @@ -102,8 +105,6 @@
'cover_regex': r'^(album|cover|\.?folder|front).*\.(gif|jpeg|jpg|png)$',
}

notification = None

# MPRIS allowed metadata tags
allowed_tags = {
'mpris:trackid': dbus.ObjectPath,
Expand Down Expand Up @@ -247,7 +248,7 @@ class MPDWrapper(object):
errors and similar
"""

def __init__(self, params):
def __init__(self, params, loop=None):
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't loop be a required argument ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@eonpatapon

Wow, ancient history, I don't know how I missed these! Sorry about that.

...I think I originally set these up as defaulted arguments because I didn't want to change the class constructors' semantics until I had my additions finalized. But they probably don't need to stay in that state now.

As far as loop goes, though... I mean, I'm not sure it really has to be required.

The only reason loop is passed in is so the relaunch code in _name_owner_changed_callback() can call self._loop.quit(). If self._loop is None (it checks), it just won't call quit() on it.

Which... seems like it would be fine? In situations where a loop isn't supplied (say, in testing).

self.client = mpd.MPDClient()

self._dbus = dbus
Expand Down Expand Up @@ -278,6 +279,10 @@ def __init__(self, params):
if self._params['mmkeys']:
self.setup_mediakeys()

self._loop = loop
ferdnyc marked this conversation as resolved.
Show resolved Hide resolved
# Wrapper to send notifications
self._notification = NotifyWrapper(self._params)

def run(self):
"""
Try to connect to MPD; retry every 5 seconds on failure.
Expand Down Expand Up @@ -320,7 +325,7 @@ def my_connect(self):
self._can_single = True

if self._errors > 0:
notification.notify(identity, _('Reconnected'))
self._notification.notify(identity, _('Reconnected'))
logger.info('Reconnected to MPD server.')
else:
logger.debug('Connected to MPD server.')
Expand All @@ -329,7 +334,7 @@ def my_connect(self):
self.client._sock.settimeout(5.0)
# Export our DBUS service
if not self._dbus_service:
self._dbus_service = MPRISInterface(self._params)
self._dbus_service = MPRISInterface(self._params, mpd_wrapper=self)
else:
# Add our service to the session bus
#self._dbus_service.add_to_connection(dbus.SessionBus(),
Expand Down Expand Up @@ -371,7 +376,7 @@ def my_connect(self):

def reconnect(self):
logger.warning("Disconnected")
notification.notify(identity, _('Disconnected'), 'error')
self._notification.notify(identity, _('Disconnected'), 'error')

# Release the DBus name and disconnect from bus
if self._dbus_service is not None:
Expand Down Expand Up @@ -610,11 +615,11 @@ def notify_about_track(self, meta, state='play'):
uri = 'media-playback-pause-symbolic'
body += ' (%s)' % _('Paused')

notification.notify(title, body, uri)
self._notification.notify(title, body, uri)

def notify_about_state(self, state):
if state == 'stop':
notification.notify(identity, _('Stopped'), 'media-playback-stop-symbolic')
self._notification.notify(identity, _('Stopped'), 'media-playback-stop-symbolic')
else:
self.notify_about_track(self.metadata, state)

Expand Down Expand Up @@ -972,15 +977,18 @@ class MPRISInterface(dbus.service.Object):
__path = "/org/mpris/MediaPlayer2"
__introspect_interface = "org.freedesktop.DBus.Introspectable"
__prop_interface = dbus.PROPERTIES_IFACE
__wrapper = None

def __init__(self, params):
def __init__(self, params, mpd_wrapper=None):
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

mpd_wrapper is required too

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@eonpatapon

mpd_wrapper, OTOH... yeah. I absolutely agree, it needs to be required. I'll fix that and rebase this PR to the current HEAD now.

dbus.service.Object.__init__(self, dbus.SessionBus(),
MPRISInterface.__path)
self._params = params or {}
self._name = self._params["bus_name"] or "org.mpris.MediaPlayer2.mpd"
if not self._name.startswith("org.mpris.MediaPlayer2."):
logger.warn("Configured bus name %r is outside MPRIS2 namespace" % self._name)

MPRISInterface.__wrapper = mpd_wrapper

self._bus = dbus.SessionBus()
self._uname = self._bus.get_unique_name()
self._dbus_obj = self._bus.get_object("org.freedesktop.DBus",
Expand All @@ -998,7 +1006,8 @@ def _name_owner_changed_callback(self, name, old_owner, new_owner):
except:
pid = None
logger.info("Replaced by %s (PID %s)" % (new_owner, pid or "unknown"))
loop.quit()
if self._loop:
self._loop.quit()

def acquire_name(self):
self._bus_name = dbus.service.BusName(self._name,
Expand All @@ -1022,29 +1031,29 @@ def release_name(self):
}

def __get_playback_status():
status = mpd_wrapper.last_status()
status = MPRISInterface.__wrapper.last_status()
return {'play': 'Playing', 'pause': 'Paused', 'stop': 'Stopped'}[status['state']]

def __set_loop_status(value):
if value == "Playlist":
mpd_wrapper.repeat(1)
if mpd_wrapper._can_single:
mpd_wrapper.single(0)
MPRISInterface.__wrapper.repeat(1)
if MPRISInterface.__wrapper._can_single:
MPRISInterface.__wrapper.single(0)
elif value == "Track":
if mpd_wrapper._can_single:
mpd_wrapper.repeat(1)
mpd_wrapper.single(1)
if MPRISInterface.__wrapper._can_single:
MPRISInterface.__wrapper.repeat(1)
MPRISInterface.__wrapper.single(1)
elif value == "None":
mpd_wrapper.repeat(0)
if mpd_wrapper._can_single:
mpd_wrapper.single(0)
MPRISInterface.__wrapper.repeat(0)
if MPRISInterface.__wrapper._can_single:
MPRISInterface.__wrapper.single(0)
else:
raise dbus.exceptions.DBusException("Loop mode %r not supported" %
value)
return

def __get_loop_status():
status = mpd_wrapper.last_status()
status = MPRISInterface.__wrapper.last_status()
if int(status['repeat']) == 1:
if int(status.get('single', 0)) == 1:
return "Track"
Expand All @@ -1054,32 +1063,32 @@ def __get_loop_status():
return "None"

def __set_shuffle(value):
mpd_wrapper.random(value)
MPRISInterface.__wrapper.random(value)
return

def __get_shuffle():
if int(mpd_wrapper.last_status()['random']) == 1:
if int(MPRISInterface.__wrapper.last_status()['random']) == 1:
return True
else:
return False

def __get_metadata():
return dbus.Dictionary(mpd_wrapper.metadata, signature='sv')
return dbus.Dictionary(MPRISInterface.__wrapper.metadata, signature='sv')

def __get_volume():
vol = float(mpd_wrapper.last_status().get('volume', 0))
vol = float(MPRISInterface.__wrapper.last_status().get('volume', 0))
if vol > 0:
return vol / 100.0
else:
return 0.0

def __set_volume(value):
if value >= 0 and value <= 1:
mpd_wrapper.setvol(int(value * 100))
MPRISInterface.__wrapper.setvol(int(value * 100))
return
Comment on lines 1078 to 1088
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@grawity This is what I was referring to — the setters and getters aren't defined as methods of MPRISInterface, so they don't have access to self.


def __get_position():
status = mpd_wrapper.last_status()
status = MPRISInterface.__wrapper.last_status()
if 'time' in status:
current, end = status['time'].split(':')
return dbus.Int64((int(current) * 1000000))
Expand Down Expand Up @@ -1169,46 +1178,46 @@ def Quit(self):
# Player methods
@dbus.service.method(__player_interface, in_signature='', out_signature='')
def Next(self):
mpd_wrapper.next()
MPRISInterface.__wrapper.next()
return
Comment on lines 1178 to 1182
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@grawity I could use self.__wrapper for these methods, if you prefer. I decided to just be consistent, but I'm open to changing it if that was the wrong call.


@dbus.service.method(__player_interface, in_signature='', out_signature='')
def Previous(self):
mpd_wrapper.previous()
MPRISInterface.__wrapper.previous()
return

@dbus.service.method(__player_interface, in_signature='', out_signature='')
def Pause(self):
mpd_wrapper.pause(1)
mpd_wrapper.notify_about_state('pause')
MPRISInterface.__wrapper.pause(1)
MPRISInterface.__wrapper.notify_about_state('pause')
return

@dbus.service.method(__player_interface, in_signature='', out_signature='')
def PlayPause(self):
status = mpd_wrapper.status()
status = MPRISInterface.__wrapper.status()
if status['state'] == 'play':
mpd_wrapper.pause(1)
mpd_wrapper.notify_about_state('pause')
MPRISInterface.__wrapper.pause(1)
MPRISInterface.__wrapper.notify_about_state('pause')
else:
mpd_wrapper.play()
mpd_wrapper.notify_about_state('play')
MPRISInterface.__wrapper.play()
MPRISInterface.__wrapper.notify_about_state('play')
return

@dbus.service.method(__player_interface, in_signature='', out_signature='')
def Stop(self):
mpd_wrapper.stop()
mpd_wrapper.notify_about_state('stop')
MPRISInterface.__wrapper.stop()
MPRISInterface.__wrapper.notify_about_state('stop')
return

@dbus.service.method(__player_interface, in_signature='', out_signature='')
def Play(self):
mpd_wrapper.play()
mpd_wrapper.notify_about_state('play')
MPRISInterface.__wrapper.play()
MPRISInterface.__wrapper.notify_about_state('play')
return

@dbus.service.method(__player_interface, in_signature='x', out_signature='')
def Seek(self, offset):
status = mpd_wrapper.status()
status = MPRISInterface.__wrapper.status()
current, end = status['time'].split(':')
current = int(current)
end = int(end)
Expand All @@ -1217,12 +1226,13 @@ def Seek(self, offset):
position = current + offset
if position < 0:
position = 0
mpd_wrapper.seekid(int(status['songid']), position)
MPRISInterface.__wrapper.seekid(int(status['songid']), position)
self.Seeked(position * 1000000)
return

@dbus.service.method(__player_interface, in_signature='ox', out_signature='')
def SetPosition(self, trackid, position):
song = MPRISInterface.__wrapper.last_currentsong()
song = mpd_wrapper.last_currentsong()
if not song:
logger.error("Failed to retrieve song position, can't seek")
Expand All @@ -1233,7 +1243,7 @@ def SetPosition(self, trackid, position):
# Convert position to seconds
position = int(position) / 1000000
if position <= int(song['time']):
mpd_wrapper.seekid(int(song['id']), position)
MPRISInterface.__wrapper.seekid(int(song['id']), position)
self.Seeked(position * 1000000)
return

Expand Down Expand Up @@ -1376,8 +1386,6 @@ def usage(params):
usage(params)
sys.exit()

logger = logging.getLogger('mpDris2')
logger.propagate = False
logger.setLevel(log_level)

# Attempt to configure systemd journal logging, if enabled
Expand Down Expand Up @@ -1481,11 +1489,8 @@ def usage(params):
logger.debug('Using legacy pygobject2 main loop.')
loop = GLib.MainLoop()

# Wrapper to send notifications
notification = NotifyWrapper(params)

# Create wrapper to handle connection failures with MPD more gracefully
mpd_wrapper = MPDWrapper(params)
mpd_wrapper = MPDWrapper(params, loop=loop)
mpd_wrapper.run()

# Run idle loop
Expand Down