<table><tr><td style="">nicolasfella requested changes to this revision.<br />nicolasfella added a comment.<br />This revision now requires changes to proceed.
</td><a style="text-decoration: none; padding: 4px 8px; margin: 0 8px 8px; float: right; color: #464C5C; font-weight: bold; border-radius: 3px; background-color: #F7F7F9; background-image: linear-gradient(to bottom,#fff,#f1f0f1); display: inline-block; border: 1px solid rgba(71,87,120,.2);" href="https://phabricator.kde.org/D12507">View Revision</a></tr></table><br /><div><div><p>Working pretty good so far.<br />
The command list does not seem to get updated when switching the selected device.<br />
Some design nitpicks as inline comments.<br />
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.</p></div></div><br /><div><strong>INLINE COMMENTS</strong><div><div style="margin: 6px 0 12px 0;"><div style="border: 1px solid #C7CCD9; border-radius: 3px;"><div style="padding: 0; background: #F7F7F7; border-color: #e3e4e8; border-style: solid; border-width: 0 0 1px 0; margin: 0;"><div style="color: #74777d; background: #eff2f4; padding: 6px 8px; overflow: hidden;"><a style="float: right; text-decoration: none;" href="https://phabricator.kde.org/D12507#inline-65799">View Inline</a><span style="color: #4b4d51; font-weight: bold;">widget_remotecommandplugin.xml:4</span></div>
<div style="font: 11px/15px "Menlo", "Consolas", "Monaco", monospace; white-space: pre-wrap; clear: both; padding: 4px 0; margin: 0;"><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span style="color: #354bb3">android:orientation=</span><span style="color: #766510">"vertical"</span> <span style="color: #354bb3">android:layout_width=</span><span style="color: #766510">"match_parent"</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span style="color: #354bb3">android:background=</span><span style="color: #766510">"#bbFFFFFF"</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span style="color: #354bb3">android:layout_height=</span><span style="color: #766510">"match_parent"</span><span style="color: #00702a">></span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">The transparent background is weird IMHO, it makes the command text hardly readable</p></div></div><br /><div style="border: 1px solid #C7CCD9; border-radius: 3px;"><div style="padding: 0; background: #F7F7F7; border-color: #e3e4e8; border-style: solid; border-width: 0 0 1px 0; margin: 0;"><div style="color: #74777d; background: #eff2f4; padding: 6px 8px; overflow: hidden;"><a style="float: right; text-decoration: none;" href="https://phabricator.kde.org/D12507#inline-65803">View Inline</a><span style="color: #4b4d51; font-weight: bold;">widget_remotecommandplugin.xml:13</span></div>
<div style="font: 11px/15px "Menlo", "Consolas", "Monaco", monospace; white-space: pre-wrap; clear: both; padding: 4px 0; margin: 0;"><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span style="color: #00702a"><ImageView</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span style="color: #354bb3">android:layout_width=</span><span style="color: #766510">"wrap_content"</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">I would like a little bit of padding below and above</p></div></div><br /><div style="border: 1px solid #C7CCD9; border-radius: 3px;"><div style="padding: 0; background: #F7F7F7; border-color: #e3e4e8; border-style: solid; border-width: 0 0 1px 0; margin: 0;"><div style="color: #74777d; background: #eff2f4; padding: 6px 8px; overflow: hidden;"><a style="float: right; text-decoration: none;" href="https://phabricator.kde.org/D12507#inline-65804">View Inline</a><span style="color: #4b4d51; font-weight: bold;">widget_remotecommandplugin.xml:35</span></div>
<div style="font: 11px/15px "Menlo", "Consolas", "Monaco", monospace; white-space: pre-wrap; clear: both; padding: 4px 0; margin: 0;"><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span style="color: #354bb3">android:orientation=</span><span style="color: #766510">"vertical"</span> <span style="color: #00702a">/></span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span style="color: #00702a"><TextView</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span style="color: #354bb3">android:id=</span><span style="color: #766510">"@+id/not_reachable_message"</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">Some padding to the left to make it symmetrical</p></div></div><br /><div style="border: 1px solid #C7CCD9; border-radius: 3px;"><div style="padding: 0; background: #F7F7F7; border-color: #e3e4e8; border-style: solid; border-width: 0 0 1px 0; margin: 0;"><div style="color: #74777d; background: #eff2f4; padding: 6px 8px; overflow: hidden;"><a style="float: right; text-decoration: none;" href="https://phabricator.kde.org/D12507#inline-65805">View Inline</a><span style="color: #4b4d51; font-weight: bold;">RunCommandPlugin.java:48</span></div>
<div style="font: 11px/15px "Menlo", "Consolas", "Monaco", monospace; white-space: pre-wrap; clear: both; padding: 4px 0; margin: 0;"><div style="padding: 0 8px; margin: 0 4px; "> <span style="color: #aa4000">private</span> <span class="n">ArrayList</span><span style="color: #aa2211"><</span><span class="n">CommandsChangedCallback</span><span style="color: #aa2211">></span> <span class="n">callbacks</span> <span style="color: #aa2211">=</span> <span style="color: #aa4000">new</span> <span class="n">ArrayList</span><span style="color: #aa2211"><>();</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span style="color: #aa4000">private</span> <span style="color: #aa4000">final</span> <span class="n">ArrayList</span><span style="color: #aa2211"><</span> <span class="n">CommandEntry</span> <span style="color: #aa2211">></span> <span class="n">commandItems</span> <span style="color: #aa2211">=</span> <span style="color: #aa4000">new</span> <span class="n">ArrayList</span><span style="color: #aa2211"><>();</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">Coding style, no spaces inside <>.<br />
Apply everywhere</p></div></div><br /><div style="border: 1px solid #C7CCD9; border-radius: 3px;"><div style="padding: 0; background: #F7F7F7; border-color: #e3e4e8; border-style: solid; border-width: 0 0 1px 0; margin: 0;"><div style="color: #74777d; background: #eff2f4; padding: 6px 8px; overflow: hidden;"><a style="float: right; text-decoration: none;" href="https://phabricator.kde.org/D12507#inline-65813">View Inline</a><span style="color: #4b4d51; font-weight: bold;">RunCommandPlugin.java:122</span></div>
<div style="font: 11px/15px "Menlo", "Consolas", "Monaco", monospace; white-space: pre-wrap; clear: both; padding: 4px 0; margin: 0;"><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span class="n">Collections</span><span style="color: #aa2211">.</span><span style="color: #354bb3">sort</span><span style="color: #aa2211">(</span> <span class="n">commandItems</span><span style="color: #aa2211">,</span> <span style="color: #aa4000">new</span> <span class="n">Comparator</span><span style="color: #aa2211"><</span><span class="n">CommandEntry</span><span style="color: #aa2211">>(){</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span style="color: #aa22ff">@Override</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">Lambda</p></div></div><br /><div style="border: 1px solid #C7CCD9; border-radius: 3px;"><div style="padding: 0; background: #F7F7F7; border-color: #e3e4e8; border-style: solid; border-width: 0 0 1px 0; margin: 0;"><div style="color: #74777d; background: #eff2f4; padding: 6px 8px; overflow: hidden;"><a style="float: right; text-decoration: none;" href="https://phabricator.kde.org/D12507#inline-65812">View Inline</a><span style="color: #4b4d51; font-weight: bold;">RunCommandWidget.java:41</span></div>
<div style="font: 11px/15px "Menlo", "Consolas", "Monaco", monospace; white-space: pre-wrap; clear: both; padding: 4px 0; margin: 0;"><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span class="n">BackgroundService</span><span style="color: #aa2211">.</span><span style="color: #354bb3">RunCommand</span><span style="color: #aa2211">(</span> <span class="n">context</span><span style="color: #aa2211">,</span> <span style="color: #aa4000">new</span> <span class="n">BackgroundService</span><span style="color: #aa2211">.</span><span style="color: #354bb3">InstanceCallback</span><span style="color: #aa2211">(</span> <span style="color: #aa2211">){</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span style="color: #aa22ff">@Override</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">Lambda</p></div></div><br /><div style="border: 1px solid #C7CCD9; border-radius: 3px;"><div style="padding: 0; background: #F7F7F7; border-color: #e3e4e8; border-style: solid; border-width: 0 0 1px 0; margin: 0;"><div style="color: #74777d; background: #eff2f4; padding: 6px 8px; overflow: hidden;"><a style="float: right; text-decoration: none;" href="https://phabricator.kde.org/D12507#inline-65811">View Inline</a><span style="color: #4b4d51; font-weight: bold;">RunCommandWidget.java:84</span></div>
<div style="font: 11px/15px "Menlo", "Consolas", "Monaco", monospace; white-space: pre-wrap; clear: both; padding: 4px 0; margin: 0;"><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span class="n">BackgroundService</span><span style="color: #aa2211">.</span><span style="color: #354bb3">RunCommand</span><span style="color: #aa2211">(</span> <span class="n">context</span><span style="color: #aa2211">,</span> <span style="color: #aa4000">new</span> <span class="n">BackgroundService</span><span style="color: #aa2211">.</span><span style="color: #354bb3">InstanceCallback</span><span style="color: #aa2211">(</span> <span style="color: #aa2211">){</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span style="color: #aa22ff">@Override</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">Lambda</p></div></div><br /><div style="border: 1px solid #C7CCD9; border-radius: 3px;"><div style="padding: 0; background: #F7F7F7; border-color: #e3e4e8; border-style: solid; border-width: 0 0 1px 0; margin: 0;"><div style="color: #74777d; background: #eff2f4; padding: 6px 8px; overflow: hidden;"><a style="float: right; text-decoration: none;" href="https://phabricator.kde.org/D12507#inline-65802">View Inline</a><span style="color: #4b4d51; font-weight: bold;">RunCommandWidget.java:121</span></div>
<div style="font: 11px/15px "Menlo", "Consolas", "Monaco", monospace; white-space: pre-wrap; clear: both; padding: 4px 0; margin: 0;"><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span style="color: #aa4000">else</span><span style="color: #aa2211">{</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span class="n">views</span><span style="color: #aa2211">.</span><span style="color: #354bb3">setTextViewText</span><span style="color: #aa2211">(</span> <span class="n">R</span><span style="color: #aa2211">.</span><span style="color: #354bb3">id</span><span style="color: #aa2211">.</span><span style="color: #354bb3">runcommandWidgetTitle</span><span style="color: #aa2211">,</span> <span class="n">DeviceHelper</span><span style="color: #aa2211">.</span><span style="color: #354bb3">getDeviceName</span><span style="color: #aa2211">(</span> <span class="n">context</span> <span style="color: #aa2211">)</span> <span style="color: #aa2211">+</span> <span style="color: #766510">"@"</span> <span style="color: #aa2211">+</span> <span class="n">currentDevice</span><span style="color: #aa2211">.</span><span style="color: #354bb3">getName</span><span style="color: #aa2211">(</span> <span style="color: #aa2211">)</span> <span style="color: #aa2211">);</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span class="n">views</span><span style="color: #aa2211">.</span><span style="color: #354bb3">setViewVisibility</span><span style="color: #aa2211">(</span> <span class="n">R</span><span style="color: #aa2211">.</span><span style="color: #354bb3">id</span><span style="color: #aa2211">.</span><span style="color: #354bb3">runcommandslist</span><span style="color: #aa2211">,</span> <span class="n">View</span><span style="color: #aa2211">.</span><span style="color: #354bb3">VISIBLE</span> <span style="color: #aa2211">);</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">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</p></div></div><br /><div style="border: 1px solid #C7CCD9; border-radius: 3px;"><div style="padding: 0; background: #F7F7F7; border-color: #e3e4e8; border-style: solid; border-width: 0 0 1px 0; margin: 0;"><div style="color: #74777d; background: #eff2f4; padding: 6px 8px; overflow: hidden;"><a style="float: right; text-decoration: none;" href="https://phabricator.kde.org/D12507#inline-65810">View Inline</a><span style="color: #4b4d51; font-weight: bold;">RunCommandWidget.java:150</span></div>
<div style="font: 11px/15px "Menlo", "Consolas", "Monaco", monospace; white-space: pre-wrap; clear: both; padding: 4px 0; margin: 0;"><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span style="color: #aa4000">public</span> <span style="color: #aa4000">static</span> <span style="color: #aa4000">void</span> <span style="color: #004012">setCurrentDevice</span><span style="color: #aa2211">(</span> <span style="color: #aa4000">final</span> <span class="n">Context</span> <span class="n">context</span><span style="color: #aa2211">,</span> <span style="color: #aa4000">final</span> <span class="n">String</span> <span class="n">DeviceId</span> <span style="color: #aa2211">){</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span class="n">BackgroundService</span><span style="color: #aa2211">.</span><span style="color: #354bb3">RunCommand</span><span style="color: #aa2211">(</span> <span class="n">context</span><span style="color: #aa2211">,</span> <span style="color: #aa4000">new</span> <span class="n">BackgroundService</span><span style="color: #aa2211">.</span><span style="color: #354bb3">InstanceCallback</span><span style="color: #aa2211">(</span> <span style="color: #aa2211">){</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">Lambda</p></div></div><br /><div style="border: 1px solid #C7CCD9; border-radius: 3px;"><div style="padding: 0; background: #F7F7F7; border-color: #e3e4e8; border-style: solid; border-width: 0 0 1px 0; margin: 0;"><div style="color: #74777d; background: #eff2f4; padding: 6px 8px; overflow: hidden;"><a style="float: right; text-decoration: none;" href="https://phabricator.kde.org/D12507#inline-65809">View Inline</a><span style="color: #4b4d51; font-weight: bold;">RunCommandWidgetDeviceSelector.java:30</span></div>
<div style="font: 11px/15px "Menlo", "Consolas", "Monaco", monospace; white-space: pre-wrap; clear: both; padding: 4px 0; margin: 0;"><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span class="n">BackgroundService</span><span style="color: #aa2211">.</span><span style="color: #354bb3">RunCommand</span><span style="color: #aa2211">(</span><span style="color: #aa4000">this</span><span style="color: #aa2211">,</span> <span style="color: #aa4000">new</span> <span class="n">BackgroundService</span><span style="color: #aa2211">.</span><span style="color: #354bb3">InstanceCallback</span><span style="color: #aa2211">()</span> <span style="color: #aa2211">{</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span style="color: #aa22ff">@Override</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">Lambda</p></div></div><br /><div style="border: 1px solid #C7CCD9; border-radius: 3px;"><div style="padding: 0; background: #F7F7F7; border-color: #e3e4e8; border-style: solid; border-width: 0 0 1px 0; margin: 0;"><div style="color: #74777d; background: #eff2f4; padding: 6px 8px; overflow: hidden;"><a style="float: right; text-decoration: none;" href="https://phabricator.kde.org/D12507#inline-65808">View Inline</a><span style="color: #4b4d51; font-weight: bold;">RunCommandWidgetDeviceSelector.java:51</span></div>
<div style="font: 11px/15px "Menlo", "Consolas", "Monaco", monospace; white-space: pre-wrap; clear: both; padding: 4px 0; margin: 0;"><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span class="n">Collections</span><span style="color: #aa2211">.</span><span style="color: #354bb3">sort</span><span style="color: #aa2211">(</span><span class="n">commandItems</span><span style="color: #aa2211">,</span> <span style="color: #aa4000">new</span> <span class="n">Comparator</span><span style="color: #aa2211"><</span><span class="n">ListAdapter</span><span style="color: #aa2211">.</span><span style="color: #354bb3">Item</span><span style="color: #aa2211">>()</span> <span style="color: #aa2211">{</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span style="color: #aa22ff">@Override</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">Lambda?</p></div></div><br /><div style="border: 1px solid #C7CCD9; border-radius: 3px;"><div style="padding: 0; background: #F7F7F7; border-color: #e3e4e8; border-style: solid; border-width: 0 0 1px 0; margin: 0;"><div style="color: #74777d; background: #eff2f4; padding: 6px 8px; overflow: hidden;"><a style="float: right; text-decoration: none;" href="https://phabricator.kde.org/D12507#inline-65807">View Inline</a><span style="color: #4b4d51; font-weight: bold;">RunCommandWidgetDeviceSelector.java:63</span></div>
<div style="font: 11px/15px "Menlo", "Consolas", "Monaco", monospace; white-space: pre-wrap; clear: both; padding: 4px 0; margin: 0;"><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span class="n">view</span><span style="color: #aa2211">.</span><span style="color: #354bb3">setAdapter</span><span style="color: #aa2211">(</span><span class="n">adapter</span><span style="color: #aa2211">);</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span class="n">view</span><span style="color: #aa2211">.</span><span style="color: #354bb3">setOnItemClickListener</span><span style="color: #aa2211">(</span><span style="color: #aa4000">new</span> <span class="n">AdapterView</span><span style="color: #aa2211">.</span><span style="color: #354bb3">OnItemClickListener</span><span style="color: #aa2211">()</span> <span style="color: #aa2211">{</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span style="color: #aa22ff">@Override</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">You can use lambda here</p></div></div></div></div></div><br /><div><strong>REVISION DETAIL</strong><div><a href="https://phabricator.kde.org/D12507">https://phabricator.kde.org/D12507</a></div></div><br /><div><strong>To: </strong>menasshock, albertvaka, KDE Connect, nicolasfella<br /><strong>Cc: </strong>kdeconnect, nicolasfella, ahmedbesbes, daniel.z.tg, jeanv, MayeulC, menasshock, apol<br /></div>