feat: Audio Playback #576
feat: Audio Playback #576
Conversation
Add the ability to play audio files. Add a slider to seek through an audio file. Add play/pause and mute/unmute buttons for audio files. Note: This is a continuation of a mistakenly closed PR: Ref: TagStudioDev#529 While redoing the changes, I made a couple of improvements. When the end of the track is reached, the pause button will swap to the play button and allow the track to be replayed. Here is the original feature request: Ref: TagStudioDev#450
CyanVoxel
added
Type: UI/UX
There was a problem hiding this comment.
It starts playing the audio once it's loaded, even when the player is paused (pause button is visible). Please fix this by preventing it from autoplaying when the player is paused. Also, make sure the code is formatted with Ruff and passes MyPy checks. (Install pre-commit if you haven’t already.)
|
Ah, nice catch! I'll get working on that. |
| from PySide6.QtCore import ( | ||
| Qt, | ||
| QUrl, | ||
| ) |
There was a problem hiding this comment.
| from PySide6.QtCore import ( | |
| Qt, | |
| QUrl, | |
| ) | |
| from PySide6.QtCore import Qt, QUrl |
| self.base_size: tuple[int, int] = (266, 75) | ||
|
|
||
| self.setMinimumSize(*self.base_size) | ||
| self.setMaximumSize(*self.base_size) |
There was a problem hiding this comment.
why do we need to set the size explicitly? this widget is added to a QSplitter and that will automatically adjust the size of its child widgets. setting the size of this widget causes all the widgets in that QSplitter to follow the same fixed width. i'm not sure why but my assumption is that in a QSplitter the layout of child widgets is typically controlled by the available space and stretch factors of each widget. when we set one widget's width to fixed size, the QSplitter tries to respect that width when distributing the remaining space among the other widgets.
btw, using setFixedSize() is basically the same as using both setMinimumSize() and setMaximumSize() if you weren't aware.
| self.base_size: tuple[int, int] = (266, 75) | |
| self.setMinimumSize(*self.base_size) | |
| self.setMaximumSize(*self.base_size) |
There was a problem hiding this comment.
I agree setting the size explicitly isn't the best approach (didn't know it would have those side effects). I think I'm probably going to have to look into a different way to size the widget though.
When I remove the min/max size, the audio widget grows/shrinks as expected. However, after a certain point the UI looks overstretched. I'll do some more research soon.
Also open to suggestions if you think of any.
There was a problem hiding this comment.
can you help me to reproduce that issue?
There was a problem hiding this comment.
Sure. If you apply your change and resize the audio player widget, the buttons/slider will space out (note: I've made a few UI tweaks).
This is normal:
This is after resizing the audio player widget:
I think it would be nice if the buttons stay more compact. Thoughts?
There was a problem hiding this comment.
What if I just do setFixedHeight() on the audio player?
Not sure how I feel about it, but I don't think it would affect the other widgets in the preview panel.
There was a problem hiding this comment.
that is the expected behavior. the self.base_layout (QVBoxLayout) stretches the widgets as the AudioPlayer widget resizes.
and yes, using setFixedHeight() will fix that. i didn’t notice any issues with other widgets in the preview panel when setting a fixed height.
| self.player.setSource(QUrl().fromLocalFile(self.filepath)) | ||
| self.play() | ||
| else: | ||
| self.player.setSource(QUrl().fromLocalFile(self.filepath)) |
There was a problem hiding this comment.
QUrl.fromLocalFile is a staticmethod.
| self.player.setSource(QUrl().fromLocalFile(self.filepath)) | |
| self.play() | |
| else: | |
| self.player.setSource(QUrl().fromLocalFile(self.filepath)) | |
| self.player.setSource(QUrl.fromLocalFile(self.filepath)) | |
| self.play() | |
| else: | |
| self.player.setSource(QUrl.fromLocalFile(self.filepath)) |
| def close(self, *args, **kwargs) -> bool: | ||
| self.player.stop() | ||
| return super().close(*args, **kwargs) | ||
|
|
There was a problem hiding this comment.
What is the purpose of overriding the close method of QWidget? this method is used to handle the closing process when the widget is opened as a window right?
| def close(self, *args, **kwargs) -> bool: | |
| self.player.stop() | |
| return super().close(*args, **kwargs) |
|
@VasigaranAndAngel, is it preferred that I commit your suggestions in github or make the changes locally? |
i’ve suggested changes because it’s easier for you, but if you prefer making changes locally, feel free to do so and commit them. it’s up to you. |
|
This is a much requested feature and necessary for parity with videos, thanks! |
|
@seakrueger, I see what you mean. There is a lot of duplication, and I agree that the two should probably me merged. My original intent was to start out small with a separate widget, and eventually suggest merging the audio/video player. I think I would be okay trying the merge in this PR, but I would like to get some feedback in terms of UI/Implementation before attempting to merge. @CyanVoxel and @VasigaranAndAngel, any thoughts? |
|
as seakrueger suggested, subclassing a multimedia widget is what i’d recommend as well. using the same |
|
Having a unified player would certainly be nice, but would take second priority behind having any audio player at all in the meantime. As for specific UI suggestions, I feel that Chrome's native media player always did a good job of staying minimal while giving you a decent basic control set, so you wouldn't go wrong trying to emulate a similar UI/UX to that. For audio files, I feel we have the advantage of the waveform and album art thumbnails which could be displayed in place of where a video would otherwise be. |
Added a MediaPlayer base class per some suggestions in the PR comments. Hopefully this reduces duplicate code between the audio/video player in the future.
|
Okay, I did some work and ended up with a Let me know of any thoughts or suggestions. Thank you all! |
|
wow the new audio player widget looks awesome 😍 i haven’t dived deep into the code, but here are a few things that caught my eye: |
|
@VasigaranAndAngel, thanks! I updated the code to use On moving the media controls to the Any thoughts on this? I appreciate your time reviewing this with me. |
isn’t that the goal of creating a base class? if we add media control buttons to both the video player and audio player separately, it would still involve duplication, right? but it’s okay to duplicate if the control buttons for video and audio players are intended to be different.
I'm okay with that idea. but i'm not sure if it's possible to make the media player efficiently show thumbnail if it's playing audio and show the video if it's playing video. we can adjust it as needed when porting the video player. so it’s fine for now. |
Move media controls from the AudioPlayer widget to the MediaPlayer base class. This removes the need for a separate AudioPlayer class, and allows the video player to reuse the media controls.
Update the position_label when the slider is moving.
That's a fair point. With the most recent commit, I've added the media controls to the base Also, I made another commit to fix an issue with the player position label. Before, it wouldn't update when the slider was being moved. Now, it should update with the slider. |
Add the ability to play audio files.
Add a slider to seek through an audio file.
Add play/pause and mute/unmute buttons for audio files.
Note: This is a continuation of a mistakenly closed PR:
Ref: #529
While redoing the changes, I made a couple of improvements. When the end of the track is reached, the pause button will swap to the play button and allow the track to be replayed.
Here is the original feature request:
Ref: #450