D7993: System volume plugin Android

[Matthijs Tijink]

mtijink requested changes to this revision.
mtijink added a comment.
This revision now requires changes to proceed.


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

INLINE COMMENTS

> mpris_control.xml:162
>  
> +    <fragment android:name="org.kde.kdeconnect.Plugins.SystemvolumePlugin.SystemvolumeFragment"
> +        android:id="@+id/systemvolume_fragment"

Does this work if the system volume plugin is disabled?

> strings.xml:239
>  
> +    <string name="systemvolume_label">System volume:</string>
> +    <string name="pref_plugin_systemvolume">System volume</string>

I'd drop the colon.

> MprisActivity.java:95
>  
> +                addSytemvolumeFragment();
> +

Should use a check if it's available.

> Sink.java:90
> +
> +    int getMaxVolume() {
> +        return maxVolume;

No `setMaxVolume`?

> SystemvolumeFragment.java:79
> +                Device device = service.getDevice(deviceId);
> +                plugin = device.getPlugin(SystemvolumePlugin.class);
> +

Check for `null` device too.

> SystemvolumeFragment.java:105
> +        for (Sink sink : plugin.getSinks()) {
> +            sink.addListener(SystemvolumeFragment.this);
> +        }

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

> SystemvolumeFragment.java:135
> +            seekBar.setMax(getItem(position).getMaxVolume());
> +            seekBar.setProgress(getItem(position).getVolume());
> +            seekBar.setOnSeekBarChangeListener(listener);

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

> SystemvolumePlugin.java:39
> +
> +    private final static String PACKET_TYPE_SYSTEMVOLUME = "kdeconnect.systemvolume";
> +

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

> SystemvolumePlugin.java:68
> +            Log.d("SinkList", "received");
> +            Log.d("SinkList", np.getJSONArray("sinkList").toString());
> +            sinks.clear();

Leftover `Log`s?

> SystemvolumePlugin.java:74
> +
> +                for (int i = 0; i < sinkArray.length(); i++) {
> +                    JSONObject sinkObj = sinkArray.getJSONObject(i);

You can use a for in loop.

> SystemvolumePlugin.java:88
> +        } else if (np.has("volume")) {
> +            sinks.get(np.getString("name")).setVolume(np.getInt("volume"));
> +        } else if (np.has("muted")) {

Should have a check on the sink name.

> SystemvolumePlugin.java:90
> +        } else if (np.has("muted")) {
> +            sinks.get(np.getString("name")).setMute(np.getBoolean("muted"));
> +        }

Here too.

REPOSITORY
  R225 KDE Connect - Android application

REVISION DETAIL
  https://phabricator.kde.org/D7993

To: nicolasfella, #kde_connect, mtijink
Cc: Murz, mtijink, #kde_connect, adeen-s, SemperPeritus, ahmedbesbes, daniel.z.tg, jeanv, seebauer, bugzy, MayeulC, menasshock, ach, apol
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kdeconnect/attachments/20180324/f494a8cd/attachment-0001.html>