D7959: Make it possible to add commands for the RunCommandsPlugin from Android

[Matthijs Tijink]

mtijink added a comment.


  Works good, but I still have some comments.

INLINE COMMENTS

> addcommanddialog.xml:17
> +
> +    <EditText
> +        android:id="@+id/addcommand_name"

I'd add some margin to these fields. The android docs use `4dp`.

> menu_runcommand.xml:1
> +<menu xmlns:android="http://schemas.android.com/apk/res/android"
> +      xmlns:app="http://schemas.android.com/apk/res-auto">

Can we use a Floating-Action-Button instead of a menu button? Or did you specifically choose the menu button?

> menu_runcommand.xml:5
> +        android:id="@+id/action_addcommand"
> +        android:icon="@drawable/ic_add_white_48dp"
> +        app:theme="@style/ThemeOverlay.AppCompat.Dark.ActionBar"

Can you use the vector asset instead?

> AddCommandDialog.java:39
> +
> +        builder.setPositiveButton(R.string.ok, new DialogInterface.OnClickListener() {
> +            public void onClick(DialogInterface dialog, int id) {

Would it be possible to disable the `ok` button while either input field is still empty?

> AddCommandDialog.java:53
> +            public void onClick(DialogInterface dialog, int id) {
> +                if(getActivity() instanceof RunCommandActivity){
> +

Why is this empty `if` here?

> RunCommandActivity.java:52
>      private String deviceId;
> +    private final RunCommandPlugin.CommandsChangedCallback theCallback = new RunCommandPlugin.CommandsChangedCallback() {
> +        @Override

`theCallback` is a weird name, I'd suggest `addCommandDialogCallback` or similar.

> RunCommandPlugin.java:141
>      public boolean hasMainActivity() {
> -        return !commandList.isEmpty();
> +        return true;
>      }

So if the desktop does not support remotely adding commands, the (empty) activity still appears, without anything to do. Can we hide it in that case, or add a text displaying how to add commands on the desktop?

REPOSITORY
  R225 KDE Connect - Android application

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

To: nicolasfella, #kde_connect
Cc: mtijink, #kde_connect, SemperPeritus, ahmedbesbes, daniel.z.tg, jeanv, tfella, aboudhar, seebauer, bugzy, progwolff, MayeulC, menasshock, ach, apol
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kdeconnect/attachments/20180114/464e7062/attachment-0001.html>