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

ferdnyc · 2021-12-01T05:04:39Z

This PR contains a set of what I'm calling "quality of life" changes for the code. Nothing user-visible, this is all an attempt to make the MPDWrapper class and its helpers more self-contained, and importable/usable outside of the mpDris2 source file itself — for testing/debugging purposes. The user should see absolutely no difference in normal operation.

Specifically, this PR eliminates all references from inside the class code to symbols that are only defined if the if __name__== '__main__' block is run. As a result, it's now possible to import the class into an interactive session and call methods on it by hand, without it crashing due to things like mpd_wrapper and loop being undefined, or notification not being callable because its value is None.

Specific changes (as separate commits):

Move logger instantiation to top of module (the level is still configured in the __main__-only code)
Set up notification in the MPDWrapper.__init__() code as self._notification, since it's never used in the __main__ code but all of the class code assumes it's been properly initialized
Still define loop only in the __main__ code (I agree that's a good idea), but pass it in to the MPDWrapper() constructor and use self._loop to access it later.
Pass the MPDWrapper instance in to MPRISInterface() along with the params, and store it as MPRISInterface.__wrapper. Then, use that to make calls back to it, instead of trying to use mpd_wrapper which is undefined if the __main__ code wasn't executed

With these changes, it's possible to debug the code interactively via the REPL (after running ln -s mpDris2 mpDris2.py), all that's required is to set up the MainLoop and create a params dictionary, then pass them both to MPDWrapper() thusly:

>>> import mpDris2
>>> mpDris2.DBusGMainLoop(set_as_default=True); loop = mpDris2.GLib.MainLoop()
<dbus.mainloop.NativeMainLoop object at 0x7f6fc6a9e4f0>
>>> mpdw = mpDris2.MPDWrapper({'password': 'XXXXXXX', 'host': 'XXXXXXX', 'port': 6600,
... 'mmkeys': False, 'progname': 'mpdris2', 'bus_name': None, 'notify_urgency': 0,
... 'notify': True, 'cover_regex': None, 'music_dir': ''}, loop=loop)
>>> mpdw.my_connect()
False
>>> mpdw.connected
True
>>> status = mpdw.status()
>>> from pprint import pp
>>> pp(status)
{'volume': '58',
 'repeat': '0',
 'random': '0',
 'single': '0',
 'consume': '0',
 'partition': 'default',
 'playlist': '8',
 'playlistlength': '6',
 'mixrampdb': '0',
 'state': 'play',
 'xfade': '4',
 'song': '2',
 'songid': '3',
 'time': '4:2354',
 'elapsed': '3.995',
 'bitrate': '192',
 'duration': '2353.848',
 'audio': '48000:24:2',
 'nextsong': '3',
 'nextsongid': '4'}
>>> mpdw.pause()

(...and it does.)

src/mpDris2.in.py

grawity

3f4d8c4 - I'm not sure I understand this one; maybe it's just been too long since I looked at this code, but why is __wrapper being made a class property? If it's assigned in __init__(), then it should probably be an instance property (i.e. self._wrapper).

ferdnyc · 2021-12-02T01:35:06Z

3f4d8c4 - I'm not sure I understand this one; maybe it's just been too long since I looked at this code, but why is __wrapper being made a class property? If it's assigned in __init__(), then it should probably be an instance property (i.e. self._wrapper).

Normally, yes, I'd agree. But that didn't work, because the callback functions aren't methods — they don't have access to self. I assume it's something to do with how the DBus interface uses them? It was like that when I found it, and I didn't want to try and change it since that would've been much more invasive. So this was the simpler path towards making sure they could access the wrapper object.

ferdnyc · 2021-12-02T01:35:33Z

(Forgot the ping: @grawity )

ferdnyc · 2021-12-02T02:32:52Z

src/mpDris2.in.py

    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


@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.

ferdnyc · 2021-12-02T02:33:57Z

src/mpDris2.in.py

    # Player methods
    @dbus.service.method(__player_interface, in_signature='', out_signature='')
    def Next(self):
-        mpd_wrapper.next()
+        MPRISInterface.__wrapper.next()
        return


@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.

eonpatapon · 2021-12-13T08:31:49Z

src/mpDris2.in.py

@@ -247,7 +248,7 @@ class MPDWrapper(object):
        errors and similar
    """

-    def __init__(self, params):
+    def __init__(self, params, loop=None):


Shouldn't loop be a required argument ?

@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).

eonpatapon · 2021-12-13T09:03:28Z

src/mpDris2.in.py


-    def __init__(self, params):
+    def __init__(self, params, mpd_wrapper=None):


mpd_wrapper is required too

@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.

eonpatapon · 2021-12-13T09:09:48Z

Pushed 6b246e4 to see what it needs to make the wrapper an instance of the class. Feels a bit more idiomatic python to me

Overall it's great to be able to run mpDris2 in the repl

ferdnyc added 4 commits November 30, 2021 21:53

Instantiate logger at top of module

bc474c6

Pass loop instance to NotifyWrapper

ccd8394

Pass wrapper instance to MPRISInterface()

3f4d8c4

Set up notifications from inside MPDWrapper()

6e71969

grawity reviewed Dec 1, 2021

View reviewed changes

src/mpDris2.in.py Show resolved Hide resolved

grawity reviewed Dec 1, 2021

View reviewed changes

Merge branch 'master' into repl-usability

a57828a

ferdnyc commented Dec 2, 2021

View reviewed changes

eonpatapon reviewed Dec 13, 2021

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

ferdnyc commented Dec 1, 2021

grawity left a comment •

edited

Loading

ferdnyc commented Dec 2, 2021

ferdnyc commented Dec 2, 2021

ferdnyc Dec 2, 2021

ferdnyc Dec 2, 2021

eonpatapon Dec 13, 2021

ferdnyc May 31, 2022

eonpatapon Dec 13, 2021

ferdnyc May 31, 2022

eonpatapon commented Dec 13, 2021


		def __init__(self, params):
		def __init__(self, params, mpd_wrapper=None):

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

Are you sure you want to change the base?

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

Conversation

ferdnyc commented Dec 1, 2021

grawity left a comment • edited Loading

Choose a reason for hiding this comment

ferdnyc commented Dec 2, 2021

ferdnyc commented Dec 2, 2021

ferdnyc Dec 2, 2021

Choose a reason for hiding this comment

ferdnyc Dec 2, 2021

Choose a reason for hiding this comment

eonpatapon Dec 13, 2021

Choose a reason for hiding this comment

ferdnyc May 31, 2022

Choose a reason for hiding this comment

eonpatapon Dec 13, 2021

Choose a reason for hiding this comment

ferdnyc May 31, 2022

Choose a reason for hiding this comment

eonpatapon commented Dec 13, 2021

grawity left a comment •

edited

Loading