GNOME Bugzilla – Bug 794166
[RFE] add default input method when skip the keyboard page
Last modified: 2018-04-03 01:52:33 UTC
For CJK users, some input method need to be added in the keyboard page. But now gnome-initial-setup may skip the keyboard page, we propose to add the default input method for some languages when user finish selection.
Created attachment 369423 [details] [review] keyboard: add default input method when skipped keyboard page When gnome-initial-setup skip the keyboard page, add default input method based on locale when user finish selection.
The above patch only provided to explain the idea, the code may not run.
Created attachment 369563 [details] [review] keyboard: add default input method when skipped keyboard page When gnome-initial-setup skip the keyboard page, add default input method based on locale when user finish selection.
Review of attachment 369563 [details] [review]: ::: gnome-initial-setup/pages/summary/gis-summary-page.c @@ +270,3 @@ done_cb (GtkButton *button, GisSummaryPage *page) { + /* if skip the language page, we setup the default input method here. */ comment refers wrongly to language, when the code uses "keyboard"
Review of attachment 369563 [details] [review]: It might be slightly nicer to give pages a general way to act when they are skipped, like a skip() function they can implemented. That would let us keep all this code where it belongs, in the keyboard page. ::: gnome-initial-setup/pages/summary/gis-summary-page.c @@ +218,3 @@ +gchar ** +pages_to_skip_from_file (void); this should probably be in a header
Review of attachment 369563 [details] [review]: I actually really like this approach, but I think you're doing it in the wrong place. Surely input method should be configured sooner than the summary page. It should available for input in gnome-initial-setup itself, right? I would handle this in gnome-initial-setup.c instead, that way you have access to pages_to_skip(). Alternatively, we could give pages a function to be called if they are skipped, and the work could be done there. That might be even better. ::: gnome-initial-setup/gnome-initial-setup.c @@ +101,3 @@ } +gchar ** This isn't good; this is supposed to be a file-private function. ::: gnome-initial-setup/pages/summary/gis-summary-page.c @@ +232,3 @@ + GVariantBuilder builder; + GSettings *input_settings; + GVariant * sources; GVariant *sources;
Created attachment 369939 [details] [review] page: add skip function for all pages If some page is skipped, configure the default options for the skipped page.
The draft patch add gis_page_skip function to GisPage. And split gis_driver_add_page function call from gis_*_prepare_page: * for normal page, add the page by gis_driver_add_page * for skipped page, skip the page and destroy the page BTW, the patch just pass compile, will test it soon.
Review of attachment 369939 [details] [review]: I like this approach! ::: gnome-initial-setup/gnome-initial-setup.c @@ +160,3 @@ + + if (skipped) { + page = page_data->prepare_page_func (driver); Do you really need to call the prepare_page_func when the page is skipped? Probably not? @@ +225,1 @@ if (should_skip_page (driver, page_data->page_id, skip_pages)) In this case, you can call gis_page_skip(), and otherwise you can call page_data->prepare_page_func(). Then you can get rid of configure_skipped_pages(). @@ +293,3 @@ driver = gis_driver_new (mode); + /* when some pages is skipped, + * we configure the default option for the skipped pages. I'd remove this comment; it isn't helpful.
Thanks for the comments! I think the patch needs to call prepare_page_func to get the page object, then call gis_page_skip on the page. The skip_pages just contains strings, it doesn't contain the page objects. Is it possible to create the page object from the string?
(In reply to Peng Wu from comment #10) > Thanks for the comments! > > I think the patch needs to call prepare_page_func to get the page object, > then call gis_page_skip on the page. OK, yes. You can still get rid of configure_skipped_pages(), though, right?
Created attachment 370034 [details] [review] page: add skip function for all pages If some page is skipped, configure the default options for the skipped page.
Okay, configure_skipped_pages is removed.
Review of attachment 370034 [details] [review]: Looks good! ::: gnome-initial-setup/gnome-initial-setup.c @@ +192,1 @@ if (should_skip_page (driver, page_data->page_id, skip_pages)) Do it in one condition: if ((page_data->new_user_only && !is_new_user) || (should_skip_page (driver, page_data->page_id, skip_pages)) ::: gnome-initial-setup/pages/account/gis-account-pages.c @@ +27,3 @@ gis_prepare_account_page (GisDriver *driver) { + GisPage *page; You don't need the local variable in most of these prep functions. Much simpler to just: return g_object_new (...); ::: gnome-initial-setup/pages/keyboard/gis-keyboard-page.c @@ +213,3 @@ +static gboolean +gis_keyboard_page_skip (GisPage *page) The space in the parameter list should be appropriate for the size of the longest data type: static gboolean gis_keyboard_page_skip (GisPage *page)
(In reply to Peng Wu from comment #8) > BTW, the patch just pass compile, will test it soon. Have you tested it now? I've just looked over the code; I haven't tested it yet.
BTW you might be interested in this other input methods issue: bug #776189
Created attachment 370137 [details] [review] page: add skip function for all pages If some page is skipped, configure the default options for the skipped page.
I just found after delete the first user, gdm greeter will run gnome-initial-setup again. Sorry, I will test the patch soon.
I'm testing the patch now. First thing I notice is that it crashes on start, because gis_prepare_software_page() returns uninitialized memory. You need to adjust the #ifdefs to ensure it returns NULL instead, like this: GisPage * gis_prepare_software_page (GisDriver *driver) { GSettingsSchemaSource *source; GSettingsSchema *schema; GisPage *page = NULL; #ifdef ENABLE_SOFTWARE_SOURCES source = g_settings_schema_source_get_default (); schema = g_settings_schema_source_lookup (source, "org.gnome.software", TRUE); if (schema != NULL && g_settings_schema_has_key (schema, "show-nonfree-software")) page = g_object_new (GIS_TYPE_SOFTWARE_PAGE, "driver", driver, NULL); if (schema != NULL) g_settings_schema_unref (schema); #endif return page; }
Next problem is some ibus-related criticals and then a crash. First critical is IBUS-CRITICAL **: ibus_bus_list_engines_async: assertion 'IBUS_IS_BUS (bus)' failed Here's a backtrace I took with G_DEBUG=fatal-criticals:
+ Trace 238508
Looks like priv->ibus is either NULL or otherwise-invalid. The code looks safe to me, though; I don't see how this could happen.
It's because CcInputChooser isn't prepared to handle being destroyed immediately, it fails to disconnect the fetch_ibus_engines callback. I can fix it in cc_input_chooser_constructed by changing: g_signal_connect_swapped (priv->ibus, "connected", G_CALLBACK (fetch_ibus_engines), chooser); to: g_signal_connect_object (priv->ibus, "connected", G_CALLBACK (fetch_ibus_engines), chooser, G_CONNECT_SWAPPED); Then there is a third crash, this time in CcInputChooser. Looks like again a problem with being unprepared for the widget to be destroyed immediately.
+ Trace 238509
I'll leave this one to you to debug further... suffice to say the patch does need to be tested. :)
Created attachment 370327 [details] [review] page: add skip function for all pages If some page is skipped, configure the default options for the skipped page.
Actually the patch delayed the page destroy, and it works now. And I add one keyboard layout and optional ibus input method when skip the keyboard page. Please review it again, thanks!
Review of attachment 370327 [details] [review]: Awesome, just a couple minor nits left to fix before this will be ready to commit. I see you also fixed https://bugzilla.redhat.com/show_bug.cgi?id=1561864, which I didn't even realize was a problem until yesterday. Thanks! I'll add it to Fedora now, for testing. Importantly: gis-keyboard-page.c uses eight-space tabs for indentation, but your patch mostly uses four spaces of indentation. Please change that to consistent with the existing code. ::: gnome-initial-setup/pages/keyboard/gis-keyboard-page.c @@ +213,3 @@ +static GSList * +get_localed_input (GDBusProxy *proxy) It looks like you've left load_localed_input() unchanged and are duplicating a bunch of code here, right? The only difference seems to be that you return the list of sources rather than saving it into priv->system_sources. You can avoid the code duplication by just moving that last bit of code at the bottom of the function to the bottom of localed_proxy_ready(), right after the call to load_localed_input(). Then you wouldn't need both functions. @@ +223,3 @@ + GSList *sources = NULL; + + if (!proxy) This isn't needed, the function should never be called if proxy is NULL. @@ +300,3 @@ + } + + g_settings_delay (input_settings); Not many developers would think to use delay/apply here, nice touch! @@ +335,3 @@ +{ + g_dbus_proxy_new_for_bus (G_BUS_TYPE_SYSTEM, + G_DBUS_PROXY_FLAGS_GET_INVALIDATED_PROPERTIES, This is kinda weird, why is G_DBUS_PROXY_FLAGS_GET_INVALIDATED_PROPERTIES needed? You can also use G_DBUS_PROXY_FLAGS_DO_NOT_CONNECT_SIGNALS here. @@ +340,3 @@ + "/org/freedesktop/locale1", + "org.freedesktop.locale1", + NULL, Pass priv->cancellable here or the operation won't get cancelled when the page is destroyed, and you'll wind up using it after it has been freed at the bottom of skip_proxy_ready. @@ +350,3 @@ +{ + GisKeyboardPagePrivate *priv = gis_keyboard_page_get_instance_private (self); + GSList * sources = get_localed_input (priv->localed); GSList *sources
Review of attachment 370327 [details] [review]: ::: gnome-initial-setup/gis-page.h @@ +61,3 @@ void (*save_data) (GisPage *page); void (*shown) (GisPage *page); + gboolean (*skip) (GisPage *page); Also: I notice the boolean return value is ultimately unused, so the skip functions should all return void instead.
Review of attachment 370327 [details] [review]: ::: gnome-initial-setup/gnome-initial-setup.c @@ +199,3 @@ + if (skipped) { + gis_page_skip (page); + g_ptr_array_add (skipped_pages, page); Aaaah, here is one more minor problem. rebuild_pages_cb() can be called many times (once on startup and once for every locale change), so you could wind up adding the same pages here again and again. You should do a g_ptr_array_free (skipped_pages, TRUE) at the top of the callback to avoid this problem.
Review of attachment 370327 [details] [review]: ::: gnome-initial-setup/gnome-initial-setup.c @@ +255,3 @@ #endif + skipped_pages = g_ptr_array_new_with_free_func (gtk_widget_destroy); And I notice a compiler warning here, you are missing a cast: skipped_pages = g_ptr_array_new_with_free_func ((GDestroyNotify) gtk_widget_destroy);
Review of attachment 370327 [details] [review]: ::: gnome-initial-setup/pages/keyboard/gis-keyboard-page.c @@ +295,3 @@ + locales = g_get_language_names (); + language = locales[0]; + if (gnome_get_input_source_from_locale (language, &type, &id)) { Question: we already added the default keyboard layout, without an input method, a couple lines up above. So if we do wind up adding an input method here, it will be as a second input source. I tested this by running: $ sudo localectl set-locale LANG=ja_JP.utf8 $ sudo localectl set-keymap jp $ sudo localectl set-x11-keymap jp to simulate Anaconda setting the system keyboard layout, then I ran (from the gnome-initial-setup source root) $ LANG=ja_JP.utf8 gnome-initial-setup/gnome-initial-setup The resulting input source list was: [('xkb', 'jp'), ('ibus', 'kkc')] Is this really the desired result? The default input source ('xkb', 'jp') cannot be used to input Japanese characters, and you have to switch to the second input source ('ibus', 'kkc') to do so, right? Wouldn't a better result be to have only one input source configured: [('ibus', 'kkc')] ? Or is having two input sources desirable and expected? Related to this, there is bug #776189 for the case where the keyboard page is not skipped. Since you're now the expert on this code, can you help with that too, to ensure we get the desired behavior? It looks closely-related to the code you've written here.
(In reply to Michael Catanzaro from comment #25) > Review of attachment 370327 [details] [review] [review]: > > ::: gnome-initial-setup/gis-page.h > @@ +61,3 @@ > void (*save_data) (GisPage *page); > void (*shown) (GisPage *page); > + gboolean (*skip) (GisPage *page); > > Also: I notice the boolean return value is ultimately unused, so the skip > functions should all return void instead. Turns out having the boolean return is really useful for me in bug #793501, so might as well leave it unchanged.
Created attachment 370432 [details] [review] page: add skip function for all pages If some page is skipped, configure the default options for the skipped page.
I updated the load_localed_input function. > This is kinda weird, why is G_DBUS_PROXY_FLAGS_GET_INVALIDATED_PROPERTIES needed? I dunno whether locale changes will invalidate the properties, just copy it here from old code. It seems invalidated property will need to receive signals, BICBW. > The resulting input source list was: > > [('xkb', 'jp'), ('ibus', 'kkc')] I think most fields in gnome-initial-setup will use just keyboard layout, only user name or some fields will use ibus input source. Normally we will configure both keyboard layout and ibus input method for CJK users. The user can remove unneeded input sources from control panel, I think. > [('ibus', 'kkc')] > > ? Or is having two input sources desirable and expected? Yes, having two input sources is expected, and keyboard layout is used first in gnome-initial-setup. Please review it again, thanks! BTW, I will be on holiday from Wednesday to Saturday.
Sure, I will look at bug #776189.
Attachment 370432 [details] pushed as 75f43d3 - page: add skip function for all pages
Approved, thanks!
Thanks for the review!