Replace simple audio #730
Replace simple audio #730
Conversation
WalkthroughThe pull request introduces several modifications across multiple files, primarily focusing on the GitHub Actions workflow for building and deploying AYAB release packages on various operating systems. Enhancements include improved caching strategies, updates to Python versions, and changes to audio handling in the application code. The Changes
Possibly related PRs
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (2)
src/main/python/main/ayab/audio.py (2)
Line range hint
44-48: Check ifQSoundEffectis loaded before playing to avoid runtime errorsBefore calling
play()on aQSoundEffectinstance, it's advisable to check if the sound effect is loaded and ready. This ensures that the sound will play as expected and helps avoid potential issues if the sound file failed to load.Apply this diff to add a status check:
wave_obj = self.__wave(sound) if wave_obj is None: return # else +if wave_obj.status() != QSoundEffect.Status.Ready: + return wave_obj.play()
37-38: Consider implementing cache eviction to manage memory usageCurrently,
self.__cachestores every loadedQSoundEffectindefinitely. If the application plays a large number of different sounds, this may lead to increased memory usage over time. Consider implementing a cache eviction policy or limiting the cache size to manage resources effectively.For example, you could limit the cache to a maximum number of sounds:
MAX_CACHE_SIZE = 10And modify the caching logic accordingly.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
.github/workflows/build-multi-os.yml(0 hunks)mypy.ini(0 hunks)requirements.build.txt(0 hunks)src/main/python/main/ayab/audio.py(4 hunks)
💤 Files with no reviewable changes (3)
- requirements.build.txt
- mypy.ini
- .github/workflows/build-multi-os.yml
🔇 Additional comments (1)
src/main/python/main/ayab/audio.py (1)
Line range hint 1-64: Verify that all references to simpleaudio have been removed from the codebase
As per the PR objectives, you've attempted to remove all dependencies on simpleaudio. To ensure that no references remain, consider running a search across the codebase.
Run the following script to check for any remaining references to simpleaudio:
This will list any files and lines where 'simpleaudio' is still referenced.
✅ Verification successful
All references to simpleaudio have been successfully removed
The comprehensive search across the codebase, including case-insensitive matches and import statements, shows no remaining references to simpleaudio. The code has been successfully migrated to use QSoundEffect from PySide6.QtMultimedia for audio playback.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for any occurrences of 'simpleaudio' in the codebase.
# Search for 'simpleaudio' in Python files
rg 'simpleaudio' --glob '*.py'
# Search for 'simpleaudio' in requirements and setup files
rg 'simpleaudio' --glob 'requirements*.txt' --glob 'setup.py' --glob 'mypy.ini'
Length of output: 106
Script:
#!/bin/bash
# Let's try a more comprehensive search including git history and different case variations
# Search for any case variations of simpleaudio in all files
rg -i "simpleaudio"
# Search in git history for recently removed references
git log -p --since="1 month ago" | rg -i "simpleaudio"
# Check for any remaining .wav file handling that might indicate legacy code
ast-grep --pattern 'import $_ from "simpleaudio"'
ast-grep --pattern 'import simpleaudio'
Length of output: 113
|
I create a test release under https://github.com/eRnEgIi/ayab-desktop/releases and tested the sound under windows. It was still working. Linux app-image and MacOS is still untested. Maybe I will find time to test the app image, but I am unable to test the MacOS version. In theory both should also work... but you never know :) |
|
@eRnEgIi could you please have a look at the suggestions by @coderabbitai? |
|
@dl1com: I have reviewed the latest changes, and it appears that the previous suggestions have been addressed successfully. There are no remaining references to 🔗 Analysis chain🏁 Scripts executedThe following scripts were executed for the analysis: Script: Length of output: 189 If you found this review helpful, would you consider giving us a shout-out on X? ✅ Actions performedComments resolved. |
|
LGTM, planning for next release after 1.0 |
|
@dl1com thank you for having a look. I am a little confused. Is there anything I should still have a look at or is everything already fine? |
It's OK for now, thanks. We'll merge it after release of 1.0 |
This is an attempt to solve the issue #725 by replacing
simpleaudiowithQSoundEffectfromPySide6.QtMultimedia.It works locally with ubuntu 24.04 and Python 3.12.
I have also tried to remove all dependencies, but I am not sure if I got everything.
Other versions are currently not tested. If I find time I will try to test it at least under windows.
However this is also some kind of playground for me to figure out how to trigger github actions to build the image from the existing
build-multi-osworkflow. (I have rarely used github at all)Summary by CodeRabbit
New Features
QSoundEffectfor improved performance and compatibility.Bug Fixes
simpleaudiodependency, which may affect audio playback functionality.Documentation