Feat: Add audio volume slider #691
Conversation
CyanVoxel
added
Type: QoL
| 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.
| @@ -90,6 +90,13 @@ def __init__(self, driver: "QtDriver") -> None: | |||
|
|
|||
| 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.
| @@ -207,3 +216,7 @@ def media_status_changed(self, status: QMediaPlayer.MediaStatus) -> None: | |||
| current = self.format_time(self.player.position()) | |||
| duration = self.format_time(self.player.duration()) | |||
| 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.
| @@ -133,6 +141,7 @@ def toggle_mute(self) -> None: | |||
| """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 <[email protected]>
|
sorry for taking a long time been busy with school |
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