feat(common/core/desktop): Allow preserved key support by rc-swag · Pull Request #5850 · keymanapp/keyman

rc-swag · 2021-10-22T08:53:57Z

Add km_kbp_keyboard_get_key_rules to get a list of virtual keys and the shift modifiers that are used in the keyboard rules.
The data structure for each item in the list is

typedef struct {
  km_kbp_virtual_key key;   // The key the belongs
  uint32_t modifier_flag;   // modifier flag for the rule
} km_kbp_keyboard_key_rules;

There is more information in the kmx processor implmentation for a key. Line; dpOutput; dpContext;. These seemed to be processor specific, and they are coupled to Group again which is a kmx specific rule layout. I wasn't sure what would be useful to have in a list of "keys". So if I am after guidance if this data structure should contain more information in this public api call to make it a more useful API call. The other option would be to make this an encode/decode pair with the structured data type which would then allow more info to be added to km_kbp_keyboard_key_rules without breaking previous implementations. That is it would be backward compatible.

User Testing

All tests are for keyman for windows.

TEST_PRESERVEDKEYS
This is to test the key rules which contain AltGr or Shift + AltGr work correctly.

Load a keyboard khmer_angkor
Open LibreOffice or Notepad
Type a AltGr + o
Observere output of ឱ
Type a Shift + AltGr + o
Observere output of ᧨

keymanapp-test-bot · 2021-10-22T08:54:01Z

User Test Results

Test specification and instructions

✅ TEST_PRESERVEDKEYS (PASSED): The characters are output per instructions given, see the screenshot below. (notes)

Test Artifacts

common/core/desktop/include/keyman/keyboardprocessor.h

mcdurdin · 2021-11-02T20:07:35Z

common/core/desktop/include/keyman/keyboardprocessor.h

+
+
+void km_kbp_keyboard_key_list_dispose(km_kbp_keyboard_key *key_list);


Need to document this function too 😉

common/core/desktop/include/keyman/keyboardprocessor.h

mcdurdin · 2021-11-02T20:22:26Z

common/core/desktop/tests/unit/kmx/kmx_key_list.cpp

+  km_kbp_keyboard_key *key_rule_it = kb_key_list;
+  auto n = 0;
+  for (; key_rule_it->key; ++key_rule_it) {
+    ++n;
+  }


Can we assert the values returned as well? I would also like to see a test for a more complex keyboard with multiple groups and with the same keys referred in multiple groups (to validate that we don't list keys more than once). We cannot assume the list order so will need to be careful on testing results!

windows/src/engine/keyman32/preservedkeymap.cpp

mcdurdin · 2021-11-02T20:32:19Z

windows/src/engine/keyman32/preservedkeymap.cpp

+  } else {
+    if (!_td->lpActiveKeyboard->Keyboard) {
+      return FALSE;
+    }
+    return pkm.MapKeyboard(_td->lpActiveKeyboard->Keyboard, pPreservedKeys, cPreservedKeys);
+  }


Do we want to add the issue number for future removal of this code and related functions?

common/core/desktop/tests/unit/kmx/kmx_key_list.cpp

mcdurdin · 2021-11-11T04:46:14Z

Jenkins failure:

[2021-11-09T09:18:44.341Z] 55/57 key_list                                FAIL     0.01 s
[2021-11-09T09:18:44.341Z] 
[2021-11-09T09:18:44.341Z] --- command ---
[2021-11-09T09:18:44.341Z] /<<PKGBUILDDIR>>/common/core/desktop/build/arch/release/tests/unit/kmx/key_list /<<PKGBUILDDIR>>/common/core/desktop/build/arch/release/tests/unit/kmx
[2021-11-09T09:18:44.341Z] --- stderr ---
[2021-11-09T09:18:44.341Z] Test failed with 7 at ../../../tests/unit/kmx/kmx_key_list.cpp:46:
[2021-11-09T09:18:44.341Z]   km_kbp_keyboard_load(full_path.native().c_str(), &test_kb)

ermshiperete · 2021-11-12T08:48:58Z

common/core/desktop/tests/unit/kmx/049 - groups ctrlalt.kmn

@@ -0,0 +1,28 @@
+c Description: Tests multiple groups. Purpose here is to interleave output and backspaces from kmx processor to consumer


Looks like this test doesn't run (because it's not listed in meson.build). I see that it's used in kmx_key_list.cpp. If it's intended to be used only from that test than IMO it would be better and less confusing to rename it to kmx_key_list.kmn or something like that.

Yes, when I was first setting it up I as going to create a new expected key list comment to go in the header of the kmn so that it would test with kmx.cpp. I ended up with having it all testested from kmx_key_list.cpp I will rename this to the suggested name.

@ermshiperete I fixed the what I thought where the issues with kmx_key_list and the meson.build, but the jenkins build is still failing. Can you also add my user to the build so I can view it in Jenkins?

ermshiperete

Looks like 049 - groups ctrlalt.kmx is still present!?

common/core/desktop/tests/unit/kmx/kmx_key_list.cpp

ermshiperete · 2021-11-15T08:36:46Z

common/core/desktop/tests/unit/kmx/kmx_key_list.cpp

+    return 1;
+}
+
+km_kbp_keyboard_key kb_key_expected_list[] = {{49,16384},{50,16384},{66,16416},{67,16384},{67,16448},{97,0},{98,0}};


The test might be easier to understand if we'd use the KM_KBP_VKEY_* and KM_KBP_MODIFIER_* consts here, especially when looking at the .kmn file and trying to understand the expected list here.

ermshiperete · 2021-11-15T08:43:19Z

windows/src/engine/keyman32/preservedkeymap.cpp

+    return TRUE;
+  }
+
+  if (*cPreservedKeys < cKeys) {


Shouldn't we set *cPreservedKeys = cKeys here as well?

If we get to this if statement it means the size of the array of PreservedKeys pPreservedKeys is going to be big enough to store all the keys. So it returns False.

When ::MapKeyboardCore is called if pPreservedKeys is NULL, we set *cPreservedKeys to the size pPreservedKeys needs to be.

So in the other cases we don't set cPreservedKeys

ermshiperete · 2021-11-15T08:45:27Z

windows/src/engine/keyman32/preservedkeymap.cpp

+    ++cRules;
+  }
+  cKeys = cRules;
+  if (cKeys == 0) {


Shouldn't we set *cPreservedKeys = cKeys here as well?

I guess we could check if pPreservedKeys == NULL and set *cPreservedKeys = cKeys basically setting it to zero. but in this case we are returning FALSE anyway. So if the function is returning FALSE for failure, I don't think the *cPreservedKeys should be trusted.
This also mimics the behaviour of the current ::MapCore

mcdurdin

Changes LGTM

MakaraSok · 2021-11-16T02:22:52Z

TEST_PRESERVEDKEYS: PASSED The characters are output per instructions given, see the screenshot below.

rc-swag · 2021-11-16T04:41:16Z

@mcdurdin The comment @ermshiperete made about using constants instead of magic numbers in the kmx_list.cpp raised an issue. I now realise what I am putting in the km_kbp_keyboard_key is not a "virtual key" but kmx KEY that can either be a "Virtual Key" or a "Character Code" determined by the modifier flag KM_KBP_MODIFIER_VIRTUALKEY. So we have a kmx engine detail in the keyboard_processor API. This is not good. I then spent some time thinking how I could change this but I wasn't sure what we should do?

mcdurdin · 2021-11-16T22:43:20Z

I now realise what I am putting in the km_kbp_keyboard_key is not a "virtual key" but kmx KEY that can either be a "Virtual Key" or a "Character Code" determined by the modifier flag KM_KBP_MODIFIER_VIRTUALKEY. So we have a kmx ~~engine~~ processor detail in the ~~keyboard_processor~~ Keyman Core API.

Two possible solutions. I lean towards the first.

Translate the characters to US vkeys before returning from kmx processor

This is probably the cleanest way to solve the issue. MapUSCharToVK is defined in preservedkeymap.cpp and CharToKeyConversion.cpp (which we should consolidate...), so that'll need to be ported over to kmx processor.

        v_key = p_group->dpKeyArray[j].Key;
        modifier_flag = p_group->dpKeyArray[j].ShiftFlags;
        if(modifier_flag == 0) {
          if(!MapUSCharToVK(&v_key, &modifier_flag)) continue;
        }
  
        map_rules[std::make_pair(v_key,modifier_flag)] = modifier_flag;

Live with it

This alternative is also okay, although not great.

We have many kmx processor details that inform the design of the Keyman Core API. The fact that the kmx processor can report chars as well as vkeys should be documented, not ignored :-).

However, let's split out the char pattern as a separate bit property rather than encoding it into the modifier_flag:

typedef struct {
  
  km_kbp_virtual_key key;   /// a virtual key, or an ASCII character if `is_character` is non-zero.
  uint16_t modifier_state;  /// bitmask from `km_kbp_modifier_state` enum 
  uint8_t is_character;     /// if non-zero, `key` is an ascii character corresponding to
                            /// the US English key cap (shifted or unshifted), and
                            /// `modifier_state` is ignored.
} km_kbp_keyboard_key;

(Noted also, use modifier_state not modifier_flag, and it should be uint16_t not uint32_t!)

That hides the worst of the implementation details (a modifier_state that included the KM_KBP_MODIFIER_VIRTUALKEY flag would differs from all other uses of modifiers in the API -- ignoring debug api which is a bit special anyway). If other engines don't need to list chars in this API, then that's fine; the is_character field will be 0 in those cases.

mcdurdin · 2021-11-18T02:33:48Z

Per discussion, moving back to draft to eliminate use of characters in place of virtual keys outside the boundaries of kmx processor.

rc-swag · 2021-11-23T04:31:25Z

Per discussion, moving back to draft to eliminate use of characters in place of virtual keys outside the boundaries of kmx processor.

This work has now been done and ready for re-review.

mcdurdin · 2021-11-29T00:14:42Z

Jenkins error:

[2021-11-23T04:36:16.697Z] ../../../common/core/desktop/src/kmx/ibus_keyman_tests-kmx_processor.o: In function `km::kbp::kmx_processor::get_key_list() const':

[2021-11-23T04:36:16.697Z] ./linux/ibus-keyman/tests/../../../common/core/desktop/src/kmx/kmx_processor.cpp:221: undefined reference to `km::kbp::kmx::MapUSCharToVK(unsigned short, unsigned short*, unsigned int*)'

[2021-11-23T04:36:16.697Z] collect2: error: ld returned 1 exit status

This suggests kmx_conversion.cpp needs to be added to linux\ibus-keyman\tests\Makefile.am. (It's a shame we are not using meson here!)

The pkeys was using the wrong counter as a index in the map check therefore matching the incorrect key

Co-authored-by: Marc Durdin <marc@durdin.net>

Also update issue comment for future removal of winows processor code

Co-authored-by: Eberhard Beilharz <ermshiperete@users.noreply.github.com>

ermshiperete · 2021-11-29T15:58:19Z

It's a shame we are not using meson here!

Indeed. I have it on my to-do list though 😄 - probably should create an issue for it: #5981

keyman-server · 2021-11-29T18:01:31Z

Changes in this pull request will be available for download in Keyman version 15.0.157-alpha

keymanapp-test-bot bot added the user-test-required User tests have not been completed label Oct 22, 2021

github-actions bot added common/ common/core/ core/ Keyman Core windows/ windows/engine/ labels Oct 22, 2021

rc-swag marked this pull request as ready for review October 29, 2021 07:31

darcywong00 added this to the A15S17 milestone Oct 30, 2021

darcywong00 changed the title ~~Feat/windows/preserved key support~~ feat(common/core/desktop) Allow preserved key support Nov 1, 2021

darcywong00 changed the title ~~feat(common/core/desktop) Allow preserved key support~~ feat(common/core/desktop): Allow preserved key support Nov 2, 2021

mcdurdin requested changes Nov 2, 2021

View reviewed changes

github-actions bot added the feat label Nov 7, 2021

rc-swag commented Nov 9, 2021

View reviewed changes

common/core/desktop/tests/unit/kmx/kmx_key_list.cpp Outdated Show resolved Hide resolved

rc-swag marked this pull request as draft November 11, 2021 05:26

ermshiperete reviewed Nov 12, 2021

View reviewed changes

mcdurdin modified the milestones: A15S17, A15S18 Nov 14, 2021

rc-swag marked this pull request as ready for review November 15, 2021 03:39

rc-swag requested review from ermshiperete and mcdurdin November 15, 2021 03:39

ermshiperete reviewed Nov 15, 2021

View reviewed changes

mcdurdin approved these changes Nov 15, 2021

View reviewed changes

keymanapp-test-bot bot removed the user-test-required User tests have not been completed label Nov 16, 2021

mcdurdin marked this pull request as draft November 18, 2021 02:32

rc-swag marked this pull request as ready for review November 23, 2021 04:30

rc-swag and others added 16 commits November 29, 2021 10:51

feat(windows): inital commit get rule keys api

9c1b670

feat(windows): complete common core get rules api

72d8188

feat(windows): fix pkeys index add comments

9f44cf4

The pkeys was using the wrong counter as a index in the map check therefore matching the incorrect key

feat(windows): rename key_rules -> key_list

ac16abd

feat(windows): Fix auto using int instead of uint

edfa9bc

feat(windows): apply suggestions from code review

4e1ccfd

Co-authored-by: Marc Durdin <marc@durdin.net>

feat(windows): Make keyboad key list unique

2353590

feat(windows): replace insert_or_assign with[] for hashmap

31d7200

Also update issue comment for future removal of winows processor code

feat(windows): remove unused vk_count var

4d1eee9

feat(windows): Rename kbd file fix meson build for key list test

6f6600a

feat(windows): Replace magic numbers with constants

c95e565

feat(core): update copyright

3df2887

Co-authored-by: Eberhard Beilharz <ermshiperete@users.noreply.github.com>

feat(windows): clean up old kbd kmx

58384ed

feat(windows): add MapUSCharToVK to kmx processor

c875cdb

feat(windows): add MapUSCharToVK to kmx processor

059af9e

feat(windows): add comment for clarity

cfed6f1

rc-swag force-pushed the feat/windows/preserved-key-support branch from c154681 to cfed6f1 Compare November 29, 2021 00:55

feat(windows): add kmx_conversion.cpp to test Makefile.am

b54be8a

github-actions bot added linux/ linux/engine/ labels Nov 29, 2021

rc-swag merged commit 13fc2bf into master Nov 29, 2021

rc-swag deleted the feat/windows/preserved-key-support branch November 29, 2021 04:48

mcdurdin added the has-user-test label Feb 12, 2022



		void km_kbp_keyboard_key_list_dispose(km_kbp_keyboard_key *key_list);

		@@ -0,0 +1,28 @@
		c Description: Tests multiple groups. Purpose here is to interleave output and backspaces from kmx processor to consumer

Uh oh!

Conversation

rc-swag commented Oct 22, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

User Testing

Uh oh!

keymanapp-test-bot bot commented Oct 22, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

User Test Results

Test Artifacts

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mcdurdin commented Nov 11, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ermshiperete left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rc-swag Nov 15, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mcdurdin left a comment

Choose a reason for hiding this comment

Uh oh!

MakaraSok commented Nov 16, 2021

Uh oh!

rc-swag commented Nov 16, 2021

Uh oh!

mcdurdin commented Nov 16, 2021

Translate the characters to US vkeys before returning from kmx processor

Live with it

Uh oh!

mcdurdin commented Nov 18, 2021

Uh oh!

rc-swag commented Nov 23, 2021

Uh oh!

mcdurdin commented Nov 29, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ermshiperete commented Nov 29, 2021

Uh oh!

keyman-server commented Nov 29, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

rc-swag commented Oct 22, 2021 •

edited

Loading

keymanapp-test-bot bot commented Oct 22, 2021 •

edited

Loading

rc-swag Nov 15, 2021 •

edited

Loading

mcdurdin commented Nov 29, 2021 •

edited

Loading