Improve keyboard switching on Ubuntu 18.04 #953

ermshiperete · 2020-06-26T09:47:35Z

This is a follow-up of PR #949. This change introduces a combined
keyboard for keyboards in GnomeShell. With that we're able to
undo some of the previous changes from #949. This change also adds
a bunch of unit tests for the GnomeShell*Adaptor classes.

This change is

Newer versions of Moq cause a crash when debugging unit tests. The reason isn't completely clear, the workarounds mentioned in the Moq issue tracker don't work for me, so for now I'm downgrading.

This change adjusts the test to the modified behavior of the library. Things recently changed so that a unknown keyboard type is of type `UnsupportedKeyboardDefinition` instead of `XkbKeyboardDescription`.

Oops

marksvc

Thanks for your work on this. Thanks for splitting it up into different commits; especially splitting the refactoring out into a separate commit.

I approve, minus my many comments.

marksvc · 2020-06-29T16:38:48Z

SIL.Windows.Forms.Keyboarding.Tests/TestHelper/GnomeShellIbusKeyboardRetrievingAdaptorDouble.cs

+			: base(new IbusKeyboardAdaptorTests.DoNothingIbusCommunicator())
+		{
+			_ibusKeyboards = ibusKeyboards;
+			ReflectionHelper.SetField(this, "_helper",


Reflection to set this._helper. That seems strange. Okay I see it is private readonly and this lets you not worry about that in the Test.

marksvc · 2020-06-29T16:43:22Z

SIL.Windows.Forms.Keyboarding/Linux/GList.cs

+
+namespace SIL.Windows.Forms.Keyboarding.Linux
+{
+	internal struct GList


I have assumptions about what this is, but it would help to have a brief comment making it clear. For example, "glibc doubly-linked list of strings. Helps interact with native code"

marksvc · 2020-06-29T17:03:20Z

SIL.Windows.Forms.Keyboarding/Linux/GlibHelper.cs

+				return list.ToArray();
+
+			var glist = GList.FromPtr(value);
+			for (; glist.HasNext; glist = glist.Next)


Won't this loop never run its contents for a list with only one element? Because the single-element list would fail the glist.HasNext condition.

Maybe instead we want something like

do { list.Add(glist.Data); glist = glist.Next; } while (glist.HasNext);

or.. something else that is a more pleasant structure than do while.

marksvc · 2020-06-29T17:14:51Z

SIL.Windows.Forms.Keyboarding/Linux/GnomeKeyboardRetrievingHelper.cs

 		{
 			var keyboards = new Dictionary<string, uint>();
 			var list = GetMyKeyboards();
 			uint kbdIndex = 0;
 			foreach (var t in list)
 				AddKeyboard(t, ref kbdIndex, keyboards, keyboardTypeMatches);

-			registerKeyboards(keyboards);
+			var firstKeyboard = SplitKeyboardEntry(list[0]);


list[0]

Are we sure there will be >0 elements in list and this not crash?
I guess that would be an odd situation.
But I've seen odd situations before :)

var firstKeyboard =

Hmm. It seems a bit fiddly to be passing around a keyboard by a pair of strings, rather than by an object that defines a keyboard. Do you think it's worth making a type for this, or is that too heavy? IIRC I saw the (string,string) in a number of other method signatures too.

marksvc · 2020-06-29T17:16:45Z

SIL.Windows.Forms.Keyboarding/Linux/GnomeShellIbusKeyboardRetrievingAdaptor.cs

+		/// <summary>
+		/// Initializes a new instance of the
+		/// <see cref="SIL.Windows.Forms.Keyboarding.Linux.GnomeShellIbusKeyboardRetrievingAdaptor"/> class.
+		/// Used in unit tests


Used in unit tests

What is this telling us? That this class is involved in our unit tests? That this class's primary purpose is for the unit tests?

marksvc · 2020-06-29T17:58:28Z

SIL.Windows.Forms.Keyboarding/Linux/GnomeShellIbusKeyboardRetrievingAdaptor.cs

+						continue;
+				}
+				else if (!keyboards.ContainsKey(ibusKeyboard.LongName))
+					continue;


I am noticing similarities in keyboard.ContainsKey() checks, and xkb-dependent setting of the layout variable.

It seems like we should be able to collapse this:

if (ibusKeyboard.LongName.StartsWith("xkb")) { type = typeof(IbusXkbKeyboardDescription); if (string.IsNullOrEmpty(ibusKeyboard.LayoutVariant) && keyboards.ContainsKey(ibusKeyboard.Layout)) layout = ibusKeyboard.Layout; else if (!string.IsNullOrEmpty(ibusKeyboard.LayoutVariant) && keyboards.ContainsKey($"{ibusKeyboard.Layout}+{ibusKeyboard.LayoutVariant}")) layout = $"{ibusKeyboard.Layout}+{ibusKeyboard.LayoutVariant}"; else continue; } else if (!keyboards.ContainsKey(ibusKeyboard.LongName)) continue;

to something like this:

if (ibusKeyboard.LongName.StartsWith("xkb")) { type = typeof(IbusXkbKeyboardDescription); layout = ibusKeyboard.Layout; if (!string.IsNullOrEmpty(ibusKeyboard.LayoutVariant) layout = $"{ibusKeyboard.Layout}+{ibusKeyboard.LayoutVariant}"; } if (!keyboards.ContainsKey(layout) continue;

And even just that collapsing may help communicate what's being checked around here. I found all the if this, if contains, continue, else, if this, else this to hinder understanding the whole.

marksvc · 2020-06-29T18:14:17Z

SIL.Windows.Forms.Keyboarding/Linux/GnomeShellIbusKeyboardRetrievingAdaptor.cs

+			_helper.InitKeyboards(type => true, RegisterKeyboards);
+		}
+
+		private void RegisterKeyboards(IDictionary<string, uint> keyboards, (string type, string layout) firstKeyboard)


I found I had to keep looking back and forth around this method to try to understand what it was doing, and what the difference is between keyboards and curKeyboards, or which was what meaning.
I think probably the foreach loop over all ibus keyboards contributed to having trouble understanding at first.

Could we either give a brief method comment saying what the general activity of this method's algorithm is, or sprinkle in some comments inside the method to describe the purpose for sections or some of the contents of the loop.

It would also help to rename keyboards to something like requestedKeyboards.

Maybe it could have a method comment that says something like "Activate all ibus or xkb keyboards in KeyboardController that are requested in the keyboards list. Deactivate the rest. Add any keyboards to KeyboardController that were missing."

FWIW, I am leaving my analysis of what the foreach loop looks to be doing:

for each ibusKeyboard that ibus knows about: skip processing if it is not in the request list (based on longname or xkb layout name) skip processing if we already processed a keyboard with this name/layout. if `curKeyboards` contains ibusKeyboard make sure it is set as available remove it from the `curKeyboards` list so we can later deactivate the rest if `curKeyboards` did not contain ibusKeyboard add the ibusKeyboard to KeyboardController (but maybe not necessarily to ibus's list of keyboards)

marksvc · 2020-06-29T18:20:38Z

SIL.Windows.Forms.Keyboarding/Linux/GnomeShellIbusKeyboardSwitchingAdaptor.cs

 		{
+			var ibusKeyboard = (IbusKeyboardDescription) keyboard;


This cast looks like it could fail. Do we know it will always be an object of class IbusKeyboardDescription?

marksvc · 2020-06-29T18:25:19Z

SIL.Windows.Forms.Keyboarding/Linux/GnomeXkbInfo.cs

+			// var displayNamePtr = IntPtr.Zero;
+			// var shortNamePtr = IntPtr.Zero;
+			// var xkbLayoutPtr = IntPtr.Zero;
+			// var xkbVariantPtr = IntPtr.Zero;


(Delete these 4 lines?)

ermshiperete

Reviewable status: 0 of 52 files reviewed, 9 unresolved discussions (waiting on @marksvc)

SIL.Windows.Forms.Keyboarding/Linux/GList.cs, line 9 at r1 (raw file):

Previously, marksvc wrote…

I have assumptions about what this is, but it would help to have a brief comment making it clear. For example, "glibc doubly-linked list of strings. Helps interact with native code"

Done.

SIL.Windows.Forms.Keyboarding/Linux/GlibHelper.cs, line 93 at r1 (raw file):

Previously, marksvc wrote…

Won't this loop never run its contents for a list with only one element? Because the single-element list would fail the glist.HasNext condition.

Maybe instead we want something like
do {
  list.Add(glist.Data);
  glist = glist.Next;
} while (glist.HasNext);
or.. something else that is a more pleasant structure than do while.

Good catch. Done.

SIL.Windows.Forms.Keyboarding/Linux/GnomeShellIbusKeyboardRetrievingAdaptor.cs, line 31 at r1 (raw file):

Previously, marksvc wrote…

Used in unit tests

What is this telling us? That this class is involved in our unit tests? That this class's primary purpose is for the unit tests?

Done.

SIL.Windows.Forms.Keyboarding/Linux/GnomeShellIbusKeyboardRetrievingAdaptor.cs, line 60 at r1 (raw file):

Previously, marksvc wrote…

I found I had to keep looking back and forth around this method to try to understand what it was doing, and what the difference is between keyboards and curKeyboards, or which was what meaning.
I think probably the foreach loop over all ibus keyboards contributed to having trouble understanding at first.

Could we either give a brief method comment saying what the general activity of this method's algorithm is, or sprinkle in some comments inside the method to describe the purpose for sections or some of the contents of the loop.

It would also help to rename keyboards to something like requestedKeyboards.

Maybe it could have a method comment that says something like "Activate all ibus or xkb keyboards in KeyboardController that are requested in the keyboards list. Deactivate the rest. Add any keyboards to KeyboardController that were missing."

FWIW, I am leaving my analysis of what the foreach loop looks to be doing:
for each ibusKeyboard that ibus knows about: 

  skip processing if it is not in the request list (based on longname or xkb layout name)
  skip processing if we already processed a keyboard with this name/layout.

  if `curKeyboards` contains ibusKeyboard
    make sure it is set as available
    remove it from the `curKeyboards` list so we can later deactivate the rest
  if `curKeyboards` did not contain ibusKeyboard
    add the ibusKeyboard to KeyboardController (but maybe not necessarily to ibus's list of keyboards)

Hope this is a bit clearer now.

SIL.Windows.Forms.Keyboarding/Linux/GnomeShellIbusKeyboardRetrievingAdaptor.cs, line 92 at r1 (raw file):

Previously, marksvc wrote…

I am noticing similarities in keyboard.ContainsKey() checks, and xkb-dependent setting of the layout variable.

It seems like we should be able to collapse this:
if (ibusKeyboard.LongName.StartsWith("xkb"))
{
	type = typeof(IbusXkbKeyboardDescription);
	if (string.IsNullOrEmpty(ibusKeyboard.LayoutVariant)
		&& keyboards.ContainsKey(ibusKeyboard.Layout))
		layout = ibusKeyboard.Layout;
	else if (!string.IsNullOrEmpty(ibusKeyboard.LayoutVariant)
		&& keyboards.ContainsKey($"{ibusKeyboard.Layout}+{ibusKeyboard.LayoutVariant}"))
		layout = $"{ibusKeyboard.Layout}+{ibusKeyboard.LayoutVariant}";
	else
		continue;
}
else if (!keyboards.ContainsKey(ibusKeyboard.LongName))
	continue;
to something like this:
if (ibusKeyboard.LongName.StartsWith("xkb"))
{
	type = typeof(IbusXkbKeyboardDescription);
	layout = ibusKeyboard.Layout;
	if (!string.IsNullOrEmpty(ibusKeyboard.LayoutVariant)
		layout = $"{ibusKeyboard.Layout}+{ibusKeyboard.LayoutVariant}";
}
if (!keyboards.ContainsKey(layout)
	continue;
And even just that collapsing may help communicate what's being checked around here. I found all the if this, if contains, continue, else, if this, else this to hinder understanding the whole.

Done.

SIL.Windows.Forms.Keyboarding/Linux/GnomeShellIbusKeyboardSwitchingAdaptor.cs, line 26 at r1 (raw file):

Previously, marksvc wrote…

This cast looks like it could fail. Do we know it will always be an object of class IbusKeyboardDescription?

This will always be of a IbusKeyboardDescription derived type.

SIL.Windows.Forms.Keyboarding/Linux/GnomeXkbInfo.cs, line 34 at r1 (raw file):

Previously, marksvc wrote…

(Delete these 4 lines?)

Done.

SIL.Windows.Forms.Keyboarding.Tests/TestHelper/GnomeShellIbusKeyboardRetrievingAdaptorDouble.cs, line 20 at r1 (raw file):

Previously, marksvc wrote…

Reflection to set this._helper. That seems strange. Okay I see it is private readonly and this lets you not worry about that in the Test.

yep

SIL.Windows.Forms.Keyboarding/Linux/GnomeKeyboardRetrievingHelper.cs, line 39 at r1 (raw file):

Hmm. It seems a bit fiddly to be passing around a keyboard by a pair of strings, rather than by an object that defines a keyboard. Do you think it's worth making a type for this, or is that too heavy? IIRC I saw the (string,string) in a number of other method signatures too.

I'm not sure it helps in clarifying what's going on unless we come up with a very precise self-descriptive name. What would you call this type? All names I can think of I fear will increase the confusion because we already have several KeyboardDescription/KeyboardDefinition... classes, and it's not immediately obvious what the differences are and what purposes they serve.

marksvc

I approve. Tho see comments.

Thanks so much for your work on this.

Reviewed 26 of 26 files at r3.
Reviewable status: 26 of 53 files reviewed, 2 unresolved discussions (waiting on @ermshiperete and @marksvc)

SIL.Windows.Forms.Keyboarding/Linux/GnomeShellIbusKeyboardRetrievingAdaptor.cs, line 60 at r1 (raw file):

Previously, ermshiperete (Eberhard Beilharz) wrote…

Hope this is a bit clearer now.

Ohh. Yes this is very helpful.

SIL.Windows.Forms.Keyboarding/Linux/GnomeKeyboardRetrievingHelper.cs, line 39 at r1 (raw file):

Previously, ermshiperete (Eberhard Beilharz) wrote…

Hmm. It seems a bit fiddly to be passing around a keyboard by a pair of strings, rather than by an object that defines a keyboard. Do you think it's worth making a type for this, or is that too heavy? IIRC I saw the (string,string) in a number of other method signatures too.

I'm not sure it helps in clarifying what's going on unless we come up with a very precise self-descriptive name. What would you call this type? All names I can think of I fear will increase the confusion because we already have several KeyboardDescription/KeyboardDefinition... classes, and it's not immediately obvious what the differences are and what purposes they serve.

Hmm. Right. Maybe KeyboardTypeAndLayout.
I'll let you decide if that would help or hinder.

SIL.Windows.Forms.Keyboarding/Linux/GnomeKeyboardRetrievingHelper.cs, line 35 at r3 (raw file):
I see the

			var sources = Unmanaged.g_settings_get_value(settings, "sources");
			if (sources == IntPtr.Zero)
				return null;

in GetMyKeyboards(). I am not familiar with these library calls. Maybe sources would be IntPtr.Zero if there were no items to return, rather than a non-null list containing zero items. If not, I think we'd still need to check here that list.Length isn't <=0.

ermshiperete

Reviewable status: 25 of 54 files reviewed, all discussions resolved (waiting on @marksvc)

SIL.Windows.Forms.Keyboarding/Linux/GnomeKeyboardRetrievingHelper.cs, line 39 at r1 (raw file):

Previously, marksvc wrote…

Hmm. Right. Maybe KeyboardTypeAndLayout.
I'll let you decide if that would help or hinder.

That would be an idea. However, I think I'll leave it as is for now.

SIL.Windows.Forms.Keyboarding/Linux/GnomeKeyboardRetrievingHelper.cs, line 35 at r3 (raw file):

Previously, marksvc wrote…

I see the
			var sources = Unmanaged.g_settings_get_value(settings, "sources");
			if (sources == IntPtr.Zero)
				return null;
in GetMyKeyboards(). I am not familiar with these library calls. Maybe sources would be IntPtr.Zero if there were no items to return, rather than a non-null list containing zero items. If not, I think we'd still need to check here that list.Length isn't <=0.

done

This is a follow-up of PR #949. This change introduces a combined keyboard for keyboards in GnomeShell. With that we're able to undo some of the previous changes from #949. This change also adds a bunch of unit tests for the GnomeShell*Adaptor classes.

marksvc

I approve.

Reviewed 1 of 2 files at r4, 1 of 1 files at r5.
Reviewable status: 27 of 54 files reviewed, all discussions resolved (waiting on @marksvc)

TestApps/TestAppKeyboard/README.md, line 7 at r5 (raw file):

**NOTE:** On Linux typing in Winforms fields with a selected Keyman keyboard works only if
a version of libgdiplus is used that has the Pango rendering enabled, e.g. by installing and
using `mono5-sil` and related packages.

Good to note, thank you.

ermshiperete added 3 commits June 23, 2020 11:25

Reformat and use newer C# language features

43f6083

Downgrade Moq to 4.7.x

921dc08

Newer versions of Moq cause a crash when debugging unit tests. The reason isn't completely clear, the workarounds mentioned in the Moq issue tracker don't work for me, so for now I'm downgrading.

Fix failing test

3aa1ce2

This change adjusts the test to the modified behavior of the library. Things recently changed so that a unknown keyboard type is of type `UnsupportedKeyboardDefinition` instead of `XkbKeyboardDescription`.

marksvc previously approved these changes Jun 29, 2020

View reviewed changes

marksvc self-requested a review June 29, 2020 16:17

marksvc requested changes Jun 29, 2020

View reviewed changes

ermshiperete requested a review from marksvc July 6, 2020 17:13

ermshiperete commented Jul 6, 2020

View reviewed changes

marksvc approved these changes Jul 8, 2020

View reviewed changes

ermshiperete requested a review from marksvc July 14, 2020 10:00

ermshiperete commented Jul 14, 2020

View reviewed changes

marksvc approved these changes Jul 14, 2020

View reviewed changes

ermshiperete merged commit 4bf83b0 into sillsdev:feature/nuget Jul 15, 2020

ermshiperete deleted the feat/issue-887 branch July 15, 2020 11:10

ermshiperete mentioned this pull request Jul 16, 2020

[linux] Crash in keyboarding code on Ubuntu 20.04 #958

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve keyboard switching on Ubuntu 18.04 #953

Improve keyboard switching on Ubuntu 18.04 #953

ermshiperete commented Jun 26, 2020 •

edited

Loading

marksvc left a comment

marksvc Jun 29, 2020

marksvc Jun 29, 2020

marksvc Jun 29, 2020

marksvc Jun 29, 2020

marksvc Jun 29, 2020

marksvc Jun 29, 2020

marksvc Jun 29, 2020

marksvc Jun 29, 2020

marksvc Jun 29, 2020

ermshiperete left a comment

marksvc left a comment

ermshiperete left a comment

marksvc left a comment

Improve keyboard switching on Ubuntu 18.04 #953

Improve keyboard switching on Ubuntu 18.04 #953

Conversation

ermshiperete commented Jun 26, 2020 • edited Loading

marksvc left a 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 left a comment

Choose a reason for hiding this comment

marksvc left a comment

Choose a reason for hiding this comment

ermshiperete left a comment

Choose a reason for hiding this comment

marksvc left a comment

Choose a reason for hiding this comment

ermshiperete commented Jun 26, 2020 •

edited

Loading