Skip to content

feat: Better Players - #1053

Draft
nnra6864 wants to merge 12 commits into
commetchat:mainfrom
nnra6864:better-players
Draft

nnra6864 wants to merge 12 commits into
commetchat:mainfrom
nnra6864:better-players

Conversation

@nnra6864

Copy link
Copy Markdown

This PR aims to implement basic controls to media players, such as volume changing.
I'll also look into implementing .mkv and .jxl support.
I could also try implementing #1052 if it's not merged by the time I get to it.

For now, I implemented the persistent audio player volume control, and will start working on the video player.
image

If you'd like, I can split the format support into a separate PR, whatever works the best for you.

closes #1051

fix: Saving the preMuteVolume instead of regular volume to prefs

style: Adjusted naming

style: Reordered func for consistency
@nnra6864

Copy link
Copy Markdown
Author

Hey, I skimmed through the video player implementation yesterday and realized that a lot of the code would be duplicated if I just copied the volume implementation.
Would you like me to extract volume control/widgets into a separate class, and see if some other controls could be extracted into that class too?

I am also open to suggestions as to how this could be made better.
The PR so far is only a rough implementation, and I already have a few minor improvements in my mind.

Comment thread commet/lib/ui/molecules/audio_player/audio_player.dart Outdated
Comment thread commet/lib/ui/molecules/audio_player/audio_player.dart
@nnra6864

nnra6864 commented Oct 8, 2026

Copy link
Copy Markdown
Author

Hey, sorry for taking a while, I've been a bit busy.

I tried my absolute best for now to implement this properly, but due to lack of experience with flutter, and the fact I am not allowed to use ai, I am finding it very difficult.
The commit I pushed above "works", but it doesn't receive updates to the volume if another player changes it, and I can't figure out why the volume change event is never triggered.

On top of that, I am not even sure if this is the correct approach, I found the video player separation way more confusing to follow compared to the audio player, where it was all defined in a single class.

Also, there's currently a bug where hovering over the volume slider makes the video overlay close, resulting in the slider flickering all the way to the left of the window and then disappearing, I am genuinely not sure how to fix this.

And as a final note, I decided to move away from preMuteVolume to appliedVolume, as it requires way less mental overhead when used.
If you agree with this design decision, I'd be up to make the audio player work the same way.

@Airyzz

Airyzz commented Oct 8, 2026

Copy link
Copy Markdown
Contributor

Hey, sorry for taking a while, I've been a bit busy.

I tried my absolute best for now to implement this properly, but due to lack of experience with flutter, and the fact I am not allowed to use ai, I am finding it very difficult. The commit I pushed above "works", but it doesn't receive updates to the volume if another player changes it, and I can't figure out why the volume change event is never triggered.

On top of that, I am not even sure if this is the correct approach, I found the video player separation way more confusing to follow compared to the audio player, where it was all defined in a single class.

Also, there's currently a bug where hovering over the volume slider makes the video overlay close, resulting in the slider flickering all the way to the left of the window and then disappearing, I am genuinely not sure how to fix this.

And as a final note, I decided to move away from preMuteVolume to appliedVolume, as it requires way less mental overhead when used. If you agree with this design decision, I'd be up to make the audio player work the same way.

Hey, no problem at all, thank you for putting in the work, and thank you for adhering to the guidelines! Admittedly, the video player code is a bit of a mess, I'll take a look in to it, and see if I can provide any guidance

@Airyzz

Airyzz commented Oct 8, 2026

Copy link
Copy Markdown
Contributor

Fixed up a couple things, let me know what you think!

And as a final note, I decided to move away from preMuteVolume to appliedVolume, as it requires way less mental overhead when used.
If you agree with this design decision, I'd be up to make the audio player work the same way.

Honestly I dont particularly mind either way, I think both approaches are fine, if you would like to make them both the same feel free to, but i'm happy to merge it either way

@nnra6864

nnra6864 commented Oct 8, 2026

Copy link
Copy Markdown
Author

Yep, this works flawlessly now!
I don't know why I thought volumes of all the players updated when you changed one, I must be misremembering how it worked.
Either way, it feels consistent now between the audio and video player, that's what matters.

I will make the audio player use the same logic as the video player to keep it consistent for future updates.

Also, since volume controls are now implemented, should I open separate PRs for other features I suggested, or should I try to implement them in this one?

} else {
setState(() {
isMuted = false;
appliedVolume = preferences.playerVolume.value;

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Question, why do we set appliedVolume to the value from preferences instead of just setting it to volume?
Unless I am missing something, it should do the same thing, but I'd assume preferences would be slower, no?
Anyways, for consistency, I'll do the same in the audio player, but if you'd like this changed again, feel free to let me know.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Feature] Better media player

2 participants