After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 703392 - Changing language from user settings for a user will not ask for restart
Changing language from user settings for a user will not ask for restart
Status: RESOLVED FIXED
Product: gnome-control-center
Classification: Core
Component: User Accounts
3.12.x
Other Linux
: Normal normal
: ---
Assigned To: Ondrej Holy
Control-Center Maintainers
Depends on:
Blocks:
 
 
Reported: 2013-07-01 13:30 UTC by Marie.KOWALCZYK
Modified: 2015-01-19 12:15 UTC
See Also:
GNOME target: ---
GNOME version: 3.7/3.8


Attachments
add restart notification (5.13 KB, patch)
2013-07-31 11:55 UTC, Ondrej Holy
reviewed Details | Review
notification screenshot (38.94 KB, image/png)
2013-07-31 11:55 UTC, Ondrej Holy
  Details
add restart notification (7.50 KB, patch)
2013-08-13 11:49 UTC, Ondrej Holy
rejected Details | Review
add restart notification (6.59 KB, patch)
2013-09-27 12:37 UTC, Ondrej Holy
committed Details | Review
do not show the notification if the locale is already used (1.68 KB, patch)
2014-08-15 14:18 UTC, Ondrej Holy
none Details | Review
user-accounts & region: allow multiline notifications (3.46 KB, patch)
2014-09-17 14:30 UTC, Ondrej Holy
committed Details | Review
screenshot of the multiline notification (42.96 KB, image/png)
2014-09-17 14:33 UTC, Ondrej Holy
  Details
do not set the language twice (1.08 KB, patch)
2015-01-16 08:48 UTC, Ondrej Holy
committed Details | Review
do not show the notification if not needed (1.75 KB, patch)
2015-01-16 08:49 UTC, Ondrej Holy
committed Details | Review

Description Marie.KOWALCZYK 2013-07-01 13:30:41 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.
Comment 1 Ondrej Holy 2013-07-31 11:55:09 UTC
Created attachment 250537 [details] [review]
add restart notification
Comment 2 Ondrej Holy 2013-07-31 11:55:46 UTC
Created attachment 250538 [details]
notification screenshot
Comment 3 Stef Walter 2013-08-08 13:08:26 UTC
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.
Comment 4 Ondrej Holy 2013-08-13 11:49:20 UTC
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.
Comment 5 Stef Walter 2013-08-15 06:17:40 UTC
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 6 Stef Walter 2013-08-16 21:13:25 UTC
Comment on attachment 250537 [details] [review]
add restart notification

Will commit this patch, with leak fix.
Comment 7 Stef Walter 2013-08-16 21:16:23 UTC
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
Comment 8 Ondrej Holy 2013-09-27 12:37:34 UTC
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
Comment 9 Ondrej Holy 2013-09-27 12:40:19 UTC
(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...
Comment 10 Ondrej Holy 2014-06-24 11:37:21 UTC
Now the warnings are fixed, could you review the attached patch please?
Comment 11 Rui Matos 2014-08-02 16:14:31 UTC
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 12 Ondrej Holy 2014-08-07 11:57:12 UTC
Comment on attachment 255924 [details] [review]
add restart notification

commit 0002313af8103c1a5bf981e6e49d862f460df378
Comment 13 Ondrej Holy 2014-08-07 11:58:47 UTC
Thanks for the review, I've modified the patch accordingly before push...
Comment 14 alex diavatis 2014-08-08 14:17:44 UTC
Switching to another language and back to current language it still forces the notification to be displayed.
Comment 15 Ondrej Holy 2014-08-15 13:44:42 UTC
The region and language panel has exactly same behavioral, so it should be fixed also in the both panels...
Comment 16 Ondrej Holy 2014-08-15 14:18:06 UTC
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.
Comment 17 Ondrej Holy 2014-09-17 14:30:19 UTC
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.
Comment 18 Ondrej Holy 2014-09-17 14:33:35 UTC
Created attachment 286384 [details]
screenshot of the multiline notification
Comment 19 Matthias Clasen 2014-09-17 14:41:23 UTC
Review of attachment 286382 [details] [review]:

Looks good
Comment 20 Matthias Clasen 2014-09-18 16:58:29 UTC
Ondrej, can you ask for a freeze break for this ?
Comment 21 Ondrej Holy 2014-09-19 07:29:01 UTC
Done :-)
Comment 22 Ondrej Holy 2014-09-19 11:38:12 UTC
Comment on attachment 286382 [details] [review]
user-accounts & region: allow multiline notifications

commit d82a2101d5ac7bfa3f24b4502a8c5afe73c0e48c
Comment 23 Ondrej Holy 2015-01-16 08:48:04 UTC
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.
Comment 24 Ondrej Holy 2015-01-16 08:49:53 UTC
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...
Comment 25 Rui Matos 2015-01-16 17:04:52 UTC
Review of attachment 294647 [details] [review]:

++
Comment 26 Rui Matos 2015-01-16 17:18:57 UTC
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 27 Ondrej Holy 2015-01-19 12:14:02 UTC
Comment on attachment 294647 [details] [review]
do not set the language twice

commit b0e6526c468b9fd761a90fa28c70af664305efc2
Comment 28 Ondrej Holy 2015-01-19 12:15:09 UTC
Comment on attachment 294648 [details] [review]
do not show the notification if not needed

Thanks for review! I renamed it and committed:
commit 8bd1a5f18d82924ab12a73a57da6659e26bc6766