feat(core): add event_flags to process_event #7046 🙀

[srl295]

• Width: uint16_t
• defined km_kbp_event_flags to define the bitfield
• bitfield has value KM_KBP_EVENT_FLAG_TOUCH for touch events
• ignored everywhere, currently

Fixes: #7046

@keymanapp-test-bot skip

[keymanapp-test-bot[bot]]

User Test Results

Test specification and instructions

User tests are not required

Test Artifacts

• Developer
  • Keyman Developer
  • Compiler Regression Tests
  • kmcomp.zip
• Keyboards
  • Test Keyboards
• Linux
  • Keyman for Linux
• macOS
  • Keyman for macOS
• Windows
  • Keyman for Windows
  • FirstVoices Keyboards for Windows

[mcdurdin]

This has failing builds for Windows:

[17:34:19][Step 1/3] "C:\BuildAgent\work\7ac43416c45637e9\keyman\windows\src\engine\keyman32\keyman32.vcxproj" (Rebuild target) (1) ->
[17:34:19][Step 1/3] (ClCompile target) -> 
[17:34:19][Step 1/3]   C:\BuildAgent\work\7ac43416c45637e9\keyman\windows\src\engine\keyman32\kmprocess.cpp(113,27): error C2660: 'km_kbp_process_event': function does not take 4 arguments [C:\BuildAgent\work\7ac43416c45637e9\keyman\windows\src\engine\keyman32\keyman32.vcxproj]
[17:34:19][Step 1/3] 
[17:34:19][Step 1/3]     0 Warning(s)
[17:34:19][Step 1/3]     1 Error(s)

and Developer:

[17:44:30][Step 1/4] "C:\BuildAgent\work\7ac43416c45637e9\keyman\developer\src\tike\tike.dproj" (default target) (1) ->
[17:44:30][Step 1/4] (_PasCoreCompile target) -> 
[17:44:30][Step 1/4]   main\Keyman.System.KeymanCore.pas(312): error E2029: Identifier expected but ')' found [C:\BuildAgent\work\7ac43416c45637e9\keyman\developer\src\tike\tike.dproj]
[17:44:30][Step 1/4]   debug\Keyman.System.Debug.DebugCore.pas(13): error F2063: Could not compile used unit 'Keyman.System.KeymanCore.pas' [C:\BuildAgent\work\7ac43416c45637e9\keyman\developer\src\tike\tike.dproj]
[17:44:30][Step 1/4] 

[srl295]

@mcdurdin looks like it was an out of date build that failed. I'll try rerunning…

[srl295]

it actually probably could use some kind of test because it affects all of the process_event paths. but that could be done later, when further changes are made

[srl295]

fix commit 56aeee3 was accidentally added to #7071 … it will come out in the ~wash~ (rebased that PR, all ok now).

[srl295]

uf.

https://build.palaso.org/buildConfiguration/KeymanDesktop_TestPullRequests/332386?buildTab=log&logView=flowAware&focusLine=2398&linesState=329

C:\BuildAgent\work\7ac43416c45637e9\keyman\windows\src\engine\keyman32\kmprocess.cpp(113,27): error C2660: 'km_kbp_process_event': function does not take 4 arguments [C:\BuildAgent\work\7ac43416c45637e9\keyman\windows\src\engine\keyman32\keyman32.vcxproj]

but yet looking at the build log:

https://github.com/keymanapp/keyman/commit/e8dff5cf4e015ce1e28e9b31ba0977112bc001b9#diff-8762b89391fb2aafa43506a684aedaae668bfff3551b906ac03ede86e7eb9141R115

https://github.com/keymanapp/keyman/blob/e8dff5cf4e015ce1e28e9b31ba0977112bc001b9/windows/src/engine/keyman32/kmprocess.cpp#L115

it's being called with 5 arguments not 4.

Is the build somehow out of date?

[srl295]

), (uint8_t)_td->state.isDown), KM_KBP_EVENT_FLAG_DEFAULT)

should be

), (uint8_t)_td->state.isDown, KM_KBP_EVENT_FLAG_DEFAULT))

[srl295]

Discussion with @rc-swag —

• [ ] will move the is_key_down parameter into a flag. So process_event will go from 4 parameters to 4.

[mcdurdin]

Discussion with @rc-swag —

• [ ] will move the is_key_down parameter into a flag. So process_event will go from 4 parameters to 4.

I'm not terribly comfortable with this. The reason is, process_event is an API surface. It's internal, sure, but we still try to maintain API stability on internal APIs. If we change the shape of the API, by adding a parameter, existing code will fail to compile*, but fixes will be straightforward -- in most cases we just need to _kmn_unused(new_param). However, if we change the meaning of an API parameter without changing the shape of the call, we have to (a) manually audit all usages, and (b) update every reference to work with the new meaning. Change-wise, this introduces greater risk, and also forces us to update all components at the same time.

• Caveat: as long as we update API declarations for all languages -- e.g. we may have API decls in Delphi and Python as well as C++!

For a public API, we would never change the shape or meaning of an API call -- we'd introduce an new API surface and mark the old one as deprecated. For an internal API, I am willing to have shape changes.

Now, I know that in semantic versioning, the idea is that major releases can change APIs. I personally hate this: a given API function, once published, should remain stable both in shape and meaning for as long as that is viable -- only changing, for example, if security flaw repairs require it. This greatly simplifies the task of updating dependencies for downstream devs, and in the end we all win.

[rc-swag]

I agree with what you say in principle and maybe the ship has already sailed. It's just in the space of a couple of months we have just added two arguments which are just flags. Each time the API call is changing and breaking by doing that. With this change, it is more resilient to future updates - at least for simple flags. I suggested this because the is_keydown was recent change. The ideal is to have an ordered struct with an encoder and decoder so that you can handle updates without breaking any existing consumers of the API call but that ship has also sailed, it also doesn't really match the pattern of the core. To match the pattern of the core it would be array-like keyboard options and context. Either way, it would be able to grow the information required in a process_event API call without having to break the API call just for the addition of new event data.

Happy to not have this change while better it is still not the ideal solution think we should look to better handling of event data across API boundaries in the future.

[mcdurdin]

Yep, good points. We do have more flexibility on the API given this is internal. Currently, we have the following API consumers that I am aware of:

1. Tests (if they can be considered consumers)
2. Keyman for Linux
3. Keyman for Windows
4. Keyman Developer - debugger
5. Keyman Developer - OSK import from .kmx

And I'd forgotten about #5, FWIW.

In terms of good API design, you are right too. I was concerned more with stability of existing APIs but it's a shame we didn't consider making key_down a bit flag from the beginning.

Happy to be flexible as long as we can be sure we verify the changes properly (user test may be required?)

[srl295]

I think this pr already hits all call sites. I'll merge and we can consider followon, it's still the feature branch.

[srl295]

However, if we change the meaning of an API parameter without changing the shape of the call, we have to (a) manually audit all usages, and (b) update every reference to work with the new meaning. Change-wise, this introduces greater risk, and also forces us to update all components at the same time.

to be clear I would keep the parameter count but change the type.
Actually, 99% of the call sites (tests) ended in …,1); meaning "key-down event" . If 1 was defined as "the flag for a keydown event" then it would be a compatible call. If we're hitting all sites

But i do hear you about silent failures, if it's too compatible then we won't catch the issues.

as long as we update API declarations for all languages

this PR updates pascal and python already.

In any event I'm unblocked from further work, so I think I will proceed with the meat of the implementation.