-
Notifications
You must be signed in to change notification settings - Fork 157
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
feat(ui): enable multiple keyboard layouts for "paste text" to remote host #405
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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. |
762cc65
to
70f7ce5
Compare
@ym I received a notification of a Could you try again? I have removed the Since you seem to be squashing the feature branches when merging, I think that Am I expected to resolve merge conflicts to |
@IDisposable do you happen to have advice on how to deal with merge conflicts with What is the maintainers' preferred approach:
Many thanks in advance! |
I'll pull yours and resolve the issues and push back as a PR to your repo... probably not until Friday |
70f7ce5
to
16b62ad
Compare
16b62ad
to
d545686
Compare
@IDisposable I rebased from dev and fixed the merge conflicts coming from #449. |
29241ec
to
7240aba
Compare
It's added 9698564 |
@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:
FYI https://www.atlassian.com/git/tutorials/merging-vs-rebasing |
@IDisposable for illustration, this is the commit graph of my feature branch: ![]() 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: ![]() 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. |
Would dk-latin1 keyboard be possible, please :) |
Just to reconfirm, it would be this Danish keyboard layout? https://www.kbdlayout.info/kbdda |
Excatly :) |
@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 |
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 |
There was a problem hiding this 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!
@adamshiervani thank you for the review. Anything blocking that is still open for me to address? |
|
YAY! Nice to see this merged :) |
Yeah we did it! Nice one, thanks for the support. |
Hallo Daniel, das funktioniert super mit deiner Erweiterung, das rettet alles :-) |
Thx for the fix!!! @dlorch |
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). |
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:
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.