Rewrite in N-API; add Wayland support #4
Conversation
|
I also must remember to chose the “squash and merge” option when landing this… or else do the squash myself locally. Lots of junky commits here. |
|
I'm reacquainting myself with the changes I made to CI. You can see in This is what |
savetheclocktower
requested review from
DeeDeeG,
confused-Techie and
mauricioszabo
|
Much like this PR on another repo, this PR represents what's been shipping in PulsarNext for four months. I'm aware of no regressions on other platforms (macOS/Windows code isn't touched except to convert to N-API) or even on Linux with X11 (the approach we revert to as soon as any of our Wayland-specific assumptions are violated). With this in mind, I might leave this PR open a while longer but treat it as a “speak now or forever hold your peace” PR, lest it stay open and remain a blocker for other items on the PulsarNext dependency checklist. |
DeeDeeG
left a comment
DeeDeeG
left a comment
There was a problem hiding this comment.
I have looked at the entire diff. Parts of it for longer than others.
Have said it elsewhere but I will say it here: I am not a C/C++ expert. And I did not dive very deep into looking over the C/C++ code. Comments below give what impression I could manage to form on that.
I had a couple of minor questions about whether some logging and a test file are still wanted, marked below with ❓ emoji. (Optional to address, and I trust your judgment on these small things -- however you resolve them (without introducing substantial new stuff, I suppose) may be considered to retain this approval.)
Other than that, looks good to me! (Unfortunately another rubber-stamp-ish review due to lack of depth of this review, but at least I've seen all the changes. Better than nothing? Heh.)
There was a problem hiding this comment.
I like that there is documentation, including helpful hints and caveats for users of this stuff. It makes me feel a bit out of depth not knowing the implications and context of some of these things I'm reading, but the comments here still seem relevant and to the point (as far as I can tell). 👍
There was a problem hiding this comment.
I have looked at (hesitate to say "looked over" due to a lot of it going over my head) all the diff in src/. The main things standing out are moving from Nan:: (and occasionally v8::) things over to Napi:: things. And "a lot of new stuff." Which one gets the impression is largely to do with the titular "add Wayland support."
Nothing bad stands out to my (somewhat under-qualified) human eyes unaided by an IDE (I wouldn't know much of where to start with this in an IDE, besides not intending to wade that far into the depths of code review for this one.)
This is my qualified (read that how you will) endorsement of the diff content in src/dir: --> 👍 (FWIW.)
(Aside: The sheer diffstat of new stuff in keyboard-layout-manager-linux.cc is quite impressive, by the way!)
| console.log('KEYMAP:', keymap); | ||
| expect(keymap.KeyG.unmodified).toBeDefined() | ||
| expect(keymap.KeyG.withShift).toBeDefined() | ||
| }) | ||
| }) | ||
|
|
||
| describe('.getCurrentKeyboardLayout()', function () { | ||
| it('returns an identifier for the current keyboard layout (basic smoke test)', function () { | ||
| expect(KeyboardLayout.getCurrentKeyboardLayout()).toBeDefined() | ||
| let layout = KeyboardLayout.getCurrentKeyboardLayout(); | ||
| console.warn('Linux keyboard layout:', layout); |
There was a problem hiding this comment.
❓
Do we still want these console.log() / console.warn() in the spec? Probably harmless. Just something I noticed.
There was a problem hiding this comment.
❓
Do we want this file?
Maybe it's only useful in development? That said, perhaps it'll be useful again for development some time in the future.
(IDK or have a strong opinion about it. Just something I noticed that stood out a bit.)
There was a problem hiding this comment.
Fair point! I don't think I meant to add this to the PR in the first place. If it stays as a utility script, it should be cleaned up a bit. I'll address this.
There was a problem hiding this comment.
Could just add a comment that
// This test file is offered without warranty express or implied
... And/or an explanation of what it's for and what insight it gives, what it's good for debugging, etc... but I found it pretty self-explanatory that it watches for locale switches and logs about it from in a similar enough JS context to likely be relevant to how Pulsar sees such events??
I make my own little test scripts often enough that I recognized what it was pretty much right away. I don't think it necessarily needs changes, personally.
There was a problem hiding this comment.
Using newer JS capabilities and enhancing readability. 👍
There was a problem hiding this comment.
These fixes seem intuitively correct, again to my human eyeballs unaided by language-aware tooling at least. 👍
|
Aside: I am not as concerned with going out of my way to ensure keeping a copy around of all the commit history for this one. GitHub seems to do that on its own, for one thing. For another thing, the diff here is smaller and IMO fairly well-commented. I could see it landing basically in one go and not being confusing. I think. |
2 participants
This brings
masterup to the version ofkeyboard-layoutthat has been used in PulsarNext since May. It delivers two major additions.N-API rewrite
The rewrite in N-API was part of demonstrating context-awareness; could’ve opted into context-awareness while continuing to use
nan, but we might as well go whole hog.Wayland support
The much larger change snuck up on me: back in May, a French user downloaded PulsarNext in order to get Wayland support and was pleased with it… except that it assumed their keyboard was standard US QWERTY and was misinterpreting keystrokes. I discovered that this package’s keyboard detection was exclusively using X11 APIs, so when the user was on Wayland, we had no insight into their keyboard layout and fell back to a QWERTY assumption.
The next few days of code authorship were not my favorite, but eventually this got addressed. We now start out assuming the user is on Wayland, but will very quickly fall back to X11 if that assumption is violated in any way. (This also at least gives the X11 code path a chance to provide answers, since it may still be able to help us if the user is running the XWayland compatibility layer.)
I was able to do some basic sanity-checking in a Linux VM running Wayland. (Issue #3 chronicles my efforts.) I updated PulsarNext to point to this branch and the French user reported that the new version fixed their keyboard shortcuts.
So that’s where we are now. One of the big upsides of PulsarNext over mainline Pulsar for Linux users is that we inherit the Wayland support that was added to Electron itself at some point, but that support will be pretty shallow for non-US users if we can’t also detect their keystrokes properly.
Testing
Testing these changes is not easy. The existing spec suite did only basic checking of Linux. And keyboard input in particular is hard to test in a CI environment. (If anyone has bright ideas, let me know. My initial experiments were not fruitful.)
I’m hoping that @mauricioszabo has the ability to do some sanity checking in Linux. If not, we might just need to rely on the fact that this fixed layout handling for at least one Wayland user and land it for now.
Next steps
As with
pathwatcher, we have never relied on our own fork ofkeyboard-layout, and mainline Pulsar still points tokeyboard-layoutfrom NPM. So we don't need to publish@pulsar-edit/keyboard-layoutuntil after we land this change. But the publish workflow already exists in this repo and I'll make sure it has access to the necessary secret.