AndreMiras / pycaw

Python Core Audio Windows Library
MIT License
385 stars 67 forks source link

Lazy load AudioDevice properties, add additional functions #97

Open timtucker opened 1 month ago

timtucker commented 1 month ago

Made a few tweaks:

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:

timtucker commented 1 month ago

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

audio\session.py

audio\controllers.py

audio\types.py

utils.py

AndreMiras commented 1 month ago

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.