Skip to content
This repository has been archived by the owner on Aug 23, 2024. It is now read-only.

Fix MPRIS volume integration #311

Merged
merged 2 commits into from
Apr 28, 2022
Merged

Fix MPRIS volume integration #311

merged 2 commits into from
Apr 28, 2022

Conversation

gelaechter
Copy link
Contributor

MPRIS volume changes now affect the volume slider.
Volume changes are now sent back to MPRIS.

Comment on lines 202 to 208
// Handle mpris volume in/out
useEffect(() => {
const volume = Number(playQueue.volume.toPrecision(2));
setLocalVolume(volume);
ipcRenderer.send('volume', volume);
}, [playQueue.volume]);

Copy link
Owner

Choose a reason for hiding this comment

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

I'm thinking it might be better to put this in the usePlayerControls hook since all the MPRIS controls are currently located there.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you want to me match your implemented of the other MPRIS controls (getting an event from the main process)? Or do you just want to move the function over to usePlayerControls?

Copy link
Owner

Choose a reason for hiding this comment

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

I think it would actually make sense to move the ipcRenderer.send('volume', volume) within the volume handlers themselves handleVolumeSlider, handleVolumeKey, and handleVolumeWheel.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Alright, I moved the MPRIS updates into the volume handlers, which like you said makes more sense :)
I left the MPRIS handler where it was and put an update into the handler for the volume slider inside PlayerBar.tsx so that MPRIS gets the volume at startup.

Copy link
Owner

Choose a reason for hiding this comment

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

Thanks. I'll test it out later today (have to reboot into Linux) and then merge if I don't find any issues.

@jeffvli jeffvli merged commit 5e0f04b into jeffvli:main Apr 28, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants