feat(linux): Add dbus system service by ermshiperete · Pull Request #8509 · keymanapp/keyman

ermshiperete · 2023-03-24T16:18:28Z

This implements a dbus system service that allows to provide the missing functionality when running under Wayland.

Fixes #4273.

User Testing

Preparations

The tests should be run on these Linux platforms:
- GROUP_JAMMY_X11: Ubuntu 22.04 Jammy with Gnome Shell and X11
- GROUP_JAMMY_WAYLAND: Ubuntu 22.04 Jammy with Gnome Shell and Wayland
- GROUP_LUNAR_WAYLAND: Ubuntu 23.04 Lunar with Gnome Shell and Wayland
Install build artifacts of this PR
Reboot
Install Nanum fonts for typing Korean:
```
sudo apt install fonts-nanum
```
Change "Complex text layout" setting in LibreOffice Writer:
Expand to see the steps
- In LO Tools/Options, go to the Language Settings/Languages tab. Under "Default Languages for Documents" check the box for "Asian" and select "Korean (RoK)" as language, and check "Complex text layout" and select "Khmer" as language.
- You'll also have to change the default style: go to Styles/Manage Styles, right-click on "Default Paragraph Style" and select Modify. Go to the Font tab. For Asian Text Font, select Family "NanumMyeongjo" and Language "Korean (RoK)". For CTL Font, select Family "Khmer Mondulkiri" (or another "Khmer" font) and Language "Khmer".

Install "IPA (SIL)", "Korean KORDA Jamo (SIL)" , and "Khmer Angkor" keyman keyboards

Tests

TEST_IPA_WRITER: Open LibreOffice Writer. Switch to "IPA (SIL)" keyboard. Type n>. Verify that the result is "ŋ".
TEST_IPA_GEDIT: Open gedit. Switch to "IPA (SIL)" keyboard. Type n>. Verify that the result is "ŋ".
TEST_KO_WRITER: Open LibreOffice Writer. Switch to "Korean KORDA Jamo (SIL)" keyboard. Type han<space>geul<space>. Verify that the result is "한글".
TEST_KO_GEDIT: Open gedit. Switch to "Korean KORDA Jamo (SIL)" keyboard. Type han<space>geul<space>. Verify that the result is "한글".
TEST_KM_WRITER: Open LibreOffice Writer. Switch to "Khmer Angkor" keyboard. Type xEjmr. Verify that the output is "ខ្មែរ".
TEST_KM_GEDIT: Open gedit. Switch to "Khmer Angkor" keyboard. Type xEjmr. Verify that the output is "ខ្មែរ".
TEST_CONTEXT:
- Open gedit
- Switch to "IPA (SIL)" keyboard.
- Type
```
an
m
```
- click after n
- type >
- verify that the output is
```
aŋ
m
```

keymanapp-test-bot · 2023-03-24T16:18:33Z

User Test Results

Test specification and instructions

✅ GROUP_JAMMY_X11: Ubuntu 22.04 Jammy with Gnome Shell and X11
7 tests PASSED
- ✅ TEST_IPA_WRITER (PASSED): Tested with the attached PR build (17.0.107-1~PR-8509+jammy44) in Ubuntu 22.04 Jammy with Gnome Shell and X11 OS and here is my observation: 1. Verified that the typing n> produces the output ŋ in LibreOffice Writer Document.
- ✅ TEST_IPA_GEDIT (PASSED): Verified that the typing n> produces the output ŋ in Text Editor.
- ✅ TEST_KO_WRITER (PASSED): Verified that typing han,space>geul produces the output 한글 in LibreOffice Writer Document.
- ✅ TEST_KO_GEDIT (PASSED): Verified that typing han,space>geul produces the output 한글 in Text Editor.
- ✅ TEST_KM_WRITER (PASSED): Verified that typing xEjmr produces the ouput ខ្មែរ in LibreOffice Writer Document.
- ✅ TEST_KM_GEDIT (PASSED): Verified that typing xEjmr produces the ouput ខ្មែរ in Text Editor.
- ✅ TEST_CONTEXT (PASSED): Verified that after typing > after the letter n, it changed to ŋ. Seems to be working as expected. (notes)
✅ GROUP_JAMMY_WAYLAND: Ubuntu 22.04 Jammy with Gnome Shell and Wayland
7 tests PASSED
- ✅ TEST_IPA_WRITER (PASSED): Tested with the attached PR build (17.0.107-1~PR-8509+jammy44) in Ubuntu 22.04 Jammy with Gnome Shell and Wayland OS and here is my observation: 1. Verified that the typing n> produces the output ŋ in LibreOffice Writer Document.
- ✅ TEST_IPA_GEDIT (PASSED): Verified that the typing n> produces the output ŋ in Text Editor.
- ✅ TEST_KO_WRITER (PASSED): Verified that typing han,space>geul produces the output 한글 in LibreOffice Writer Document.
- ✅ TEST_KO_GEDIT (PASSED): Verified that typing han,space>geul produces the output 한글 in Text Editor.
- ✅ TEST_KM_WRITER (PASSED): Verified that typing xEjmr produces the ouput ខ្មែរ in LibreOffice Writer Document.
- ✅ TEST_KM_GEDIT (PASSED): Verified that typing xEjmr produces the ouput ខ្មែរ in Text Editor.
- ✅ TEST_CONTEXT (PASSED): Verified that after typing > after the letter n, it changed to ŋ. Seems to be working as expected. (notes)
🟩 GROUP_LUNAR_WAYLAND: Ubuntu 23.04 Lunar with Gnome Shell and Wayland
- ✅ TEST_IPA_WRITER (PASSED): Tested with the attached PR build (17.0.108-1~PR-8509+lunar48) in Ubuntu 23.04 Lunar with Gnome Shell and Wayland OS and here is my observation: 1. Verified that the typing n> produces the output ŋ in LibreOffice Writer Document.
- ✅ TEST_IPA_GEDIT (PASSED): Verified that the typing n> produces the output ŋ in Text Editor.
- ✅ TEST_KO_WRITER (PASSED): Verified that typing han,space>geul produces the output 한글 in LibreOffice Writer Document.
- ✅ TEST_KO_GEDIT (PASSED): Verified that typing han,space>geul produces the output 한글 in Text Editor.
- ✅ TEST_KM_WRITER (PASSED): Verified that typing xEjmr produces the ouput ខ្មែរ in LibreOffice Writer Document.
- ✅ TEST_KM_GEDIT (PASSED): Verified that typing xEjmr produces the ouput ខ្មែរ in Text Editor.
- 🟩 TEST_CONTEXT (SKIPPED): ): I created bug(linux): context not picked up in Gnome Text Editor #8828 for the failing test.

Test Artifacts

Linux
- Keyman for Linux
- Keyman for Linux

mcdurdin · 2023-05-16T10:39:47Z

I hope to review this in next few days

bharanidharanj · 2023-05-16T11:50:14Z

Test Results

GROUP_LUNAR_WAYLAND: Ubuntu 23.04 Lunar with Gnome Shell and Wayland

TEST_IPA_WRITER (PASSED): Tested with the attached PR build (17.0.108-1~PR-8509+lunar48) in Ubuntu 23.04 Lunar with Gnome Shell and Wayland OS and here is my observation: 1. Verified that the typing n> produces the output ŋ in LibreOffice Writer Document.
TEST_IPA_GEDIT (PASSED): Verified that the typing n> produces the output ŋ in Text Editor.
TEST_KO_WRITER (PASSED): Verified that typing han,space>geul produces the output 한글 in LibreOffice Writer Document.
TEST_KO_GEDIT (PASSED): Verified that typing han,space>geul produces the output 한글 in Text Editor.
TEST_KM_WRITER (PASSED): Verified that typing xEjmr produces the ouput ខ្មែរ in LibreOffice Writer Document.
TEST_KM_GEDIT (PASSED): Verified that typing xEjmr produces the ouput ខ្មែរ in Text Editor.
TEST_CONTEXT (FAILED): Noticed that after typing > after the letter n, it is showing wrong output. Seems to be an issue.

..in LibreOffice Writer

..in Text Editor

darcywong00 · 2023-05-17T07:34:47Z

+  sd_bus* bus          = NULL;
+  int result;
+
+  if (capsLock > 1) {


Since capsLock is guint32, does that mean any non-zero value is invalid?

Any value besides 0 and 1 is invalid.

darcywong00 · 2023-05-17T07:40:21Z

+
+  if (ret < 0) {
+    g_error("Failed to connect to system bus: %s", strerror(-ret));
+    return;


Does this need exit_loop(slot, bus);?

Not really, since opening the user bus failed bus is not set, nor slot, but for consistency it would be better to add it.

This is because C++ RAII means we don't get nice things (try/finally) -- otherwise we need to wrap these variables in a class for automatic destruction, which is tedious boilerplate. Another pattern I see used is goto loop_cleanup, which at least can keep the cleanup inside the same function, and is thus a little clearer, a little shorter, and avoids accidentally missing the return statement, or calling the cleanup twice (which in the current model would be bad because it would result in double-frees):

if (ret < 0) { g_error("Failed to connect to system bus: %s", strerror(-ret)); goto cleanup; } // ... function continues ... cleanup: if (bus) sd_bus_release_name(bus, KEYMAN_TESTSVC_BUS_NAME); if (slot) sd_bus_slot_unref(slot); if (bus) sd_bus_close_unref(bus); }

I don't like gotos, so I put the whole thing in a class to benefit from C++ RAII 😄

I don't like gotos, so I put the whole thing in a class to benefit from C++ RAII 😄

I thought there was a good chance that you would say that! 🤣

darcywong00 · 2023-05-17T07:52:29Z

+    export WAYLAND_DISPLAY
+    WAYLAND_DISPLAY=$(grep "Using Wayland display" "$TMPFILE" | cut -d"'" -f2)


I notice this is how the previous code was. But why wouldn't the two lines be combined?

Suggested change

export WAYLAND_DISPLAY

WAYLAND_DISPLAY=$(grep "Using Wayland display" "$TMPFILE" | cut -d"'" -f2)

export WAYLAND_DISPLAY=$(grep "Using Wayland display" "$TMPFILE" | cut -d"'" -f2)

Combining it masks failures. See https://www.shellcheck.net/wiki/SC2155.

Follow-up of code review comments. # Keyman Conventional Commit suggestions: # - Consider appending the text ". Fixes #4273" to your commit message.

darcywong00

lgtm - I think @mcdurdin wants to get to this too.

ermshiperete · 2023-05-17T10:38:50Z

GROUP_LUNAR_WAYLAND
TEST_CONTEXT (SKIPPED): I created #8828 for the failing test.

mcdurdin · 2023-05-22T05:10:08Z

@ermshiperete note the format change for your test skip (the format you used skipped all tests)

mcdurdin

This looks really solid. I have one request around resource cleanup patterns, because I think the separate-function pattern is a bit fragile. But I didn't really see any other significant issues. Good job!

mcdurdin · 2023-05-22T05:21:44Z

+
+  if (ret < 0) {
+    g_error("Failed to connect to system bus: %s", strerror(-ret));
+    return;


This is because C++ RAII means we don't get nice things (try/finally) -- otherwise we need to wrap these variables in a class for automatic destruction, which is tedious boilerplate. Another pattern I see used is goto loop_cleanup, which at least can keep the cleanup inside the same function, and is thus a little clearer, a little shorter, and avoids accidentally missing the return statement, or calling the cleanup twice (which in the current model would be bad because it would result in double-frees):

if (ret < 0) { g_error("Failed to connect to system bus: %s", strerror(-ret)); goto cleanup; } // ... function continues ... cleanup: if (bus) sd_bus_release_name(bus, KEYMAN_TESTSVC_BUS_NAME); if (slot) sd_bus_slot_unref(slot); if (bus) sd_bus_close_unref(bus); }

mcdurdin · 2023-05-22T05:22:57Z

+    if (ret > 0)  // we processed a request, try to process another one, right-away
+      continue;


Suggested change

if (ret > 0) // we processed a request, try to process another one, right-away

continue;

if (ret > 0) {

// we processed a request, try to process another one, right-away

continue;

}

mcdurdin · 2023-05-22T05:25:40Z

+    if (exitFlag) {
+      g_debug("Exiting loop");
+      exit_loop(slot, bus);
+      return;
+    }


Suggested change

if (exitFlag) {

g_debug("Exiting loop");

exit_loop(slot, bus);

return;

}

if (exitFlag) {

// `exitFlag` can be modified by the callback function

// `on_exit_method` which can be called by `sd_bus_process`

g_debug("Exiting loop");

exit_loop(slot, bus);

return;

}

mcdurdin · 2023-05-22T05:26:27Z

+static void
+finish(sd_bus_error* error, sd_bus_message* msg, sd_bus * bus) {
+  if (error) { sd_bus_error_free(error); }
+  if (msg)   { sd_bus_message_unref(msg); }
+  if (bus)   { sd_bus_unref(bus); }
+}


I think I'd prefer to see the goto cleanup pattern which I've noted in another comment -- it's less vulnerable to copypasta errors.

mcdurdin · 2023-05-22T05:28:00Z

  else
-    echo "Can't find neither ${COMMON_ARCH_DIR}/release nor ${COMMON_ARCH_DIR}/debug"
-    exit 2
+    echo "Can't find neither ../../../build/$(arch)/debug nor ../../../build/$(arch)/release"


Suggested change

echo "Can't find neither ../../../build/$(arch)/debug nor ../../../build/$(arch)/release"

echo "Cannot find ../../../build/$(arch)/debug or ../../../build/$(arch)/release"

Yay English grammar (or ... "Can find neither a nor b" is technically okay but weird)

mcdurdin · 2023-05-22T05:39:27Z

+  int32_t ret;
+  uint32_t state;
+
+  ret_error = NULL;


This is a no-op. Did you mean to set the value rather than the pointer?

Suggested change

ret_error = NULL;

*ret_error = NULL;

mcdurdin · 2023-05-22T05:40:05Z

+) {
+  uint32_t state;
+
+  ret_error = NULL;


Did you mean to do this?

Suggested change

ret_error = NULL;

*ret_error = NULL;

mcdurdin · 2023-05-22T05:40:32Z

+static int finish(int ret, void *kbd_devices, sd_bus_slot *slot, sd_bus *bus) {
+  close_kbd_devices(kbd_devices);
+  sd_bus_slot_unref(slot);
+  sd_bus_unref(bus);
+
+  return ret < 0 ? 1 : 0;
+}


Again, would prefer a goto cleanup pattern

Refactored to make use of C++ RAII.

mcdurdin · 2023-05-22T05:40:52Z

+    if (ret > 0) // we processed a request, try to process another one, right-away
+      continue;


Suggested change

if (ret > 0) // we processed a request, try to process another one, right-away

continue;

if (ret > 0) {

// we processed a request, try to process another one, right-away

continue;

}

Also apply changes from code review comments.

mcdurdin

LGTM!

mcdurdin · 2023-05-22T22:33:36Z

+KeymanSystemServiceClient::~KeymanSystemServiceClient() {
+  if (error) { sd_bus_error_free(error); }
+  if (msg)   { sd_bus_message_unref(msg); }
+  if (bus)   { sd_bus_unref(bus); }
+}


In the pattern you have now, if you call SetCapsLockIndicator twice and both times an error is returned, you'll leak error. It would be good to make note of this -- that the class functions are intended to be called just once in the class lifecycle, for future maintainers.

mcdurdin · 2023-05-22T22:35:59Z

+  dbus  = g_test_dbus_new(G_TEST_DBUS_NONE);
+
+  // Add the private directory with our in-tree service files.
+  g_test_dbus_add_service_dir(dbus, KEYMAN_TEST_SERVICE_PATH);
+
+  std::cout << "Add service dir to " << KEYMAN_TEST_SERVICE_PATH << std::endl;
+
+  // Start the private D-Bus daemon
+  g_test_dbus_up(dbus);


What happens if any of these functions fail?

Dunno. Most of these functions are void. g_test_dbus_get_bus_address will return NULL only if g_test_dbus_up hasn't been called. Other than that there are no other possible failures listed in the documentation for these methods.

keyman-server · 2023-05-24T07:49:07Z

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

ermshiperete added this to the A17S9 milestone Mar 24, 2023

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

github-actions Bot added docs linux/ linux/engine/ feat labels Mar 24, 2023

mcdurdin modified the milestones: A17S9, A17S10 Apr 4, 2023

mcdurdin modified the milestones: A17S10, A17S11 Apr 15, 2023

mcdurdin modified the milestones: A17S11, A17S12 Apr 29, 2023

ermshiperete force-pushed the feat/linux/4273-wayland-systemd branch 8 times, most recently from 5f8baaf to 9f3db08 Compare May 12, 2023 16:00

github-actions Bot added the linux/config/ label May 12, 2023

ermshiperete changed the base branch from master to chore/linux/install-tc May 12, 2023 16:00

ermshiperete force-pushed the feat/linux/4273-wayland-systemd branch from 9f3db08 to 166dc83 Compare May 12, 2023 16:04

github-actions Bot added linux/config/ and removed linux/config/ labels May 12, 2023

ermshiperete force-pushed the feat/linux/4273-wayland-systemd branch from 166dc83 to f27038b Compare May 12, 2023 16:07

github-actions Bot added linux/config/ and removed linux/config/ labels May 12, 2023

ermshiperete force-pushed the feat/linux/4273-wayland-systemd branch from f27038b to cf96012 Compare May 12, 2023 16:18

keymanapp-test-bot Bot added user-test-failed and removed user-test-required User tests have not been completed labels May 16, 2023

darcywong00 reviewed May 17, 2023

View reviewed changes

feat(linux): Make cleanup more consistent

16daabd

Follow-up of code review comments. # Keyman Conventional Commit suggestions: # - Consider appending the text ". Fixes #4273" to your commit message.

github-actions Bot added linux/config/ and removed linux/config/ labels May 17, 2023

darcywong00 reviewed May 17, 2023

View reviewed changes

ermshiperete mentioned this pull request May 17, 2023

bug(linux): context not picked up in Gnome Text Editor #8828

Open

keymanapp-test-bot Bot removed the user-test-failed label May 22, 2023

mcdurdin requested changes May 22, 2023

View reviewed changes

ermshiperete added 2 commits May 22, 2023 18:10

chore(linux): Fix meson warnings

a820d34

feat(linux): Refactor to benefit from C++ RAII

ab25157

Also apply changes from code review comments.

ermshiperete requested a review from mcdurdin May 22, 2023 17:52

github-actions Bot added linux/config/ and removed linux/config/ labels May 22, 2023

mcdurdin approved these changes May 22, 2023

View reviewed changes

feat(linux): Address code review comments

1823e1f

github-actions Bot added linux/config/ and removed linux/config/ labels May 23, 2023

ermshiperete merged commit a7c3c54 into master May 23, 2023

ermshiperete deleted the feat/linux/4273-wayland-systemd branch May 23, 2023 14:26

nikita-moor mentioned this pull request May 1, 2025

question(linux): Arch Linux package - build with --prefix and test failure #13814

Closed

8 tasks

nikita-moor mentioned this pull request May 10, 2025

bug(linux): Korean keyboard malfunction in gedit #13880

Closed

8 tasks

		export WAYLAND_DISPLAY
		WAYLAND_DISPLAY=$(grep "Using Wayland display" "$TMPFILE" \| cut -d"'" -f2)

	export WAYLAND_DISPLAY
	WAYLAND_DISPLAY=$(grep "Using Wayland display" "$TMPFILE" \| cut -d"'" -f2)
	export WAYLAND_DISPLAY=$(grep "Using Wayland display" "$TMPFILE" \| cut -d"'" -f2)

		if (ret > 0) // we processed a request, try to process another one, right-away
		continue;

	echo "Can't find neither ../../../build/$(arch)/debug nor ../../../build/$(arch)/release"
	echo "Cannot find ../../../build/$(arch)/debug or ../../../build/$(arch)/release"

Uh oh!

Conversation

ermshiperete commented Mar 24, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

User Testing

Preparations

Tests

Uh oh!

keymanapp-test-bot Bot commented Mar 24, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

User Test Results

Test Artifacts

Uh oh!

mcdurdin commented May 16, 2023

Uh oh!

bharanidharanj commented May 16, 2023

Test Results

GROUP_LUNAR_WAYLAND: Ubuntu 23.04 Lunar with Gnome Shell and Wayland

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!

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!

Choose a reason for hiding this comment

Uh oh!

darcywong00 left a comment

Choose a reason for hiding this comment

Uh oh!

ermshiperete commented May 17, 2023 • edited by mcdurdin Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mcdurdin commented May 22, 2023

Uh oh!

mcdurdin left a comment

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!

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!

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!

Uh oh!

mcdurdin left a comment

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 commented Mar 24, 2023 •

edited

Loading

keymanapp-test-bot Bot commented Mar 24, 2023 •

edited

Loading

ermshiperete commented May 17, 2023 •

edited by mcdurdin

Loading