A simple widget to execute commands from android desktop
Details
Details
- Reviewers
albertvaka nicolasfella - Group Reviewers
KDE Connect - Commits
- R225:e0fde1652522: Added runcommandplugin widget
Add the widget and execute commands from it.
By default, the selected device is the first device in the device list.
The selected device can be changed via the widget title.
If there is no commands, the widget show an empty list
If there is no connected device, show the message located in @string/unreachable_description
Diff Detail
Diff Detail
- Repository
- R225 KDE Connect - Android application
- Lint
Automatic diff as part of commit; lint not applicable. - Unit
Automatic diff as part of commit; unit tests not applicable.
Comment Actions
Thanks for the patch!
Sorry for keeping you hanging so long, but reviews are quite slow in general atm. I will test and give you some feedback next week
Comment Actions
Working pretty good so far.
The command list does not seem to get updated when switching the selected device.
Some design nitpicks as inline comments.
Please respect the coding style of the rest of the project, i.e. no spaces inside braces. We are using the standard settings of Android Studio, so you can use the builtin formatter.
| res/layout/widget_remotecommandplugin.xml | ||
|---|---|---|
| 5 | The transparent background is weird IMHO, it makes the command text hardly readable | |
| 14 | I would like a little bit of padding below and above | |
| 36 | Some padding to the left to make it symmetrical | |
| src/org/kde/kdeconnect/Plugins/RunCommandPlugin/RunCommandPlugin.java | ||
| 48 | Coding style, no spaces inside <>. | |
| 122 | Lambda | |
| src/org/kde/kdeconnect/Plugins/RunCommandPlugin/RunCommandWidget.java | ||
| 42 | Lambda | |
| 85 | Lambda | |
| 122 | I don't think it's necessary to include the phones name here. In my case it results in "OnePlus@nico@neon", two time @ is too much | |
| 151 | Lambda | |
| src/org/kde/kdeconnect/Plugins/RunCommandPlugin/RunCommandWidgetDeviceSelector.java | ||
| 31 | Lambda | |
| 52 | Lambda? | |
| 64 | You can use lambda here | |
Comment Actions
Code formatted according to the default Android Studio CodeStyle, and lambda applyed everywhere
Comment Actions
Filtered out devices that are not paired
- ArrayList<ListAdapter.Item> commandItems; renamed to deviceItems
- Coding style
Comment Actions
Another minor issue I found: When the device turns unreachable the command list is still shown instead of the unreachable message. You might be able to use DeviceListChangedCallback from Backgroundservice to work on this
Comment Actions
Looks cool, thanks!
I just have a small improvement suggestion: the device selected by default is the first in the list, even if it's not paired. If there are paired devices, you could select by default the first *paired* device. You can't select a device that is not paired after clicking the device name anyway, so it should not be selected by default.
Another nitpicky thing: maybe the title bar could be smaller? It takes up some real state that could be used for the actual command list.
Apart from these, it looks very good. Let me know if/when you make this changes and want to get this merged, since I see you don't have commit rights.
Comment Actions
I'm having trouble applying your patch. Can you please upload it again using arc? It makes our and your life easier. See https://community.kde.org/Infrastructure/Phabricator#Using_Arcanist_to_post_patches for instructions
Comment Actions
I get
error: patch failed: AndroidManifest.xml:30 error: AndroidManifest.xml: patch does not apply
Comment Actions
I still can't apply the patch using arc, I have to download and apply the diff. When I do so the commit message with the description is lost, so I can't merge this. Can you @nicolasfella push it if arc works for you?
Comment Actions
Arc still does not work for me, but I can download and apply it, I couldn't do that before