<html>
<body>
<div style="font-family: Verdana, Arial, Helvetica, Sans-Serif;">
<table bgcolor="#f9f3c9" width="100%" cellpadding="12" style="border: 1px #c9c399 solid; border-radius: 6px; -moz-border-radius: 6px; -webkit-border-radius: 6px;">
<tr>
<td>
This is an automatically generated e-mail. To reply, visit:
<a href="https://git.reviewboard.kde.org/r/129727/">https://git.reviewboard.kde.org/r/129727/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On January 10th, 2017, 9:35 p.m. UTC, <b>Albert Vaca Cintora</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">>For the json-payload I used the syntax of the key-events as sent by mousepad-plugin with the addition of a "sendAck"-flag. If "sendAck" is set to true the remote peer should echo a key-event if it could be handled, thus allowing the local client to find out whether the key was accepted. For performance reasons, it's allowed to send multi-char strings in the "key" property (performs much better if you send a whole string via "echo '...' | kdeconnect-cli ..." e.g.)
I like the fact that you used the same protocol. This actually should allow us to send keys phone-to-phone and kde-to-kde, but as of now it's not working because the package type used phone-to-desktop are called "kdeconnect.mousepad" and the desktop-to-phone are called "kdeconnect.remotekeyboard". We tried changing your code to use "kdeconnect.mousepad" and it worked Android to Android :) so it might be a good idea to rename the package type (even though it only implements the keyboard part and not the cursor part, but it's not a big deal). It's true that you "extended" the protocol by adding the "echos" (maybe they can be renamed to "kdeconnect.mousepad.echos" to make it more clear?) but I don't think it will break if echos are not present (eg: desktop-to-desktop).
>kdeconnect-cli: For now takes a string and transforms it into single key-events for visible characters only. In a first implementation I used a kbhit() helper that used termios.h to catch and relay keypresses interactively (including some special-events), which was not optimal. A better approch might be to use linux input-api directly. Would this be an option regarding cross-platform compatibility or can I assume to develop for Linux only? Being a command-line guy, I'd really like to have a fully featured kdeconnect-cli interface ;-)
I like your approach because it's simple and works multi-platform.
>Factor out the Qt::Key-to-internal keymap to some core-helper because it corresponds to the mapping in the mousepad-plugin?
It sounds like a good idea, we can do it in a separate patch.
>The plasmoid is not perfect as it is: A single line containing a non-echoing TextField (i.e. it eats all the KeyPress events), and only ack-ed keypress-packets from the peer device are injected if they contain visible keys. Advantage: the user sees whether his key-presses are accepted by the peer device. Disadvantage: The echoed text does not correspond 1:1 to what is shown on the peer's display, user might be confused when typing without success. I played around with different variations each of which with its proper shortcomings:
1. An echoing Textfield for typing: Has the advantage that the user can directly see what he is typing, which makes interaction in the typing field easier, BUT messes up interaction if the Editor on the peer is changed silently and does not notify the user if his keypresses are not handled by the peer.
2. A non-echoing TextField for typing PLUS a readonly one for printing visible echoed keys. Disadvantage: same as for the previous one and uses more space on the plasmoid.
Comments? Ideas?
Agreed. I also found it a bit confusing that it is no 1:1. Maybe if we echo a few more things like the Backspaces it will get better?
One extra thought: right now the Plasmoid doesn't know if the keyboard is opened on the Android side or not. If we make the Android side notify the desktop when the keyboard is open or closed we can use that info on the desktop. For example: gain focus of the input field automatically, or hide it when there is no receiver on Android. Any thoughts?</pre>
</blockquote>
</blockquote>
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><blockquote style="text-rendering: inherit;padding: 0 0 0 1em;border-left: 1px solid #bbb;white-space: normal;margin: 0 0 0 0.5em;line-height: inherit;">
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">We tried changing your code to use "kdeconnect.mousepad" and it worked Android to Android :) so it might be a good idea to rename the package type (even though it only implements the keyboard part and not the cursor part, but it's not a big deal).</p>
</blockquote>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Ok, will change it to mousepad!</p>
<blockquote style="text-rendering: inherit;padding: 0 0 0 1em;border-left: 1px solid #bbb;white-space: normal;margin: 0 0 0 0.5em;line-height: inherit;">
<blockquote style="text-rendering: inherit;padding: 0 0 0 1em;border-left: 1px solid #bbb;white-space: normal;margin: 0 0 0 0.5em;line-height: inherit;">
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">The plasmoid is not perfect as it is: A single line containing a non-echoing TextField (i.e. it eats all the KeyPress events), and only ack-ed keypress-packets from the peer device are injected if they contain visible keys. Advantage: the user sees whether his key-presses are accepted by the peer device. Disadvantage: The echoed text does not correspond 1:1 to what is shown on the peer's display, user might be confused when typing without success. I played around with different variations each of which with its proper shortcomings:
1. An echoing Textfield for typing: Has the advantage that the user can directly see what he is typing, which makes interaction in the typing field easier, BUT messes up interaction if the Editor on the peer is changed silently and does not notify the user if his keypresses are not handled by the peer.
2. A non-echoing TextField for typing PLUS a readonly one for printing visible echoed keys. Disadvantage: same as for the previous one and uses more space on the plasmoid.
Comments? Ideas?</p>
</blockquote>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Agreed. I also found it a bit confusing that it is no 1:1. Maybe if we echo a few more things like the Backspaces it will get better?</p>
</blockquote>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Not sure, unless we interpret it completely as is done on android side it will feel incomplete. My first try was to inject a real QKeyEvent upon receiving an echo package into the Textfield, which would result in a (almost) perfect behavoir as the Android keyhandling tries to mimic TextField's handling of special keys on desktop. I tried hard via a native wrapper for QCoreApplication::postEvent() but there was never a event arriving at the QQuickItem. Hints from the Qt-experts?</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Another option would be to simply disable echoing in the plasmoid completely and let the user look at the result on the Android side.</p>
<blockquote style="text-rendering: inherit;padding: 0 0 0 1em;border-left: 1px solid #bbb;white-space: normal;margin: 0 0 0 0.5em;line-height: inherit;">
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">One extra thought: right now the Plasmoid doesn't know if the keyboard is opened on the Android side or not. If we make the Android side notify the desktop when the keyboard is open or closed we can use that info on the desktop. For example: gain focus of the input field automatically, or hide it when there is no receiver on Android. Any thoughts?</p>
</blockquote>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Thought about an extra package for that case, too. For now the echoing in the plasmoid is supposed to give the user feedback about whether his keypresses are handled remotely or not. We could add a kdeconnect.mousepad.keyboard_status package and act as you proposed (gray-out vs. activate the textfield or so). With this feedback keypress echoing could also be disabled in the plasmoid completely because we give other feedback to the user.</p></pre>
<br />
<p>- Holger</p>
<br />
<p>On January 11th, 2017, 4:32 p.m. UTC, Holger Kaelberer wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="12" style="border: 1px #888a85 solid; border-radius: 6px; -moz-border-radius: 6px; -webkit-border-radius: 6px;">
<tr>
<td>
<div>Review request for kdeconnect.</div>
<div>By Holger Kaelberer.</div>
<p style="color: grey;"><i>Updated Jan. 11, 2017, 4:32 p.m.</i></p>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Bugs: </b>
<a href="http://bugs.kde.org/show_bug.cgi?id=370919">370919</a>
</div>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt;">Repository: </b>
kdeconnect-kde
</div>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Description </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: 1px solid #b8b5a0">
<tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Allow to inject keypress events to remote peers (most notably Android devices)</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Notes / open issues / possible improvements:</p>
<ul style="padding: 0;text-rendering: inherit;margin: 0 0 0 1em;line-height: inherit;white-space: normal;">
<li style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">For the json-payload I used the syntax of the key-events as sent by mousepad-plugin with the addition of a "sendAck"-flag. If "sendAck" is set to true the remote peer should echo a key-event if it could be handled, thus allowing the local client to find out whether the key was accepted. For performance reasons, it's allowed to send multi-char strings in the "key" property (performs much better if you send a whole string via "echo '...' | kdeconnect-cli ..." e.g.)</p>
</li>
<li style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">kdeconnect-cli: For now takes a string and transforms it into single key-events for visible characters only. In a first implementation I used a kbhit() helper that used termios.h to catch and relay keypresses interactively (including some special-events), which was not optimal. A better approch might be to use linux input-api directly. Would this be an option regarding cross-platform compatibility or can I assume to develop for Linux only? Being a command-line guy, I'd really like to have a fully featured kdeconnect-cli interface ;-)</p>
</li>
<li style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Factor out the Qt::Key-to-internal keymap to some core-helper because it corresponds to the mapping in the mousepad-plugin?</p>
</li>
<li style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">The plasmoid is not perfect as it is: A single line containing a non-echoing TextField (i.e. it eats all the KeyPress events), and only ack-ed keypress-packets from the peer device are injected if they contain visible keys. Advantage: the user sees whether his key-presses are accepted by the peer device. Disadvantage: The echoed text does not correspond 1:1 to what is shown on the peer's display, user might be confused when typing without success. I played around with different variations each of which with its proper shortcomings:</p>
</li>
<li style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">An echoing Textfield for typing: Has the advantage that the user can directly see what he is typing, which makes interaction in the typing field easier, BUT messes up interaction if the Editor on the peer is changed silently and does not notify the user if his keypresses are not handled by the peer.</li>
<li style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">A non-echoing TextField for typing PLUS a readonly one for printing visible echoed keys. Disadvantage: same as for the previous one and uses more space on the plasmoid.
Comments? Ideas?</li>
</ul></pre>
</td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Testing </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: 1px solid #b8b5a0">
<tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">A lot!</p></pre>
</td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Diffs</b> </h1>
<ul style="margin-left: 3em; padding-left: 0;">
<li>cli/kdeconnect-cli.cpp <span style="color: grey">(1acb723)</span></li>
<li>interfaces/CMakeLists.txt <span style="color: grey">(19a9cdc)</span></li>
<li>interfaces/dbusinterfaces.h <span style="color: grey">(9848d0c)</span></li>
<li>interfaces/dbusinterfaces.cpp <span style="color: grey">(aab3e05)</span></li>
<li>plasmoid/declarativeplugin/kdeconnectdeclarativeplugin.cpp <span style="color: grey">(282f2a9)</span></li>
<li>plasmoid/package/contents/ui/DeviceDelegate.qml <span style="color: grey">(e90e021)</span></li>
<li>plasmoid/package/contents/ui/RemoteKeyboard.qml <span style="color: grey">(PRE-CREATION)</span></li>
<li>plugins/CMakeLists.txt <span style="color: grey">(9d230ac)</span></li>
<li>plugins/mousepad/kdeconnect_mousepad.json <span style="color: grey">(850e855)</span></li>
<li>plugins/remotekeyboard/CMakeLists.txt <span style="color: grey">(PRE-CREATION)</span></li>
<li>plugins/remotekeyboard/README <span style="color: grey">(PRE-CREATION)</span></li>
<li>plugins/remotekeyboard/kdeconnect_remotekeyboard.json <span style="color: grey">(PRE-CREATION)</span></li>
<li>plugins/remotekeyboard/remotekeyboardplugin.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>plugins/remotekeyboard/remotekeyboardplugin.cpp <span style="color: grey">(PRE-CREATION)</span></li>
</ul>
<p><a href="https://git.reviewboard.kde.org/r/129727/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>