Skip to content

feat(ui): enable multiple keyboard layouts for "paste text" to remote host #405

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 34 commits into from
May 21, 2025

Conversation

dlorch
Copy link
Contributor

@dlorch dlorch commented May 2, 2025

Pasting text in the "Paste text" dialog sends individual key strokes to the target device using an USB keyboard emulation. Which numerical key codes are being sent depend on a mapping. This mapping was based on US English until now.

This PR provides the mechanism to enable multiple keyboard layouts for different languages and comes included with keyboard layouts for:

  • Belgian Dutch
  • Czech
  • English (UK)
  • English (US)
  • French
  • German
  • Italian
  • Norwegian
  • Spanish
  • Swedish
  • Swiss French
  • Swiss German

Let me know if you would like to see further keyboard layouts and I see what I can do.

I've seen #116, but the scope of that PR is much bigger and aiming at overhauling the entire keyboard functionality. For this PR here, I wrote the minimal set of changes to enable keyboard layouts in the "Paste text" dialog. We can then later align the initiatives.

This provides fixes for #22, #30, #65, #104, #47.

Could potentially address #436.

paste-modal-spanish

keyboard-layout-config

Copy link
Contributor

@IDisposable IDisposable left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Generally looks awesome!

@dlorch
Copy link
Contributor Author

dlorch commented May 8, 2025

Generally looks awesome!

Thank you for the feedback and for reviewing! My head is still spinning from all the different amazing accent variants that are available in the Czech language.

@IDisposable
Copy link
Contributor

This is almost perfect, the only remaining thing (in my opinion) is to make the name of the locale be IN the language of the locale. I've submitted a PR on your repo @dlorch dlorch#1

@dlorch
Copy link
Contributor Author

dlorch commented May 9, 2025

This is almost perfect, the only remaining thing (in my opinion) is to make the name of the locale be IN the language of the locale. I've submitted a PR on your repo @dlorch dlorch#1

Merged!

@dlorch dlorch changed the title Enable multiple keyboard layouts for "Paste text" from host feat(ui): enable multiple keyboard layouts for "paste text" from host May 13, 2025
@dlorch dlorch changed the title feat(ui): enable multiple keyboard layouts for "paste text" from host feat(ui): enable multiple keyboard layouts for "paste text" to remote host May 13, 2025
@dlorch dlorch force-pushed the paste-text-keyboard-layouts branch from 762cc65 to 70f7ce5 Compare May 14, 2025 23:18
@dlorch
Copy link
Contributor Author

dlorch commented May 14, 2025

@ym I received a notification of a lint error in usb_mass_storage.go: https://github.com/jetkvm/kvm/actions/runs/15002349526 but this file is not part of my changeset.

Could you try again? I have removed the git merge dev from the history and force pushed the changes to this branch.

Since you seem to be squashing the feature branches when merging, I think that git merge dev could have caused some issues.

Am I expected to resolve merge conflicts to dev? If yes, through rebasing with git rebase dev?

@dlorch
Copy link
Contributor Author

dlorch commented May 15, 2025

@IDisposable do you happen to have advice on how to deal with merge conflicts with dev? Since this PR was created, the dev branch has evolved a bit. The merge conflicts are not structural, but just having a change in the same code area than another contributor can cause them.

What is the maintainers' preferred approach:

  1. do I need to git merge dev and resolve the conflicts? I think this is unlikely, as PRs are squashed when merged
  2. do I need to run git rebase dev and resolve the conflicts?
  3. do you fix the (simple) merge conflicts yourself?

Many thanks in advance!

@IDisposable
Copy link
Contributor

I'll pull yours and resolve the issues and push back as a PR to your repo... probably not until Friday

@dlorch dlorch force-pushed the paste-text-keyboard-layouts branch from 70f7ce5 to 16b62ad Compare May 16, 2025 14:25
@dlorch dlorch closed this May 16, 2025
@dlorch dlorch force-pushed the paste-text-keyboard-layouts branch from 16b62ad to d545686 Compare May 16, 2025 14:27
@dlorch dlorch reopened this May 16, 2025
@dlorch
Copy link
Contributor Author

dlorch commented May 16, 2025

@IDisposable I rebased from dev and fixed the merge conflicts coming from #449.

@dlorch dlorch force-pushed the paste-text-keyboard-layouts branch from 29241ec to 7240aba Compare May 19, 2025 23:03
@dlorch
Copy link
Contributor Author

dlorch commented May 19, 2025

Is there an option to add Dutch (Belgium) to the keyboards?

That would be the AZERTY keyboard as used in Flanders? https://kbdlayout.info/kbdbene

Correct! 👍

It's added 9698564

@dlorch
Copy link
Contributor Author

dlorch commented May 19, 2025

@IDisposable I rebased from dev again and fixed the merge conflicts. I would consider this PR ready. Let's ship this thing! The many users in #22, #30, #65, #104, #47 will be thankful. Thank you for the support!

@dlorch
Copy link
Contributor Author

dlorch commented May 20, 2025

@IDisposable could you rebase from my branch and not merge? I‘m seeing merges in your feature branch and that‘s not going to work: https://github.com/IDisposable/kvm/commits/paste-text-keyboard/

I‘ve rebased from the „dev“ branch to keep current and fix merge conflicts. Rebasing rewrites history and you‘re basing off an old branch.

My recommendation:

  1. Get my PR merged in as-is, as it will apply cleanly (but „dev“ is moving fast with changes coming in, so please don‘t hold off. Waiting means more work for me…)
  2. Create separate PRs for your changes based off the new dev branch

FYI https://www.atlassian.com/git/tutorials/merging-vs-rebasing

@dlorch
Copy link
Contributor Author

dlorch commented May 20, 2025

@IDisposable for illustration, this is the commit graph of my feature branch:

Screenshot 2025-05-20 at 11 38 39

What can we see on the picture? My feature branch "branches off" the HEAD of the "dev" branch (b4dd496) in a straight line.

I was able to do this by "rebasing" my changes on top of the latest "dev" branch. This is a technique to rewrite the git history as if I would have created a fresh branch just now.

All operations that rewrite history also change the commit IDs (hashes), and therefore there needs to be a communication and alignment between developers that work on the same branch, otherwise people will continue to work off different histories of the same code. Maybe the message got lost in translation, but I've inquired above on your preferred approach to get the merge conflicts resolved. Apologies if I was not clear on the implications of this choice.

The JetKVM maintainers seem to "squash" feature branches before merging them. "Squashing" is a technique to "flatten" a list of commits into a single one. This helps to keep the Git history compact and is generally a recommended approach, because after merging a PR, the detailed feature branch can be thrown away and only the squashed commit will be kept. But this also means the feature branches should be "clean" and ideally not contain any unrelated changes that would occur from for example merging things.

For comparison, this is the commit graph of your feature branch:

Screenshot 2025-05-20 at 11 36 29

Your feature branch now contains a mix of my changes, your changes, and a few unrelated commits from the latest "dev" branch. That could cause complications when "squashing" the branch.

Also pay close attention to the "SHA" column between our two commit graphs. For the same commits, we have different values, as I have rebased for the second time and you are working off an older (outdated) branch.

Mark, you are a proponent of keeping PRs "low risk", small and self-contained. For all intents an purposes, this PR is ready to be merged for already two weeks. As mentioned before, changes that occur in "dev" that affect the same code areas as this PR (happened two times already) mean there's effort for me to do an OSS contributor to keep this PR up to date.

Unless there are reasons not to do so, let's get this PR merged and fix forward. Any refactorings that are based on top of it, can be cleanly applied as separate and "low risk" PRs. I hope this is a reasonable request.

@erialor
Copy link

erialor commented May 20, 2025

Would dk-latin1 keyboard be possible, please :)

@dlorch
Copy link
Contributor Author

dlorch commented May 20, 2025

Would dk-latin1 keyboard be possible, please :)

Just to reconfirm, it would be this Danish keyboard layout? https://www.kbdlayout.info/kbdda

@erialor
Copy link

erialor commented May 20, 2025

Would dk-latin1 keyboard be possible, please :)

Just to reconfirm, it would be this Danish keyboard layout? https://www.kbdlayout.info/kbdda

Excatly :)

@dlorch
Copy link
Contributor Author

dlorch commented May 21, 2025

@IDisposable again I hope you and your loved ones are doing well.

If I had to summarize my efforts on this PR, it would be about 30% to write the code, which I‘d say is quite a feat. considering the number of keyboard layouts already included (and manually validated), supporting complex accent key handling, and 70% going back and forth on variable naming, shuffling logic around and git branching woes.

The PR proposed by you dlorch#2 still contains unrelated changes merged in from „dev“, duplicates logic in areas, contains debug statements, and there‘s also a shift in the programming style versus what‘s already there.

This PR here is ready to be merged, has clear user value, but instead it is being micro-managed without clear outline on how to get this into „dev“. I‘ve been extremely responsive on getting everything addressed that was raised to me, but it has become a never ending infinite loop of iterations on refactoring stuff.

I‘m doing these OSS contributions in my free time because I like the product and need this feature here to be fixed for myself, I’ve been also nice enough to cover other languages, but I can‘t support the sustained back and forth on this anymore.

At this point, this is a take-it-or-leave-it situation. I hope this reasonable and understandable.

Looking forward to the Google Meet and having a chat!

CC: @adamshiervani @ym

@IDisposable
Copy link
Contributor

I'm fine with whatever the decision is... I can always submit a new one on top of this after it gets merged. I understand you concerns about a different coding style (in that I'm using a distinct type for the keyboard layouts so we get all the attributes in one object instead of having to keep two things in sync). The changes from dev are because I rebased onto your version, which was recently rebased as well, not something else I pulled in (unless unintentionally).

Copy link
Contributor

@adamshiervani adamshiervani left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wonderful work! I tested it on my dev setup, and it works great functionally. Just a couple of tiny change requests.

Let's get this one shipped soon. I know many people will be happy!

@dlorch dlorch requested a review from adamshiervani May 21, 2025 11:50
@dlorch
Copy link
Contributor Author

dlorch commented May 21, 2025

@adamshiervani thank you for the review. Anything blocking that is still open for me to address?

@adamshiervani adamshiervani merged commit b91a995 into jetkvm:dev May 21, 2025
4 of 5 checks passed
@dlorch
Copy link
Contributor Author

dlorch commented May 21, 2025

Would dk-latin1 keyboard be possible, please :)

Just to reconfirm, it would be this Danish keyboard layout? https://www.kbdlayout.info/kbdda

Excatly :)

@erialor please subscribe to notifications on this PR: #494

@IDisposable
Copy link
Contributor

YAY! Nice to see this merged :)

@dlorch
Copy link
Contributor Author

dlorch commented May 21, 2025

YAY! Nice to see this merged :)

Yeah we did it! Nice one, thanks for the support.

@v7o73
Copy link

v7o73 commented May 22, 2025

Hallo Daniel, das funktioniert super mit deiner Erweiterung, das rettet alles :-)
Ein grosses Merci !

@cgilis
Copy link

cgilis commented May 26, 2025

Thx for the fix!!! @dlorch
Only small issue when using combined with Macro's, still in qwerty here.

@IDisposable
Copy link
Contributor

Yep, those two things were built in parallel so we need to implement these fixes for Paste (and Virtual Keyboard needs adjusting once #500 is merged).

We need to further unify the keyboard state management so things can say "I want to emit this keycode" (for macros and virtual keyboard) or "I want to cause this unicode to be entered" (for paste and macros in literal mode).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants