System volume plugin Android
ClosedPublic

Authored by nicolasfella on Sep 26 2017, 11:05 AM.

Details

Summary

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

Diff Detail

Repository
R225 KDE Connect - Android application
Branch
arcpatch-D7993
Lint
No Linters Available
Unit
No Unit Test Coverage
nicolasfella created this revision.Sep 26 2017, 11:05 AM

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.

Looks ok to me (and it works). But why is this implemented as a separate plugin?

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

apol edited the summary of this revision. (Show Details)Jan 16 2018, 10:21 PM
Murz added a subscriber: Murz.Feb 4 2018, 10:37 AM

Extract control slider into a fragment.
Preparation, but still no support for multiple sinks.

Adapt to desktop

nicolasfella retitled this revision from WIP: System volume plugin Android to System volume plugin Android.Feb 5 2018, 6:41 PM
nicolasfella edited the summary of this revision. (Show Details)Feb 5 2018, 6:45 PM

Merge branch 'master' into arcpatch-D7993

  • Merge branch 'master' into arcpatch-D7993_1
mtijink requested changes to this revision.Mar 24 2018, 7:05 PM

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

Does this work if the system volume plugin is disabled?

res/values/strings.xml
241

I'd drop the colon.

src/org/kde/kdeconnect/Plugins/MprisPlugin/MprisActivity.java
96

Should use a check if it's available.

src/org/kde/kdeconnect/Plugins/SystemvolumePlugin/Sink.java
91

No setMaxVolume?

src/org/kde/kdeconnect/Plugins/SystemvolumePlugin/SystemvolumeFragment.java
80

Check for null device too.

106

What about old sinks? Now you're subscribing twice.

136

Shouldn't it display 0 volume if it's muted?

src/org/kde/kdeconnect/Plugins/SystemvolumePlugin/SystemvolumePlugin.java
40

Did you split this into remote/local requests? Because we can't control the Android volume yet, and also cannot control from the desktop.

69

Leftover Logs?

75

You can use a for in loop.

89

Should have a check on the sink name.

91

Here too.

This revision now requires changes to proceed.Mar 24 2018, 7:05 PM
nicolasfella marked 6 inline comments as done.
  • Merge branch 'master' into arcpatch-D7993_2
  • Do things
res/layout/mpris_control.xml
162

Yes, as the class still exists. The Activity handles a not loaded systemvolumeplugin gracefully

res/values/strings.xml
241

I don't need it at all

src/org/kde/kdeconnect/Plugins/MprisPlugin/MprisActivity.java
96

I do that in the fragment

src/org/kde/kdeconnect/Plugins/SystemvolumePlugin/Sink.java
91

Maybe someday

src/org/kde/kdeconnect/Plugins/SystemvolumePlugin/SystemvolumeFragment.java
106

addListener checks for that

136

No, muting is orthogonal to the actual volume. Plasma-pa behaves the same

src/org/kde/kdeconnect/Plugins/SystemvolumePlugin/SystemvolumePlugin.java
75

JSONArray doesn't really support that

mtijink added inline comments.Mar 25 2018, 12:50 PM
src/org/kde/kdeconnect/Plugins/MprisPlugin/MprisActivity.java
96

Yeah, but now the fragment is added even if it won't do anything.

src/org/kde/kdeconnect/Plugins/SystemvolumePlugin/SystemvolumePlugin.java
89

This is not done, right? The sink might not actually exist here.

  • Replace with variable
nicolasfella marked an inline comment as done.Mar 25 2018, 12:54 PM
  • Merge branch 'master' into arcpatch-D7993_2

Hide Fragment in Landscape mode
There is not enough vertical space for it

  • Merge branch 'master' into arcpatch-D7993
  • Tweak mpris layout

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

Why is this here if it's not shown?

nicolasfella added inline comments.Apr 30 2018, 10:16 AM
res/layout-land/activity_mpris.xml
26

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

mtijink added inline comments.Apr 30 2018, 6:43 PM
res/layout-land/activity_mpris.xml
26

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.

  • Merge branch 'master' into arcpatch-D7993
  • Don't crash when switching to landscape
  • New line at end of file
MatMaul added a subscriber: MatMaul.Jun 3 2018, 5:14 PM
Restricted Application added a subscriber: kdeconnect. · View Herald TranscriptJun 3 2018, 5:14 PM
albertvaka accepted this revision.Jul 26 2018, 5:56 PM
albertvaka added a subscriber: albertvaka.

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

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).

This revision was not accepted when it landed; it landed in state Needs Review.Jul 27 2018, 7:20 AM
This revision was automatically updated to reflect the committed changes.