Lazy load AudioDevice properties, add additional functions #97

timtucker · 2024-09-21T03:26:14Z

Made a few tweaks:

Changed to lazy load AudioDevice properties -- this should be a little faster in cases where the properties otherwise go unused and helps get around some of the issues with exceptions being thrown when accessing the properties on system devices
Updated associated tests for CreateDevice to include manually accessing the properties -- with the update to lazy load, one of the tests failed as-is because it was expecting a failure that was no longer thrown until you manually access properties
Added a few functions to make it easier to enumerate things & access properties -- as an example, AudioDevice now has a Sessions property that can be used to access the list of sessions for the device
Did some refactoring to move duplicate code into helper functions
Tried to make variable names more consistent in spots where the same type of object was being referred to with different names
Added typing in multiple spots for better IDE auto-completion

Note that in every case where I could I kept the behavior the same.

Changed behavior from the current release:

Getting session by process id. This now looks at all sessions, regardless of which device they're associated with.

Not changed, but probably should change:

"GetAllSessions" -- it doesn't actually return "all" sessions. It just returns sessions associated with the default playback device.

A few of the areas where that might not be all:

Recording-only sessions
Windows configured to output sounds for different applications to different devices
Windows configured to have different output devices as the default for playback vs. communications

…ve redundant code into helper functions, make variable names more consistent, add typing

AndreMiras

Thanks for the refactoring, the main idea is good, the refactoring makes sense and the lazy loading is a nice addition.
The pull request is a bit too big for my taste, I like to break down into smaller pull requests when possible so it's easier to keep focus when reviewing and easier to git bisect it when there's a regression. Also small PRs get less comments and get merged faster.
Thanks for bringing some type hints, I like them too, in the future we would need to lint them to make sure they're not lies, but let's keep it for another PR.

AndreMiras · 2024-09-24T08:49:44Z

pycaw/utils.py

+        # TODO: the current behavior here isn't really getting "all sessions"
+        #       Leaving behavior as-is for backward compatibility


Thanks I appreciate the comment

AndreMiras · 2024-09-24T08:52:23Z

pycaw/utils.py

+        flow=EDataFlow.eAll.value, deviceState=DEVICE_STATE.ACTIVE.value
+    ) -> List[AudioDevice]:
+        """
+        Get devices based on filteres for flow direction and device state.


s/filteres/filters/

AndreMiras · 2024-09-24T09:05:44Z

tests/test_core.py

        with warnings.catch_warnings(record=True) as w:
-            AudioUtilities.CreateDevice(dev)
+            device = AudioUtilities.CreateDevice(dev)
+            # Properties are lazy-loaded and won't throw exceptions
+            # until accessed
+            dev = device.properties
        assert len(w) == 1


Thanks for updating the tests too.
Maybe we should take the CreateDevice() call out of the context manager and only put the property access into it to prove that it's indeed raising only with this specific line.
So test would be:

AudioUtilities.CreateDevice(dev) device = AudioUtilities.CreateDevice(dev) with warnings.catch_warnings(record=True) as w: # Properties are lazy-loaded and won't throw exceptions # until accessed device.properties assert len(w) == 1

AndreMiras · 2024-09-24T09:12:30Z

pycaw/utils.py

+    @staticmethod
+    def getProperty(self, key):
+        store = self._dev.OpenPropertyStore(STGM.STGM_READ.value)
+        if store is None:
+            return None


Given this is a static method, maybe let's call that self obj instead or something to avoid confusion, what do you think?

Can't remember why I added this as a static method -- this should probably just be
@property def property(self, key):

Yes indeed it's really only consumed by the audio device object so that would be the more logical change

AndreMiras · 2024-09-24T09:18:52Z

pycaw/utils.py

+            for j in range(propCount):
+                pk = store.GetAt(j)
+
+                value = AudioDevice.getProperty(self, pk)
+                if value is None:
+                    continue


that feels like O(n*n/2) given that getProperty will loop all over again. No big deal at performance level, but just got me curious if there's not a more elegant approach

If getProperty gets changed to just property & directly accesses by the name, that would help

AndreMiras · 2024-09-24T09:24:29Z

pycaw/utils.py

+
+    @property
+    def properties(self) -> dict:
+        if self._properties is None:


Are you familiar with guard clauses?
Basically inverting the condition to exit early and deal with the rest of the code one level of indentation lower:

if self._properties is not None: return self._properties store = self._dev.OpenPropertyStore(STGM.STGM_READ.value) ...

And then do not else, but rest of the code.
That saves one indentation level and the brain doesn't have to stack level of logic when tracking the code.
This is relatively simple code of course, but it's a good practice to generalize to make things easier to follow

AndreMiras · 2024-09-24T09:25:55Z

pycaw/utils.py

+    def DisplayName(self, value: str) -> str:
        s = self._ctl.GetDisplayName()
        if s != value:
            self._ctl.SetDisplayName(value, IID_Empty)


Comments (and typehints) are future lies unless tested.
Are we sure this method returns a string?

timtucker · 2024-09-25T17:31:13Z

Thanks for the refactoring, the main idea is good, the refactoring makes sense and the lazy loading is a nice addition. The pull request is a bit too big for my taste, I like to break down into smaller pull requests when possible so it's easier to keep focus when reviewing and easier to git bisect it when there's a regression. Also small PRs get less comments and get merged faster. Thanks for bringing some type hints, I like them too, in the future we would need to lint them to make sure they're not lies, but let's keep it for another PR.

Agreed that this is definitely a lot -- I started working on a small project locally and started doing some late-night tinkering and just figured I'd upload what I had done so far.

IMO, part of the issue is that there's too much in utils right now.

Changes might be easier to keep narrower in scope if it were split up something like:

audio\device.py

class to interact with a specific device
instances could cache for device properties or interfaces that are unlikely to change

audio\session.py

class to interact with a specific session
instances could cache of session properties or interfaces that are unlikely to change

audio\controllers.py

class to list & interact with devices, with caching of previously seen devices
class to list & interact with sessions, with caching of previously seen sessions
default instances of the session & device controllers that are aware of one another
Example usage would just be: from pycaw.audio.controllers import audio_sessions, audio_devices

audio\types.py

type definitions to avoid circular imports

utils.py

stubs that use the default controllers + new interfaces for backward compatibility

AndreMiras · 2024-09-28T08:43:41Z

Yes you're right that this should be split like you said.
Let's keep this for a follow up PR, because I don't want to move things around too much and it would be harder to track the rest of the changes.

timtucker added 3 commits September 20, 2024 10:04

Lazy load AudioDevice properties, add additional helper functions, mo…

6dc20f4

…ve redundant code into helper functions, make variable names more consistent, add typing

lint

25fcce2

Remove unused import

ff90192

AndreMiras reviewed Sep 24, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Lazy load AudioDevice properties, add additional functions #97

Lazy load AudioDevice properties, add additional functions #97

timtucker commented Sep 21, 2024 •

edited

Loading

AndreMiras left a comment

AndreMiras Sep 24, 2024

AndreMiras Sep 24, 2024

AndreMiras Sep 24, 2024

AndreMiras Sep 24, 2024

timtucker Sep 25, 2024

AndreMiras Sep 28, 2024

AndreMiras Sep 24, 2024

timtucker Sep 25, 2024

AndreMiras Sep 24, 2024

AndreMiras Sep 24, 2024

timtucker commented Sep 25, 2024

AndreMiras commented Sep 28, 2024

		# TODO: the current behavior here isn't really getting "all sessions"
		# Leaving behavior as-is for backward compatibility

Lazy load AudioDevice properties, add additional functions #97

Are you sure you want to change the base?

Lazy load AudioDevice properties, add additional functions #97

Conversation

timtucker commented Sep 21, 2024 • edited Loading

AndreMiras left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timtucker commented Sep 25, 2024

AndreMiras commented Sep 28, 2024

timtucker commented Sep 21, 2024 •

edited

Loading