GNOME Bugzilla – Bug 703392
Changing language from user settings for a user will not ask for restart
Last modified: 2015-01-19 12:15:21 UTC
User can notice that changing the language from settings - user, system will not ask for restart as it does changing from region & language. Steps : 1. Boot system 2. Go to Region & language and change the language 3. Observe system asks for restart 4. Go to settings - users 5. Change the language for the current user 6. Observe user is not asked for system restart as it does in region & language Expected outcome: System prompts user for restart when changing the language from user - settings. Actual outcome: System does not prompts user for restart, user us confused.
Created attachment 250537 [details] [review] add restart notification
Created attachment 250538 [details] notification screenshot
Review of attachment 250537 [details] [review]: I haven't tested this, but looks good to me, with the following fix. ::: panels/user-accounts/um-user-panel.c @@ +847,3 @@ + gd_notification_dismiss (GD_NOTIFICATION (d->notification)); + + bus = g_bus_get_sync (G_BUS_TYPE_SESSION, NULL, NULL); |bus| leaks.
Created attachment 251460 [details] [review] add restart notification (In reply to comment #3) > Review of attachment 250537 [details] [review]: > > I haven't tested this, but looks good to me, with the following fix. > > ::: panels/user-accounts/um-user-panel.c > @@ +847,3 @@ > + gd_notification_dismiss (GD_NOTIFICATION (d->notification)); > + > + bus = g_bus_get_sync (G_BUS_TYPE_SESSION, NULL, NULL); > > |bus| leaks. I don't know what following fix have you in mind, however I've changed the patch according the region panel to be using GDBusProxy.
Review of attachment 251460 [details] [review]: A simple g_object_unref(bus) after g_dbus_connection_call() would have done the trick, no? The async style patch has a few issues with corner cases, so just doing that would be simpler. ::: panels/user-accounts/um-user-panel.c @@ +847,3 @@ + gd_notification_dismiss (GD_NOTIFICATION (d->notification)); + + g_dbus_proxy_call (d->session, Race condition if d->session is not yet set. @@ +1523,3 @@ + NULL, + session_proxy_ready, + NULL, Will crash if user panel is destroyed before session_proxy_ready is called, (eg: if the user clicks the back button to go to the main overview of g-c-c). If you really want to do this, you should hold a ref to self as a closure.
Comment on attachment 250537 [details] [review] add restart notification Will commit this patch, with leak fix.
Get these WARNING: (gnome-control-center:30914): Gtk-WARNING **: GtkComboBox 0x1444440 is mapped but not child_visible (gnome-control-center:30914): Gtk-WARNING **: GtkComboBox 0x1444440 is mapped but visible=1 child_visible=0 parent GtkNotebook 0x1790e20 mapped=1 (gnome-control-center:30914): Gtk-WARNING **: GtkComboBox 0x1444440 is mapped but not child_visible (gnome-control-center:30914): Gtk-WARNING **: GtkComboBox 0x1444440 is mapped but visible=1 child_visible=0 parent GtkNotebook 0x1790e20 mapped=1
Created attachment 255924 [details] [review] add restart notification The commit hasn't been committed, so attaching fixed version: - leak fix mentioned in the review - show the restart message with new locale - show the notification only for active user
(In reply to comment #7) > Get these WARNING: These warnings aren't connected with this patch, they have been there long time. They comes from UmEditableCombo, but don't know how to fix them...
Now the warnings are fixed, could you review the attached patch please?
Review of attachment 255924 [details] [review]: Looks fine after those issues are fixed. ::: panels/user-accounts/um-user-panel.c @@ +933,3 @@ + if (self_selected) { + show_restart_notification (d, lang); + } Curly braces just add noise when you have a single statement, but no big deal @@ +1500,3 @@ button = get_widget (d, "user-icon-button"); d->photo_dialog = um_photo_dialog_new (button); + d->main_box = get_widget (d, "overlay"); This doesn't seem to be needed and obfuscates the code since a GtkOverlay is not a GtkBox. You can use get_widget() in show_restart_notification()
Comment on attachment 255924 [details] [review] add restart notification commit 0002313af8103c1a5bf981e6e49d862f460df378
Thanks for the review, I've modified the patch accordingly before push...
Switching to another language and back to current language it still forces the notification to be displayed.
The region and language panel has exactly same behavioral, so it should be fixed also in the both panels...
Created attachment 283503 [details] [review] do not show the notification if the locale is already used Maybe it could be fixed this way, however I'm not sure LC_MESSAGES is correct variable to check current language. We need to test it after the Bug 730478 will be fixed.
Created attachment 286382 [details] [review] user-accounts & region: allow multiline notifications The notification is wider then window for some languages (e.g. France). This patch allows wrapping, limits chars and changes margin as a result of discussion on #gnome-design.
Created attachment 286384 [details] screenshot of the multiline notification
Review of attachment 286382 [details] [review]: Looks good
Ondrej, can you ask for a freeze break for this ?
Done :-)
Comment on attachment 286382 [details] [review] user-accounts & region: allow multiline notifications commit d82a2101d5ac7bfa3f24b4502a8c5afe73c0e48c
Created attachment 294647 [details] [review] do not set the language twice The act_user_set_language function is called twice as a result of commit 96c0c0a, which is not necessary.
Created attachment 294648 [details] [review] do not show the notification if not needed Restart notification is shown if there is different language in accountsservice. However restart isn't needed if user selects original language (after several changes), which is already used in the system. This is updated patch to master with some fixes...
Review of attachment 294647 [details] [review]: ++
Review of attachment 294648 [details] [review]: otherwise fine ::: panels/user-accounts/um-user-panel.c @@ +1095,3 @@ ActUser *user; const gchar *lang, *current_language; + gchar *system_language; Please call this current_language and s/current_language/account_language/ the existing one. These names describe what this is all about better IMO. system_language might be misinterpreted as the system-wide language.
Comment on attachment 294647 [details] [review] do not set the language twice commit b0e6526c468b9fd761a90fa28c70af664305efc2
Comment on attachment 294648 [details] [review] do not show the notification if not needed Thanks for review! I renamed it and committed: commit 8bd1a5f18d82924ab12a73a57da6659e26bc6766