Created attachment 77388 [details] Proposed patch In Ubuntu, we've been using a patch to add a list of user keyboard layouts to accountsservice. This is useful for both using the user's preferred layout when entering their password in a greeter as well as letting the user switch between preferred layouts in that greeter. Here's a git patch to add KeyboardLayouts to the DBus interface and config file.
Again discussing this in Nuremberg we realized that it's a bit more complicated than just the layout, given input methods, keyboard options and so on. So this would be an array of ( dict (key, value) ) or aa{ss} * Each item in the outer array is a configured input source for the user. * key is 'xkb' for X keyboard layouts, 'ibus' for IBus input methods, 'xkb-options' for X options. * value is as you might expect I'll hopefully update the patch for this soon.
Any news on the patch, Stef ?
We could store the aa{ss} described in Comment 1, like the following: [InputSource0] key = value key1 = value1 key2 = value2 [InputSource1] ... What do you think?
Hey Rui, have two questions for you: a) If we were to "move" the input-sources setting storage to accountsservice (so it's available before the user authenticates). Other Desktops want this in accountsservice so they can have the user type their password in their own keyboard layout. b) Why did you choose aa(ss) instead of aa{ss} for storing the info? Anything in particular spring to mind. Can't find anything bout that here: https://bugzilla.gnome.org/show_bug.cgi?id=676101
(In reply to comment #4) > a) If we were to "move" the input-sources setting storage to > accountsservice (so > it's available before the user authenticates). Other Desktops want this > in > accountsservice so they can have the user type their password in their > own > keyboard layout. Yeah, this would be a good thing since right now, it's not guaranteed that the input source you use when you set your password is available in GDM. Think of setting the password with an AZERTY layout and then only having QWERTY available. Fixing that would require different code paths in g-s-d though since right now it just assumes that GDM's session is just like any other session[1] and thus has its own input sources settings. Instead, we'd need code to apply the selected user account's input sources when the user clicks one in GDM's chooser. [1] it's a bit more complicated than that but doesn't matter for this particular issue. > b) Why did you choose aa(ss) instead of aa{ss} for storing the info? > Anything > in particular spring to mind. Can't find anything bout that here: > > https://bugzilla.gnome.org/show_bug.cgi?id=676101 It's not aa(ss) but just a(ss). It's like that because it's really just a list or tuples of the form (type, identifier). You propose a list of dicts but why do you need a dict to describe an input source?
Created attachment 83044 [details] [review] Proposed input sources patch Adds input sources to the user object. The type used is 'a(ss)'.
I think that a(ss) isn't going to cut it. We already have problems with this in GNOME. Imagine I setup the following input options: - ibus pinyin - xkb english qwerty - xkb french azerty 'ibus pinyin', on its own, isn't sufficient information to know how to configure my input. GNOME actually has an existing bug that selecting this option from the keyboard indicator will get different results based on what the previously-selected xkb layout was. ie: if you are in azerty mode and you want to type pinyin-on-qwerty, then you would first have to switch to qwerty mode and *then* to pinyin. This will be an even bigger problem at the login screen. If the user wants pinyin-on-azerty then they'll be completely screwed unless they fancy changing their input source twice on every login (since I assume we'll be coming from qwerty to start with). This is why we decided in Nuremberg that we need the dictionary... We need to be able to specify xkb layout *and* input method, and maybe even xkb options on top of that as well... as a speaker of Esperanto, this one is actually quite important to me since the best way to type Esperanto is actually via an xkb option rather than a separate layout.
Created attachment 83265 [details] [review] Proposed input sources patch aa{ss} Adds input sources to the user object. The type is updated to 'aa{ss}' to allow for more generic definitions of input sources.
(In reply to comment #8) > Created attachment 83265 [details] [review] [review] > Proposed input sources patch aa{ss} > > Adds input sources to the user object. The type is updated to 'aa{ss}' to > allow for more generic definitions of input sources. While I agree with Ryan, a quick comment: As a nod to GNOME migration here, can we make this 'aa(ss)'? That way, GNOME can just use/set the first ss in the a(ss) array for now, until a bigger overhaul. This is inline with what Ryan talked about on IRC, that the protocol should be flexible even if not all options are exposed in the UI in a given implementation or version.
Comment on attachment 83265 [details] [review] Proposed input sources patch aa{ss} Review of attachment 83265 [details] [review]: ----------------------------------------------------------------- In general looks good. I would still suggest aa(ss) above for compatibility. ie: have a notion of which ss is 'first', rather than dict which in many DBus client implementations is treated as unordered (although I know it's not actually in GVariant or DBus itself). ::: src/libaccountsservice/act-user.c @@ +1324,5 @@ > + g_variant_ref (user->input_sources); > + g_object_notify (G_OBJECT (user), "input-sources"); > + } > + > + if (sources) Nitpick: I don't think sources would ever be null here. Obviously if a critical occurs during g_variant_get() then behavior is undefined. @@ +1586,5 @@ > + if (user->input_sources) > + g_variant_unref (user->input_sources); > + user->input_sources = user_to_copy->input_sources; > + if (user->input_sources) > + g_variant_ref (user->input_sources); Are we sure that user->input_sources is never the same GVariant as user_to_copy->input_sources? Because if we cannot guaratnee that, then this code will fail when the two point to the same GVariant. @@ +1704,5 @@ > +{ > + GError *error = NULL; > + > + g_return_if_fail (ACT_IS_USER (user)); > + g_return_if_fail (ACCOUNTS_IS_USER (user->accounts_proxy)); I think that we should fail early here if @sources in of the wrong variant type. ::: src/user.c @@ +328,5 @@ > > + if (user->input_sources != NULL) > + g_variant_unref (user->input_sources); > + user->input_sources = NULL; > + I'd like to suggest splitting this (and following code that loads InputSourceN groups) into its own function. @@ +444,4 @@ > if (user->language) > g_key_file_set_string (keyfile, "User", "Language", user->language); > > + if (user->input_sources) { Again suggest splitting this and following code out into its own function. @@ +2264,5 @@ > + case PROP_INPUT_SOURCES: > + user->input_sources = g_value_get_variant (value); > + > + if (user->input_sources) > + g_variant_ref (user->input_sources); This leaks if user->input_sources is already set.
Created attachment 83384 [details] [review] Proposed input sources patch aa{ss} Thanks for the review, Stef. I've fixed those changes, but I kept the type as aa{ss}. My reasoning is that this: [InputSource0] xkb=layout xkb-options=options should be interpreted the same as: [InputSource0] xkb-options=options xkb=layout and we leave the onus on whoever is doing the migration from a(ss) to aa{ss} to make sure it types properly. Since it's a change in protocol anyways, there's no real point in making sacrifices to ease the transition, imo. Thoughts?
(In reply to comment #11) > and we leave the onus on whoever is doing the migration from a(ss) to aa{ss} > to make sure it types properly. Since it's a change in protocol anyways, > there's no real point in making sacrifices to ease the transition, imo. > Thoughts? I'll prepare some patches for GNOME and let you know.
(In reply to comment #12) > I'll prepare some patches for GNOME and let you know. William, Ryan was saying you might be interested in patching gnome-control-center for this. Is the case? If so makes me very happy :)
One thing that is not clear to me is the policy with respect to which layout gets used by default at the login screen. My guess: the first one. Do we want to track changes in the layout between instances of the session and/or login screen? My guess: no.
I guess we should independently track each user's login layout between login sessions and each user's session layout between sessions. Mainly because the main thing the user does in the login session is type in their password, which will always be in one particular layout, and this may be different from their usual working layout. But I can only speculate though...
So it seems to me the status of this is: * The accountsservice patch here is ready * GNOME is not ready Ubuntu has been using the patch attached here for a couple years without anything blowing up. Plus they have a patch to gnome-settings-daemon to sync keyboard layout changes to accountsservice. That means the keyboard layout configuration is stored in two places: gnome-settings-daemon and accountsservice. This is probably necessary, though, because it allows unprivileged users to change their own keyboard layout, which wouldn't be possible via accountsservice. The biggest problem with the gnome-settings-daemon patch is simply that it hasn't (AFAIK) been proposed for upstream. I'll create a bug for that, since it needs to land shortly after the patch in this bug. Another problem with their g-s-d patch is that it syncs changes from g-s-d to accountsservice, but not vice-versa, but that's easy to fix. We also probably want to modify gdm to use the user's keyboard layout, like lightdm does, but that's really a separate issue.
(In reply to Michael Catanzaro from comment #16) > This is probably necessary, though, because it allows > unprivileged users to change their own keyboard layout, which wouldn't be > possible via accountsservice. This is incorrect; I didn't realize accountsservice has a separate polkit action for this case.
Comment on attachment 83384 [details] [review] Proposed input sources patch aa{ss} Review of attachment 83384 [details] [review]: ----------------------------------------------------------------- ::: src/libaccountsservice/act-user.c @@ +474,5 @@ > + "Input sources", > + "User's input sources.", > + G_VARIANT_TYPE ("aa{ss}"), > + NULL, > + G_PARAM_READABLE)); Also G_PARAM_STATIC_STRINGS, otherwise all these string literals will be unnecessarily strduped. I know this file is missing that everywhere, but it will probably be fixed before this patch gets accepted. @@ +1073,5 @@ > + * Returns the input sources of @user. > + * > + * Returns: (transfer none): a list of input sources > + */ > +GVariant * Should return a const GVariant, like act_user_get_login_history(). @@ +1317,5 @@ > + > + if (!user->input_sources || !g_variant_equal (sources, user->input_sources)) { > + if (user->input_sources) > + g_variant_unref (user->input_sources); > + user->input_sources = g_variant_ref (sources); Shame this is necessary. Maybe it's time to add g_set_variant() to glib? ::: src/libaccountsservice/act-user.h @@ +77,4 @@ > gboolean act_user_is_nonexistent (ActUser *user); > const char *act_user_get_icon_file (ActUser *user); > const char *act_user_get_language (ActUser *user); > +GVariant *act_user_get_input_sources (ActUser *user); Ditto. ::: src/user.c @@ +2472,4 @@ > iface->handle_set_home_directory = user_set_home_directory; > iface->handle_set_icon_file = user_set_icon_file; > iface->handle_set_language = user_set_language; > + iface->handle_set_input_sources = user_set_input_sources; Should probably go one line higher up -- for whatever reason, this list is sorted differently from all the others (alphabetically).
(In reply to Michael Catanzaro from comment #18) > @@ +1073,5 @@ > > + * Returns the input sources of @user. > > + * > > + * Returns: (transfer none): a list of input sources > > + */ > > +GVariant * > > Should return a const GVariant, like act_user_get_login_history(). Tried this, bad idea, since none of the GVariant functions take const GVariants. I've been spending too much time in C++.... Probably act_user_get_login_history() was a mistake.
-- GitLab Migration Automatic Message -- This bug has been migrated to freedesktop.org's GitLab instance and has been closed from further activity. You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.freedesktop.org/accountsservice/accountsservice/issues/18.
Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.