Feat: Add audio volume slider#691
Conversation
CyanVoxel
added
Type: QoL
VasigaranAndAngel
left a comment
VasigaranAndAngel
left a comment
There was a problem hiding this comment.
my suggestions
| self.volume_slider.setValue(int(self.player.audioOutput().volume() * 100)) | ||
| else: | ||
| self.player.audioOutput().setMuted(True) | ||
| self.volume_slider.setValue(0) |
There was a problem hiding this comment.
my suggestion is that toggling mute should only toggle mute state of the player leaving the slider untouched. that way users have more control over the volume. even when it's muted. setting the slider to 0 when muted still allows users to adjust the volume. but it doesn't unmute when the slider is changed. which feels a bit unintuitive atleast for me 😅. or it would make more sense if adjusting the slider automatically unmutes the audio.
| self.media_btns_layout.addWidget(self.mute) | ||
|
|
||
| self.volume_slider = QSlider() | ||
| self.volume_slider.setValue(50) |
There was a problem hiding this comment.
it's explicitly setting only the volume slider to 50% but not the player's volume. either make the slider reflect the player's current volume or make the player's volume reflect slider's volume. (moving this line after the one that connects the valueChanged signal ensures the player's volume matches the slider's volume. 50% in this case.)
| self.volume_slider = QSlider() | ||
| self.volume_slider.setValue(50) | ||
| self.volume_slider.setOrientation(Qt.Orientation.Horizontal) | ||
| self.volume_slider.sliderMoved.connect(self.volume_slider_changed) |
There was a problem hiding this comment.
| self.volume_slider.sliderMoved.connect(self.volume_slider_changed) | |
| self.volume_slider.valueChanged.connect(self.volume_slider_changed) |
using the valueChanged signal is the proper way to handle a QSlider. the sliderMoved signal only triggers when the slider is dragged. but qslider also supports scrolling, mouse clicks, keyboard controls, and more.
| self.position_label.setText(f"{current} / {duration}") | ||
|
|
||
| def volume_slider_changed(self, position: int) -> None: | ||
| """Position is divided by 100 since volume is between 0.0f and 1.0f.""" |
There was a problem hiding this comment.
shouldn't method's docstring describe what the method does? using comments to explain specific parts of a line seems like a better approach.
VasigaranAndAngel
left a comment
VasigaranAndAngel
left a comment
There was a problem hiding this comment.
thanks for the changes ❤️. everything lgtm. minor suggestions (okay to ignore),
| """Toggle the mute state of the media.""" | ||
| if self.player.audioOutput().isMuted(): | ||
| self.player.audioOutput().setMuted(False) | ||
| self.volume_slider.setValue(int(self.player.audioOutput().volume() * 100)) |
CyanVoxel
added
Status: Changes Requested
Co-authored-by: VasigaranAndAngel <72515046+VasigaranAndAngel@users.noreply.github.com>
|
sorry for taking a long time been busy with school |
CyanVoxel
left a comment
CyanVoxel
left a comment
There was a problem hiding this comment.
Thank you so much for this, and thank you @VasigaranAndAngel for reviewing!
3 participants
In the name!
got absolutely terrified by the sheer volume of an audio file with no way to change it so I added it
this just simply adds a volume slider next to the mute button
I also updated the roadmap to reflect the features relating to audio that are currently in tagstudio