change(linux): Implement ordered output without patched ibus #11535

ermshiperete · 2024-05-24T16:53:55Z

This change allows to press F24 as ordered output sentinel from keyman-system-service.

This is part of implementing serialized output with keyman-system-service instead of requiring a patched ibus.

The problem both approaches try to solve is that with non-compliant apps it is not possible to directly delete characters from the context. Instead we have to emit a backspace key before we can commit the new characters. However, the backspace key press goes through a different code path in ibus and so it can happen that the commit gets processed before the backspace which then deletes from the characters we just added instead of from the old content.

The previous implementation solved this by forwarding a F24 ordered output sentinel key to ibus and relying on the patched ibus to send that back to us. When we received the F24 key we committed the characters that we queued when we forwarded the F24 key (implemented in #7079).

The new approach implemented in this change instead sends the F24 ordered output sentinel key through keyman-system-service and so follows the regular key processing without requiring a patched ibus to send the key back to us. The rest of the algorithm stays the same: when we receive the F24 key we commit the characters previously queued.

Closes: #10799

keymanapp-test-bot · 2024-05-24T16:54:00Z

User Test Results

Test specification and instructions

ERROR: user tests have not yet been defined

Test Artifacts

Linux

ermshiperete · 2024-05-24T16:55:36Z

linux/ibus-keyman/src/engine.c

+#define KEYMAN_LALT   56 // 0x38
+#define KEYMAN_RCTRL  97 // 0x61
+#define KEYMAN_RALT  100 // 0x64
+#define KEYMAN_F24_KEYCODE_OUTPUT_SENTINEL  194 // 0xC2


In debugging I realized that the key code we used previously was not F24...

Should we be changing the value or changing the define name? 😁

202 is the X11 keycode for F24 (= kernel keycode + 8)

ermshiperete · 2024-05-24T17:03:44Z

linux/ibus-keyman/src/engine.c

@@ -639,6 +617,7 @@ process_backspace_action(IBusEngine *engine, unsigned int code_points_to_delete)
  } else {
    g_message("%s: non-compliant app: forwarding %d backspaces", __FUNCTION__, code_points_to_delete);
    while (code_points_to_delete > 0) {
+      // REVIEW: Do we have to go through keyman-system-service for sending the backspace?


It seems it might be better to go through keyman-system-service for the backspace so that they get processed by ibus in the correct order with the F24 keys.

No. If we go through keyman-system-service we simulate a keypress which means it round-trips and comes back to us.

If we directly send it to ibus we use ibus_engine_forward_key_event which gets processed by the client and doesn't come back to us, resulting in the correct output.

mcdurdin · 2024-08-21T07:59:41Z

linux/ibus-keyman/src/KeymanSystemServiceClient.cpp

+order_output(gboolean isKeyUp) {
+  g_message("%s: Calling order output sentinel on keyman-system-service: isKeyDown=%d",
+    __FUNCTION__, !isKeyUp);
+  KeymanSystemServiceClient client;


Is this going to be expensive to instantiate (and destroy) (twice) on every keystroke? Should we be storing a global instance?

mcdurdin · 2024-08-21T08:01:06Z

linux/ibus-keyman/src/engine.c

+#define KEYMAN_LALT   56 // 0x38
+#define KEYMAN_RCTRL  97 // 0x61
+#define KEYMAN_RALT  100 // 0x64
+#define KEYMAN_F24_KEYCODE_OUTPUT_SENTINEL  194 // 0xC2


Should we be changing the value or changing the define name? 😁

mcdurdin · 2024-08-21T08:05:32Z

linux/keyman-system-service/src/KeymanSystemService.cpp

@@ -70,6 +93,7 @@ KeymanSystemService::KeymanSystemService()
  int ret;

  GetKbdDevices();
+  CreateOrderOutputDevice();


Suggested change

CreateOrderOutputDevice();

CreateOrderedOutputDevice();

and elsewhere

mcdurdin · 2024-08-21T08:06:12Z

linux/keyman-system-service/src/KeymanSystemService.cpp

@@ -115,6 +139,12 @@ KeymanSystemService::~KeymanSystemService()
      delete device;
    }
    delete kbd_devices;
+    kbd_ordered_output = nullptr;


Suggested change

kbd_ordered_output = nullptr;

kbd_devices = nullptr;

mcdurdin · 2024-08-21T08:06:30Z

linux/keyman-system-service/src/KeymanSystemService.cpp

@@ -166,6 +196,16 @@ void KeymanSystemService::GetKbdDevices() {
  }
 }

+void KeymanSystemService::CreateOrderOutputDevice() {
+  if (!kbd_ordered_output) {
+    kbd_ordered_output = new OrderOutputDevice();


Suggested change

kbd_ordered_output = new OrderOutputDevice();

kbd_ordered_output = new OrderedOutputDevice();

ditto 😁

mcdurdin · 2024-08-21T08:07:04Z

linux/keyman-system-service/src/KeymanSystemService.h

@@ -3,18 +3,21 @@

 #include <list>
 #include <systemd/sd-bus.h>
+#include "OrderOutputDevice.h"


Suggested change

#include "OrderOutputDevice.h"

#include "OrderedOutputDevice.h"

mcdurdin · 2024-08-21T08:08:03Z

linux/keyman-system-service/src/OrderOutputDevice.cpp

+
+using namespace std;
+
+#define KEYMAN_F24_KEYCODE_OUTPUT_SENTINEL 194  // 0xC2


Ideally we should be sharing value with ibus-keyman rather than defining in two places

mcdurdin · 2024-08-21T08:09:29Z

linux/keyman-system-service/src/OrderOutputDevice.cpp

+      __FUNCTION__, isKeyDown, strerror(-error));
+    return false;
+  }
+  error = libevdev_uinput_write_event(uinput_dev, EV_SYN, SYN_REPORT, 0);


Can we add a comment on this?

mcdurdin · 2024-08-21T08:10:17Z

linux/keyman-system-service/src/com.keyman.SystemService1.System.xml

+      CallOrderedOutputSentinel:
+      @isKeyDown: Whether or not we're dealing with a keydown or keyup
+
+      Press the orderd output sentinel key to serialize the output.


Suggested change

Press the orderd output sentinel key to serialize the output.

Press the ordered output sentinel key to serialize the output.

…ervice This change allows to press F24 as ordered output sentinel from keyman-system-service. This is part of implementing serialized output with keyman-system-service instead of requiring a patched ibus. The problem both approaches try to solve is that with non-compliant apps it is not possible to directly delete characters from the context. Instead we have to emit a backspace key before we can commit the new characters. However, the backspace key press goes through a different code path in ibus and so it can happen that the commit gets processed before the backspace which then deletes from the characters we just added instead of from the old content. The previous implementation solved this by forwarding a F24 ordered output sentinel key to ibus and relying on the patched ibus to send that back to us. When we received the F24 key we committed the characters that we queued when we forwarded the F24 key (implemented in #7079). The new approach implemented in this change instead sends the F24 ordered output sentinel key through keyman-system-service and so follows the regular key processing without requiring a patched ibus to send the key back to us. The rest of the algorithm stays mostly the same: when we receive the F24 key we commit the characters previously queued. The difference to the previous implementation is that we now also queue the backspace keys that we generate. Part-of: #10799

This change implements things on the ibus-keyman side. Fixes: #10799

keymanapp-test-bot bot added the user-test-missing User tests have not yet been defined for the PR label May 24, 2024

keymanapp-test-bot bot added this to the A18S2 milestone May 24, 2024

github-actions bot added linux/ linux/engine/ refactor labels May 24, 2024

ermshiperete force-pushed the refactor/linux/10799_SerializedOutput branch 2 times, most recently from 1d2a12e to eb3b6db Compare May 24, 2024 17:01

ermshiperete commented May 24, 2024

View reviewed changes

mcdurdin modified the milestones: A18S2, A18S3 May 24, 2024

ermshiperete changed the title ~~refactor(linux): Implement orered output without patched ibus~~ refactor(linux): Implement ordered output without patched ibus May 27, 2024

ermshiperete mentioned this pull request May 27, 2024

bug: Displayed contributions don't match calculated contributions keymanapp/status.keyman.com#449

Open

ermshiperete changed the title ~~refactor(linux): Implement ordered output without patched ibus~~ change(linux): Implement ordered output without patched ibus May 27, 2024

ermshiperete force-pushed the refactor/linux/10799_SerializedOutput branch from eb3b6db to 53c3d2b Compare May 28, 2024 13:39

ermshiperete modified the milestones: A18S3, A18S4 May 28, 2024

github-actions bot added the change label May 28, 2024

ermshiperete removed the refactor label May 28, 2024

ermshiperete force-pushed the refactor/linux/10799_SerializedOutput branch from 53c3d2b to 25959aa Compare June 13, 2024 10:54

darcywong00 modified the milestones: A18S4, A18S5 Jun 21, 2024

darcywong00 modified the milestones: A18S5, A18S6 Jul 8, 2024

darcywong00 modified the milestones: A18S6, A18S7 Jul 19, 2024

darcywong00 modified the milestones: A18S7, A18S8 Aug 2, 2024

darcywong00 modified the milestones: A18S8, A18S9 Aug 17, 2024

mcdurdin reviewed Aug 21, 2024

View reviewed changes

ermshiperete added 2 commits August 21, 2024 17:15

refactor(linux): Implement ordered output without patched ibus

436d00a

This change implements things on the ibus-keyman side. Fixes: #10799

ermshiperete force-pushed the refactor/linux/10799_SerializedOutput branch from 25959aa to 436d00a Compare August 21, 2024 16:25

darcywong00 modified the milestones: A18S9, A18S10 Aug 31, 2024

darcywong00 modified the milestones: A18S10, A18S11 Sep 14, 2024

darcywong00 modified the milestones: A18S11, A18S12 Sep 28, 2024

darcywong00 modified the milestones: A18S12, A18S13 Oct 11, 2024

darcywong00 modified the milestones: A18S13, A18S14 Oct 26, 2024

darcywong00 modified the milestones: A18S14, A18S15 Nov 9, 2024

darcywong00 modified the milestones: A18S15, A18S16 Nov 24, 2024

darcywong00 modified the milestones: A18S16, A18S17 Dec 7, 2024

darcywong00 modified the milestones: A18S17, A18S18 Dec 21, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

change(linux): Implement ordered output without patched ibus #11535

change(linux): Implement ordered output without patched ibus #11535

ermshiperete commented May 24, 2024 •

edited

Loading

keymanapp-test-bot bot commented May 24, 2024 •

edited

Loading

ermshiperete May 24, 2024

mcdurdin Aug 21, 2024

ermshiperete Aug 21, 2024

ermshiperete May 24, 2024

ermshiperete Jun 11, 2024

mcdurdin Aug 21, 2024

mcdurdin Aug 21, 2024

mcdurdin Aug 21, 2024

mcdurdin Aug 21, 2024

mcdurdin Aug 21, 2024

mcdurdin Aug 21, 2024

mcdurdin Aug 21, 2024

mcdurdin Aug 21, 2024

mcdurdin Aug 21, 2024

	kbd_ordered_output = new OrderOutputDevice();
	kbd_ordered_output = new OrderedOutputDevice();

	#include "OrderOutputDevice.h"
	#include "OrderedOutputDevice.h"


		using namespace std;

		#define KEYMAN_F24_KEYCODE_OUTPUT_SENTINEL 194 // 0xC2

	Press the orderd output sentinel key to serialize the output.
	Press the ordered output sentinel key to serialize the output.

change(linux): Implement ordered output without patched ibus #11535

Are you sure you want to change the base?

change(linux): Implement ordered output without patched ibus #11535

Conversation

ermshiperete commented May 24, 2024 • edited Loading

keymanapp-test-bot bot commented May 24, 2024 • edited Loading

User Test Results

Test Artifacts

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ermshiperete commented May 24, 2024 •

edited

Loading

keymanapp-test-bot bot commented May 24, 2024 •

edited

Loading