Android: add raw key event support for terminal compatibility#272
Conversation
|
@SashankBhamidi: Nice, thanks! |
|
Lmk if changes needed, will ship as required! |
bk138
left a comment
There was a problem hiding this comment.
pretty neat, some minor comments! thanks for the contribution! ❤️
android/app/src/main/java/com/coboltforge/dontmind/multivnc/AndroidKeyToXt.kt
Outdated
Show resolved
Hide resolved
android/app/src/main/java/com/coboltforge/dontmind/multivnc/AndroidKeyToXt.kt
Outdated
Show resolved
Hide resolved
android/app/src/main/java/com/coboltforge/dontmind/multivnc/AndroidKeyToXt.kt
Show resolved
Hide resolved
android/app/src/main/java/com/coboltforge/dontmind/multivnc/VNCConn.java
Outdated
Show resolved
Hide resolved
android/app/src/main/java/com/coboltforge/dontmind/multivnc/VNCConn.java
Show resolved
Hide resolved
android/app/src/main/java/com/coboltforge/dontmind/multivnc/VNCConn.java
Show resolved
Hide resolved
android/app/src/main/java/com/coboltforge/dontmind/multivnc/db/ConnectionBean.kt
Show resolved
Hide resolved
android/app/src/test/java/com/coboltforge/dontmind/multivnc/AndroidKeyToXtTest.java
Outdated
Show resolved
Hide resolved
- Add XT scan code specification link to documentation - Move hasXtMapping() helper method from production code to test file - Extend existing OutputEvent constructor instead of adding second one - Bump database version to 16 with migration for sendRawKeys column - Add translations for all supported languages (ca, de, es, gl, it, ja, pt, ru, uk, zh, zh-rTW) Addresses review comments from PR bk138#272.
|
@bk138 Addressed 5/8 suggestions. Implemented: XT spec link, moved hasXtMapping() to tests, extended constructor, database version bump, and added translations. Kept unit tests for validation and current int pattern for consistency. |
|
Quick note on translations: These were AI-generated per your suggestion. I don't speak all these languages, I do not guarantee their accuracy. |
android/app/src/main/java/com/coboltforge/dontmind/multivnc/AndroidKeyToXt.kt
Outdated
Show resolved
Hide resolved
android/app/src/main/java/com/coboltforge/dontmind/multivnc/VNCConn.java
Show resolved
Hide resolved
android/app/src/main/java/com/coboltforge/dontmind/multivnc/VNCConn.java
Show resolved
Hide resolved
android/app/src/main/java/com/coboltforge/dontmind/multivnc/db/VncDatabase.java
Outdated
Show resolved
Hide resolved
- Replace int xtKeycode with OptionalInt for cleaner code semantics - Remove AndroidKeyToXtTest.java and related test dependencies - Update all xtKeycode usage to use OptionalInt.isPresent()/getAsInt() - Compilation verified successful Addresses final review feedback from PR bk138#272.
|
@bk138 Changes complete! OptionalInt implemented and tests removed as requested. Ready for review, thanks for the feedback! |
android/app/src/main/java/com/coboltforge/dontmind/multivnc/VNCConn.java
Outdated
Show resolved
Hide resolved
android/app/src/main/java/com/coboltforge/dontmind/multivnc/db/VncDatabase.java
Outdated
Show resolved
Hide resolved
|
@SashankBhamidi almost there! Thanks for your patience! |
- Change database migration to use BOOLEAN instead of INTEGER for sendRawKeys - Remove useExtended field, use only OptionalInt.isPresent() for cleaner design - Add authoritative specification links: RFC 6143 for RFB protocol and vncdotool for QEMU Extended Key Event protocol - Simplify OutputEvent constructor by removing useExtended parameter All changes compile successfully. Addresses final review comments from PR bk138#272.
Since merging this PR will automatically close the issue, I think it’s better to reward the PR on IssueHunt before merging. |
|
@bk138, I thought you were going to merge this PR along with your commits. Closing instead of merging affects my GitHub stats. |
|
@SashankBhamidi Sorry. I don't know how to put commits into your branch/PR other than doing that locally. Do you have docs/info on that for next time? |
For future reference, you can push to contributor branches: git remote add contributor https://github.com/SashankBhamidi/multivnc.git
git push contributor master:feature/android-raw-keys-214Then merge the updated PR normally. |
|
@bk138 Could you reopen and merge PR #272? I've force pushed an empty commit so this PR can get merged. "Merged" vs "Closed" status is important for my GitHub contribution stats since I use them for freelancing gigs. Note: Merging this empty PR won't modify any files since all the actual changes are already in master - this is just to fix the PR status from "Closed" to "Merged". |
|
@SashankBhamidi get your point. There are merge conflicts now, maybe add a single new commit improving some indentation or the like. |
d808706 to
6165281
Compare
|
@bk138, my bad, forgot to reset with upstream before pushing an empty commit. Ready for merge. |
|
Thanks! Will get to issue #216 shortly. |
|
@SashankBhamidi: Good job, thanks! |
|
@bk138 mind if I ask you about the steps you used to reward me? I’m running into an issue with another bounty where my PR isn’t being rewarded, and I’m not too familiar with the process. If you could walk me through it quickly, I’d really appreciate it! Reference: OpenRefine/OpenRefine#7432 (comment) |
The issuehunt page shows funded issues per project, once there is a PR a "reward" button appears, this is how i did it for this issue. It can be that projects need to be added to issuehunt by the maintainer manually; there is an "Add Repository" button in the upper right. |
Summary
Implements QEMU Extended Key Event protocol support for Android client to address raw key event handling issues, particularly for terminal applications like Termux.
Test Results
Technical Details
Uses existing LibVNCClient
SendExtendedKeyEventAPI (available since 2020). No submodule changes required. Maintains backward compatibility with default setting disabled.Addresses issue #214.
IssueHunt Summary
Referenced issues
This pull request has been submitted to: