Created attachment 213814 [details] [review]
express,fedora: Fetch kbd layout info from system

Fetch keyboard layout from system settings, not user settings. Also move
keyboard layout handling to FedoraInstaller class as Windows doesn't
seem to use this information.

The system setting may not be related at all with the current keyboard layout the user has in their GNOME session, imo this should be mentioned in the commit log and in the comment explaining why this  file is being parsed

(In reply to comment #2)
> The system setting may not be related at all with the current keyboard layout
> the user has in their GNOME session, imo this should be mentioned in the commit
> log and in the comment explaining why this  file is being parsed

I thought I did that already.

I reread the log and the diff, and what I read doesn't really hint that it's far from being a perfect fix (we may not be setting the correct layout at all), but that doing this at least makes sure we are not feeding an incorrect value to anaconda.

Created attachment 213820 [details] [review]
express,fedora: Fetch kbd layout info from system

This version makes use of 'org.freedesktop.locale1' system service in attempt to be less Fedora-specific and to have much simpler implementation.

(In reply to comment #4)
> I reread the log and the diff, and what I read doesn't really hint that it's
> far from being a perfect fix (we may not be setting the correct layout at all),
> but that doing this at least makes sure we are not feeding an incorrect value
> to anaconda.

I think it would make sense to add option to anaconda to be able to set X keyboard layout(s) from kickstart file.

Moving the whole keyboard code from the generic code to fedora is a bad move imo. We will need something similar for Debian, openSUSE, ...
Windows could use it too through an InputLocale node in the Unattend.xml node, but it obviously wants it in a different format, either en-US or 0409:00000409 (yum!) (http://technet.microsoft.com/fr-fr/library/cc748926%28v=ws.10%29.aspx)
I think getting the keyboard layout setting should be kept in the generic code, and subclasses would have a virtual method which maps this value to the format expected by the specific unattended installer.
Until we are able to do that properly for the fedora unattended installer, an additional patch could read and use the value from the system, and we'll have to cross fingers hoping that the distro we are running on uses a value anaconda accepts...

(In reply to comment #7)
> Moving the whole keyboard code from the generic code to fedora is a bad move
> imo. We will need something similar for Debian, openSUSE, ...

When we have those, I promise we'll also have a generic linux class. Pretty sure these classes wont even exist before that could happen as hopefully soon we start using the new libosinfo APIs.

> Windows could use it too through an InputLocale node in the Unattend.xml node,
> but it obviously wants it in a different format, either en-US or 0409:00000409
> (yum!) (http://technet.microsoft.com/fr-fr/library/cc748926%28v=ws.10%29.aspx)
> I think getting the keyboard layout setting should be kept in the generic code,
> and subclasses would have a virtual method which maps this value to the format
> expected by the specific unattended installer.
> Until we are able to do that properly for the fedora unattended installer, an
> additional patch could read and use the value from the system, and we'll have
> to cross fingers hoping that the distro we are running on uses a value anaconda
> accepts...

? If we get a wrong value, its a distro bug. If anaconda doesn't accept even this string, thats definitely an anaconda bug this time.

(In reply to comment #8)
> 
> ? If we get a wrong value, its a distro bug. If anaconda doesn't accept even
> this string, thats definitely an anaconda bug this time.

I can imagine new keymaps being added in newer distros that won't work with older systems, distros being creative in the format they use for the system keymap value, ...

(In reply to comment #9)
> (In reply to comment #8)
> > 
> > ? If we get a wrong value, its a distro bug. If anaconda doesn't accept even
> > this string, thats definitely an anaconda bug this time.
> 
> I can imagine new keymaps being added in newer distros that won't work with
> older systems, distros being creative in the format they use for the system
> keymap value, ...

We'll see. Anaconda has already been patched so it doesn't crash but now does what it always does when it doesn't accept some value: warn and ask for that information. So no biggie if that happens. Also I imagine people trying out/using newer (than host OS) OSs in VMs so that would make this even lesser possibility. Also, is it very likely that new distros will add new keymaps before Fedora does, especially within next few months?

In short, unlikely and not a very worrying scenario if it happens. I might be  wrong about former but latter is a fact.

(In reply to comment #10)
> In short, unlikely and not a very worrying scenario if it happens.

Just anticipating the crazy bugs we'll get from using some distros, that's it, not saying we shouldn't take this patch because of this.
I'm more interested in the commit log/comment being more explicit about this change being a temporary not-perfect workaround..

(In reply to comment #11)
> (In reply to comment #10)
> > In short, unlikely and not a very worrying scenario if it happens.
> 
> Just anticipating the crazy bugs we'll get from using some distros, that's it,
> not saying we shouldn't take this patch because of this.
> I'm more interested in the commit log/comment being more explicit about this
> change being a temporary not-perfect workaround..

OK, sometimes I'm not good with words so tell me what you want there and I'll put it. :)

You can append something like this to the commit log:
 "If the user configured a layout different from the default system one in their X session preferences, using the system layout won't do the right thing. However, doing this avoids a bad anaconda crash during unattended installs. The reason is that anaconda expects a linux console keymap name as the keyboard to use, and X layout names (which is what was passed to anaconda until now) are not necessarily valid linux keymaps names, causing anaconda to choke on them."

(In reply to comment #13)
> You can append something like this to the commit log:
>  "If the user configured a layout different from the default system one in
> their X session preferences, using the system layout won't do the right thing.
> However, doing this avoids a bad anaconda crash during unattended installs. The
> reason is that anaconda expects a linux console keymap name as the keyboard to
> use, and X layout names (which is what was passed to anaconda until now) are
> not necessarily valid linux keymaps names, causing anaconda to choke on them."

Thanks, now that I understand what you meant. I'd write something like this:

"It would be really nice to be able to configure X keyboard layout of the guest based on the hosts' X keyboard layout as well but currently Anaconda doesn't support that and we can only specify system/console layout so we do that.".

I don't see the point of blaming anaconda while instead of this patch, we could go the (longer but better) way of mapping x layouts back to console layouts, which would make anaconda happy and would do the right thing from the user point of view.

(In reply to comment #15)
> I don't see the point of blaming anaconda while instead of this patch, we could
> go the (longer but better) way of mapping x layouts back to console layouts,
> which would make anaconda happy and would do the right thing from the user
> point of view.

I'm not blaming, just saying where I think this problem should be solved. Having a local map also sounds like a work around to me.

(In reply to comment #16)
> (In reply to comment #15)
> > I don't see the point of blaming anaconda while instead of this patch, we could
> > go the (longer but better) way of mapping x layouts back to console layouts,
> > which would make anaconda happy and would do the right thing from the user
> > point of view.
> 
> I'm not blaming, just saying where I think this problem should be solved.
> Having a local map also sounds like a work around to me.

But seems like a better work-around and /usr/lib/python2.7/site-packages/system_config_keyboard/keyboard_models.py seems to already provide the mapping (just in the opposite direction) so I'll make another patch that uses the mapping instead..

Created attachment 213901 [details] [review]
express,fedora: Translate X kbd layout ID to console

Translate X kbd layout ID to console layout ID as Anaconda expects the
latter in the kickstart file.

Also move keyboard layout handling to FedoraInstaller class as Windows
doesn't seem to use this information. We'll probably need layout
handling in express installation of other Linux distros but:

1. Just like in case of Windows, we'll add a generic installer media
base class for Linux when we add support for any other distro and this
code will move there.

2. We'll most probably be removing most of this code anyways in favor of
new libosinfo API before adding support for other distros:

https://www.redhat.com/archives/virt-tools-list/2012-February/msg00236.html
http://www.google-melange.com/gsoc/project/google/gsoc2012/fidencio/41002

Created attachment 213902 [details] [review]
express,fedora: Translate X kbd layout ID to console

Translate X kbd layout ID to console layout ID as Anaconda expects the
latter in the kickstart file.

Also move keyboard layout handling to FedoraInstaller class as Windows
doesn't seem to use this information. We'll probably need layout
handling in express installation of other Linux distros but:

1. Just like in case of Windows, we'll add a generic installer media
base class for Linux when we add support for any other distro and this
code will move there.

2. We'll most probably be removing most of this code anyways in favor of
new libosinfo API before adding support for other distros:

https://www.redhat.com/archives/virt-tools-list/2012-February/msg00236.html
http://www.google-melange.com/gsoc/project/google/gsoc2012/fidencio/41002

Review of attachment 213902 [details] [review]:

Patch looks good overall, just a few random comments, but probably nothing to change now
* is there a nice way to initialize a GHastTable in vala? This would be better than the array of array used to translate the keymaps
* I'm still not convinced moving all this keyboard handling code to the fedora specific class is the right way forward, but this can easily be changed afterwards
* there is still one small issue with the way we use /org/gnome/libgnomekbd/keyboard/layouts in that the first item in the list is not necessarily the active layout, but I have no idea how to get the currently active one
* some layouts are probably missing in there, but we can add them later on
* and I want to check a few more things on Monday, that's why I'm not touching the Patch Status for now ;) (I suspect we want to match (XkbLayout, XkbVariant) => console keymap to be more accurate instead of just XkbLayout => keymap)

(In reply to comment #20)
> Review of attachment 213902 [details] [review]:
> 
> Patch looks good overall, just a few random comments, but probably nothing to
> change now
> * is there a nice way to initialize a GHastTable in vala?
> This would be better
> than the array of array used to translate the keymaps

I totally agree and I asked around on #vala but seems there isn't any way to do that right now. :(

> * I'm still not convinced moving all this keyboard handling code to the fedora
> specific class is the right way forward, but this can easily be changed
> afterwards

Well, I did provide arguments in the commit log so I tried. :)

> * there is still one small issue with the way we use
> /org/gnome/libgnomekbd/keyboard/layouts in that the first item in the list is
> not necessarily the active layout, but I have no idea how to get the currently
> active one

Ah yes, I didn't think of that though it does pick-up your primary selection so its not that bad.

> * some layouts are probably missing in there, but we can add them later on

I might have accidently removed some needed ones while removing duplicates.

(In reply to comment #21)
> (In reply to comment #20)

> > * there is still one small issue with the way we use
> > /org/gnome/libgnomekbd/keyboard/layouts in that the first item in the list is
> > not necessarily the active layout, but I have no idea how to get the currently
> > active one
> 
> Ah yes, I didn't think of that though it does pick-up your primary selection so
> its not that bad.

Agreed, filed bug #676017 for that.

Created attachment 214019 [details] [review]
express,fedora: Translate X kbd layout ID to console

This version takes variants into acount is therefore more accurate. Also, I introduced a private struct to represent keyboard layouts to improve readability.

Comment on attachment 214019 [details] [review]
express,fedora: Translate X kbd layout ID to console

>+
>+    private struct KbdLayout {
>+        public string x;
>+        public string variant;

I'd prefer xkb_layout and xkb_variant, xkb is confusing enough, better to make the naming explicit wrt what it refers to

>+        public string console;
>+    }
>+
>+    private string fetch_console_kbd_layout () {
>+        var settings = new GLib.Settings ("org.gnome.libgnomekbd.keyboard");
>+        var layouts = settings.get_strv ("layouts");
>+        var layout_str = layouts[0];
>+        if (layout_str == null || layout_str == "") {
>+            warning ("Failed to fetch prefered keyboard layout from user settings. Using 'us'..");
>+
>+            return "us";
>+        }
>+
>+        var tokens = layout_str.split("\t");

The block below...

>+        KbdLayout layout = { tokens[0], tokens[1], "us" };
>+
>+        for (var i = 0; i < kbd_layouts.length; i++)
>+           if (layout.x == kbd_layouts[i].x && layout.variant == kbd_layouts[i].variant) {
>+               layout.console = kbd_layouts[i].console;
>+
>+               break;
>+           }
>+
>+        debug ("Using '%s' keyboard layout.", layout.console);
>+
>+        return layout.console;

... is a bit too much magic in my opinion.

var xkb_layout = tokens[0];
var xkb_variant = tokens[1];
var console = null;

for (....) {
...
}

if (console != null) {
  debug ("Using '%s'....)
  return console
else
  debug ("Couldn't find a console keymap for '%s', falling back to a 'us' keymap");
  return "us"

would be more readable I think
We could also have a smarter fallback by trying to match only the layout and ignore the variant when falling back, and only falling back to "us" when that fails.

Created attachment 214020 [details] [review]
express,fedora: Translate X kbd layout ID to console

This one is much less magical. :)

Comment on attachment 214020 [details] [review]
express,fedora: Translate X kbd layout ID to console

>+        for (var i = 0; i < kbd_layouts.length; i++)
>+           if (xkb_layout == kbd_layouts[i].xkb_layout && xkb_variant == kbd_layouts[i].xkb_variant) {
>+               console_layout = kbd_layouts[i].console_layout;
>+
>+               break;
>+           }

I think a
/* in case we don't have an exact (XkbLayout, XkbVariant) match,
 * fallback to one of the layouts matching the XkbLayout alone, it
 * will hopefully be a better choice than raw "us"
 */
if ((!console_layout) && (xkb_layout == kbd_layout[i]))
    console_layout = kbd_layout[i].console_layout 

at this place inside the for() loop would add the smart fallback I aluded too in the previous comment. But I understand if you want to split this in a separate patch that needs more testing before being merged 
Patch looks good otherwise.



>+
>+        if (console_layout == null) {
>+            debug ("Couldn't find a console layout for X layout '%s', falling back to 'us'..", layout_str);
>+            console_layout = "us";
>+        }
>+        debug ("Using '%s' keyboard layout.", console_layout);
>+
>+        return console_layout;
>+    }
>+
>+    // Modified copy of KeyboardModels._modelDict from system-config-keyboard project:
>+    // https://fedorahosted.org/system-config-keyboard/
>+    //
>+    private const KbdLayout[] kbd_layouts = {
>+        { "ara", null, "ar-azerty" },
>+        { "ara", "azerty", "ar-azerty" },
>+        { "ara", "azerty_digits", "ar-azerty-digits" },
>+        { "ara", "digits", "ar-digits" },
>+        { "ara", "qwerty", "ar-qwerty" },
>+        { "ara", "qwerty_digits", "ar-qwerty-digits" },
>+        { "be", null, "be-latin1" },
>+        { "bg", null, "bg_bds-utf8" },
>+        { "bg", "phonetic", "bg_pho-utf8" },
>+        { "bg", "bas_phonetic", "bg_pho-utf8" },
>+        { "br", null, "br-abnt2" },
>+        { "ca(fr)", null, "cf" },
>+        { "hr", null, "croat" },
>+        { "cz", null, "cz-us-qwertz" },
>+        { "cz", "qwerty", "cz-lat2" },
>+        { "cz", "", "cz-us-qwertz" },
>+        { "de", null, "de" },
>+        { "de", "nodeadkeys", "de-latin1-nodeadkeys" },
>+        { "dev", null, "dev" },
>+        { "dk", null, "dk" },
>+        { "dk", "dvorak", "dk-dvorak" },
>+        { "es", null, "es" },
>+        { "ee", null, "et" },
>+        { "fi", null, "fi" },
>+        { "fr", null, "fr" },
>+        { "fr", "latin9", "fr-latin9" },
>+        { "gr", null, "gr" },
>+        { "gur", null, "gur" },
>+        { "hu", null, "hu" },
>+        { "hu", "qwerty", "hu101" },
>+        { "ie", null, "ie" },
>+        { "in", null, "us" },
>+        { "in", "ben", "ben" },
>+        { "in", "ben-probhat", "ben_probhat" },
>+        { "in", "guj", "guj" },
>+        { "in", "tam", "tml-inscript" },
>+        { "in", "tam_TAB", "tml-uni" },
>+        { "is", null, "is-latin1" },
>+        { "it", null, "it" },
>+        { "jp", null, "jp106" },
>+        { "kr", null, "ko" },
>+        { "latam", null, "la-latin1" },
>+        { "mkd", null, "mk-utf" },
>+        { "nl", null, "nl" },
>+        { "no", null, "no" },
>+        { "pl", null, "pl2" },
>+        { "pt", null, "pt-latin1" },
>+        { "ro", null, "ro" },
>+        { "ro", "std", "ro-std" },
>+        { "ro", "cedilla", "ro-cedilla" },
>+        { "ro", "std_cedilla", "ro-std-cedilla" },
>+        { "ru", null, "ru" },
>+        { "rs", null, "sr-cy" },
>+        { "rs", "latin", "sr-latin"},
>+        { "se", null, "sv-latin1" },
>+        { "ch", "de_nodeadkeys", "sg" },
>+        { "ch", "fr", "fr_CH" },
>+        { "sk", null, "sk-qwerty" },
>+        { "si", null, "slovene" },
>+        { "tj", null, "tj" },
>+        { "tr", null, "trq" },
>+        { "gb", null, "uk" },
>+        { "ua", null, "ua-utf" },
>+        { "us", null, "us" },
>+        { "us", "intl", "us-acentos" }
>+    };
> }
>diff --git a/src/unattended-installer.vala b/src/unattended-installer.vala
>index 0f4ada8..4aa1962 100644
>--- a/src/unattended-installer.vala
>+++ b/src/unattended-installer.vala
>@@ -45,13 +45,11 @@ private abstract class Boxes.UnattendedInstaller: InstallerMedia {
>     protected Gtk.Entry password_entry;
> 
>     protected string timezone;
>-    protected string kbd;
>     protected string lang;
> 
>     private static Regex username_regex;
>     private static Regex password_regex;
>     private static Regex timezone_regex;
>-    private static Regex kbd_regex;
>     private static Regex lang_regex;
>     private static Fdo.Accounts accounts;
> 
>@@ -60,7 +58,6 @@ private abstract class Boxes.UnattendedInstaller: InstallerMedia {
>             username_regex = new Regex ("BOXES_USERNAME");
>             password_regex = new Regex ("BOXES_PASSWORD");
>             timezone_regex = new Regex ("BOXES_TZ");
>-            kbd_regex = new Regex ("BOXES_KBD");
>             lang_regex = new Regex ("BOXES_LANG");
>             try {
>                 accounts = Bus.get_proxy_sync (BusType.SYSTEM, "org.freedesktop.Accounts", "/org/freedesktop/Accounts");
>@@ -93,10 +90,6 @@ private abstract class Boxes.UnattendedInstaller: InstallerMedia {
>         var date = new DateTime.from_timeval_local (time);
>         timezone = date.get_timezone_abbreviation ();
> 
>-        var settings = new GLib.Settings ("org.gnome.libgnomekbd.keyboard");
>-        var layouts = settings.get_strv ("layouts");
>-        kbd = layouts[0] ?? "us";
>-
>         var langs = Intl.get_language_names ();
>         lang = langs[0];
> 
>@@ -246,7 +239,6 @@ private abstract class Boxes.UnattendedInstaller: InstallerMedia {
>         var str = username_regex.replace (data, data.length, 0, username_entry.text);
>         str = password_regex.replace (str, str.length, 0, password);
>         str = timezone_regex.replace (str, str.length, 0, timezone);
>-        str = kbd_regex.replace (str, str.length, 0, kbd);
>         str = lang_regex.replace (str, str.length, 0, lang);
> 
>         return str;
>-- 
>1.7.7.6

Created attachment 214030 [details] [review]
express,fedora: Translate X kbd layout ID to console

This version should do that.

Attachment 214030 [details] pushed as b3a4ef5 - express,fedora: Translate X kbd layout ID to console