Add a slider for every sink to the MPRIS Activity. The sink can be muted by clicking on the volume icon.
.
See D7992
BUG: 377319
Details
- Reviewers
mtijink albertvaka - Group Reviewers
KDE Connect - Maniphest Tasks
- T4659: Add general system volume control to MPRIS controls using pulseaudio
- Commits
- R225:f9c30148b7ab: System volume plugin Android
Diff Detail
- Repository
- R225 KDE Connect - Android application
- Branch
- arcpatch-D7993_2
- Lint
No Linters Available - Unit
No Unit Test Coverage
Looks ok to me (and it works). But why is this implemented as a separate plugin?
I think it needs to be able to see the current volume (also after volume changes, so maybe libpulse is required anyway) to be really useful, though.
src/org/kde/kdeconnect/Plugins/MprisPlugin/MprisActivity.java | ||
---|---|---|
418 | Some code should be added to hide the "set system volume" controls if the plugin is not available. |
First I had it as part of the MPRIS Plugin but I decided to put it into a separate plugin because of the following reasons:
- pactl or even pulseaudio could not be available
- either the desktop or the client part could not be up to date and lack the required code
Both would require a lot of conditional code inside the MPRIS Plugin that handles all those cases. If we have separate plugins we just check those conditions when loading the plugins and enable/disable the systemvolume plugin. Also the functionality is quite disjoint (althought of course somewhat related) so a separate plugin is more clear and maintainable. They only share an UI, but in my current work I encapsulate the UI parts into a own Fragment that can be easily moved to/reused at other places.
I think it needs to be able to see the current volume (also after volume changes, so maybe libpulse is required anyway) to be really useful, though.
That's why it's work in progress :)
I think I can publish my current work soon-ish
Extract control slider into a fragment.
Preparation, but still no support for multiple sinks.
I added a couple of code comments.
In general: this doesn't fit in the MPRIS controls on landscape mode (I'd say at most one slider fits).
res/layout/mpris_control.xml | ||
---|---|---|
162 ↗ | (On Diff #30402) | Does this work if the system volume plugin is disabled? |
res/values/strings.xml | ||
239 ↗ | (On Diff #30402) | I'd drop the colon. |
src/org/kde/kdeconnect/Plugins/MprisPlugin/MprisActivity.java | ||
95 ↗ | (On Diff #30402) | Should use a check if it's available. |
src/org/kde/kdeconnect/Plugins/SystemvolumePlugin/Sink.java | ||
90 ↗ | (On Diff #30402) | No setMaxVolume? |
src/org/kde/kdeconnect/Plugins/SystemvolumePlugin/SystemvolumeFragment.java | ||
79 ↗ | (On Diff #30402) | Check for null device too. |
105 ↗ | (On Diff #30402) | What about old sinks? Now you're subscribing twice. |
135 ↗ | (On Diff #30402) | Shouldn't it display 0 volume if it's muted? |
src/org/kde/kdeconnect/Plugins/SystemvolumePlugin/SystemvolumePlugin.java | ||
39 ↗ | (On Diff #30402) | Did you split this into remote/local requests? Because we can't control the Android volume yet, and also cannot control from the desktop. |
68 ↗ | (On Diff #30402) | Leftover Logs? |
74 ↗ | (On Diff #30402) | You can use a for in loop. |
88 ↗ | (On Diff #30402) | Should have a check on the sink name. |
90 ↗ | (On Diff #30402) | Here too. |
- Merge branch 'master' into arcpatch-D7993_2
- Do things
res/layout/mpris_control.xml | ||
---|---|---|
162 ↗ | (On Diff #30402) | Yes, as the class still exists. The Activity handles a not loaded systemvolumeplugin gracefully |
res/values/strings.xml | ||
239 ↗ | (On Diff #30402) | I don't need it at all |
src/org/kde/kdeconnect/Plugins/MprisPlugin/MprisActivity.java | ||
95 ↗ | (On Diff #30402) | I do that in the fragment |
src/org/kde/kdeconnect/Plugins/SystemvolumePlugin/Sink.java | ||
90 ↗ | (On Diff #30402) | Maybe someday |
src/org/kde/kdeconnect/Plugins/SystemvolumePlugin/SystemvolumeFragment.java | ||
105 ↗ | (On Diff #30402) | addListener checks for that |
135 ↗ | (On Diff #30402) | No, muting is orthogonal to the actual volume. Plasma-pa behaves the same |
src/org/kde/kdeconnect/Plugins/SystemvolumePlugin/SystemvolumePlugin.java | ||
74 ↗ | (On Diff #30402) | JSONArray doesn't really support that |
src/org/kde/kdeconnect/Plugins/MprisPlugin/MprisActivity.java | ||
---|---|---|
95 ↗ | (On Diff #30402) | Yeah, but now the fragment is added even if it won't do anything. |
src/org/kde/kdeconnect/Plugins/SystemvolumePlugin/SystemvolumePlugin.java | ||
88 ↗ | (On Diff #30402) | This is not done, right? The sink might not actually exist here. |
I get a delay before the UI stuff appears, probably because it's still waiting for the information to display. Would it be possible to load this in advance, like is done in the MPRIS plugin?
res/layout-land/activity_mpris.xml | ||
---|---|---|
26 ↗ | (On Diff #32929) | Why is this here if it's not shown? |
res/layout-land/activity_mpris.xml | ||
---|---|---|
26 ↗ | (On Diff #32929) | Because the plugin assumes that it is there and will crash otherwise as soon as my phone switches to landscape it will crash. It's not excactly a clean solution, I'll try to come up with something better |
res/layout-land/activity_mpris.xml | ||
---|---|---|
26 ↗ | (On Diff #32929) | Android restarts the activity after rotating to landscape (https://developer.android.com/guide/topics/resources/runtime-changes). So you can just check (while creating the activity) if the fragment exists and don't do anything with it if necessary. |
I suggest a small change, but it looks good. It's fine to merge this now and we can make the change later if you want.
src/org/kde/kdeconnect/Plugins/SystemvolumePlugin/SystemvolumePlugin.java | ||
---|---|---|
38 ↗ | (On Diff #30402) | Since the packets you send and the packets you receive are not symmetrical (you receive packets that have a 'sinkList' but you send packets that have, for example a 'name' and 'volume', and they are never interchangeable), I think we should split the type in two (like it is done in other asymmetrical plugins). |