Share RemoteKeyboard implementation
ClosedPublic

Authored by nicolasfella on May 2 2018, 7:41 PM.

Details

Summary

Extract RemoteKeyboard.qml into declarativeplugin to be able to use it from both plasmoid and app without code duplication

Diff Detail

Repository
R224 KDE Connect
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
nicolasfella created this revision.May 2 2018, 7:41 PM
Restricted Application added a project: KDE Connect. · View Herald TranscriptMay 2 2018, 7:41 PM
nicolasfella requested review of this revision.May 2 2018, 7:41 PM
apol added a subscriber: apol.May 2 2018, 11:40 PM
apol added inline comments.
plasmoid/package/contents/ui/DeviceDelegate.qml
39

Please include the original feature developer to review.

plasmoid/package/contents/ui/RemoteKeyboard.qml
61

Needs destroying the previous remoteKeyboard?

By removing this code you change behaviour: The initial idea was to not make the TextField echo immediately what has been typed, but visualize on desktop what the remote device acked via a network package. This allows for a kind of realtime confirmation of what the remote device *really* processed. Therefore the original event was not accepted and injected artificially onAckPackageReceived. This has some shortcomings, as the echoing in the TextField is incomplete with respect to the handled special keys etc.

A more complete discussion can be found in the original RR: https://git.reviewboard.kde.org/r/129727/

If one wants to keep this behaviour, IMHO the optimal solution would be to inject a real QKeyEvent to the TextField on reception of the ack-package. I played around with a thin native wrapper around QCoreApplication::postEvent() but did not succeed.

It is a design decision whether you want to stick to this echo-behaviour or simply echo immediately what was typed (what you propose here). Personally I would not have a problem with changing it, as in my everyday usage I noted that I always look on my phone when typing remotely via remotekeyboard.

By removing this code you change behaviour: The initial idea was to not make the TextField echo immediately what has been typed, but visualize on desktop what the remote device acked via a network package. This allows for a kind of realtime confirmation of what the remote device *really* processed. Therefore the original event was not accepted and injected artificially onAckPackageReceived. This has some shortcomings, as the echoing in the TextField is incomplete with respect to the handled special keys etc.

A more complete discussion can be found in the original RR: https://git.reviewboard.kde.org/r/129727/

If one wants to keep this behaviour, IMHO the optimal solution would be to inject a real QKeyEvent to the TextField on reception of the ack-package. I played around with a thin native wrapper around QCoreApplication::postEvent() but did not succeed.

It is a design decision whether you want to stick to this echo-behaviour or simply echo immediately what was typed (what you propose here). Personally I would not have a problem with changing it, as in my everyday usage I noted that I always look on my phone when typing remotely via remotekeyboard.

I think that's a good idea to keep, especially if we want to support moving the cursor on the desktop in response to the same on the client (in the future).

apol added a comment.Jul 26 2018, 5:38 PM

What's the status of this one?

Restricted Application added a subscriber: kdeconnect. · View Herald TranscriptJul 26 2018, 5:38 PM
  • Make RemoteKeyboard.qml self-contained
nicolasfella retitled this revision from More RemoteKeyboard cleanup to Make RemoteKeyboard.qml self-contained.Nov 7 2018, 9:42 PM
nicolasfella edited the summary of this revision. (Show Details)
nicolasfella edited the test plan for this revision. (Show Details)
nicolasfella planned changes to this revision.Nov 7 2018, 10:36 PM
apol accepted this revision.Nov 7 2018, 11:29 PM

I'd change the title to "Share RemoteKeyboard implementation". This is not what self-contained means ^^'.

nicolasfella retitled this revision from Make RemoteKeyboard.qml self-contained to Share RemoteKeyboard implementation.Nov 8 2018, 1:01 AM
This revision was not accepted when it landed; it landed in state Changes Planned.Nov 8 2018, 1:20 AM
This revision was automatically updated to reflect the committed changes.