Created attachment 238493 [details] [review]
region: Translate restart notification into the target language

We used to do this before the panel re-design.

Review of attachment 238493 [details] [review]:

Looks good to me

Review of attachment 238493 [details] [review]:

Seems that this code would still show a restart notification if the language was already the language of the session. Looks fine otherwise.

Created attachment 238577 [details] [review]
region: Show restart notification only if strictly needed

If the user is changing the setting back to the current locale we
shouldn't show the restart notification.
--

(In reply to comment #3)
> Seems that this code would still show a restart notification if the language
> was already the language of the session. Looks fine otherwise.

Good point. This should address that and while at it also your comment
https://bugzilla.gnome.org/show_bug.cgi?id=695535#c6 .

Review of attachment 238577 [details] [review]:

::: panels/region/cc-region-panel.c
@@ +275,3 @@
+        current_locale = setlocale (category, NULL);
+
+        if (!gnome_parse_locale (current_locale, &current_lang_code, &current_country_code, NULL, NULL))

The current locale is the one you would get calling GetLocale() on org.gnome.SessionManager, not the one you would get running SetLocale.

Created attachment 238820 [details] [review]
region: Use a GDBusProxy to access org.gnome.SessionManager

We'll use this interface beyond just calling Logout() so lets keep a
proxy around.

Created attachment 238821 [details] [review]
region: Show restart notification only if strictly needed

--

(In reply to comment #5)
> The current locale is the one you would get calling GetLocale() on
> org.gnome.SessionManager, not the one you would get running SetLocale.

Ok. This one does that

Review of attachment 238820 [details] [review]:

::: panels/region/cc-region-panel.c
@@ +1595,3 @@
+        GError *error = NULL;
+
+        priv->session = g_dbus_proxy_new_for_bus_finish (res, &error);

Needs to check for G_IO_ERROR_CANCELLED here

@@ +1626,3 @@
         priv->user_manager = act_user_manager_get_default ();
 
+        g_dbus_proxy_new_for_bus (G_BUS_TYPE_SESSION,

Missing a cancellable.

Review of attachment 238821 [details] [review]:

Looks fine otherwise.

::: panels/region/cc-region-panel.c
@@ +331,3 @@
+{
+        CcRegionPanelPrivate *priv = self->priv;
+        MaybeNotifyData *mnd = g_new0 (MaybeNotifyData, 1);

split this.

@@ +337,3 @@
+        mnd->target_locale = g_strdup (target_locale);
+
+        g_dbus_proxy_call (priv->session,

Needs a GCancellable.

Created attachment 238856 [details] [review]
region: Cancel any async operations with callbacks on finalize

Otherwise we may end up using a finalized "object" in the callbacks.

Created attachment 238857 [details] [review]
region: Use a GDBusProxy to access org.gnome.SessionManager

--

There was a previous place where we weren't using a cancellable
so I attached a patch to fix that too.

(In reply to comment #8)
> ::: panels/region/cc-region-panel.c
> @@ +1595,3 @@
> +        GError *error = NULL;
> +
> +        priv->session = g_dbus_proxy_new_for_bus_finish (res, &error);
>
> Needs to check for G_IO_ERROR_CANCELLED here

Good point fixed.

> @@ +1626,3 @@
>          priv->user_manager = act_user_manager_get_default ();
>
> +        g_dbus_proxy_new_for_bus (G_BUS_TYPE_SESSION,
>
> Missing a cancellable.

Fixed.

Created attachment 238858 [details] [review]
region: Show restart notification only if strictly needed

--

(In reply to comment #9)
> ::: panels/region/cc-region-panel.c
> @@ +331,3 @@
> +{
> +        CcRegionPanelPrivate *priv = self->priv;
> +        MaybeNotifyData *mnd = g_new0 (MaybeNotifyData, 1);
>
> split this.

Done.

> @@ +337,3 @@
> +        mnd->target_locale = g_strdup (target_locale);
> +
> +        g_dbus_proxy_call (priv->session,
>
> Needs a GCancellable.

Fixed.

Review of attachment 238856 [details] [review]:

Looks good.

Review of attachment 238857 [details] [review]:

Looks good.

Review of attachment 238858 [details] [review]:

Looks good otherwise.

::: panels/region/cc-region-panel.c
@@ +292,3 @@
+        const gchar *current_locale = NULL;
+
+        retval = g_dbus_proxy_call_finish (self->priv->session, res, &error);

NO!
s/self->priv->session/G_DBUS_PROXY(source)/

(In reply to comment #15)
> ::: panels/region/cc-region-panel.c
> @@ +292,3 @@
> +        const gchar *current_locale = NULL;
> +
> +        retval = g_dbus_proxy_call_finish (self->priv->session, res, &error);
>
> NO!
> s/self->priv->session/G_DBUS_PROXY(source)/

GAH! Thanks for spotting, pushed with the fix.

Attachment 238493 [details] pushed as e2163ea - region: Translate restart notification into the target language
Attachment 238856 [details] pushed as 3efb71b - region: Cancel any async operations with callbacks on finalize
Attachment 238857 [details] pushed as 50ea2c1 - region: Use a GDBusProxy to access org.gnome.SessionManager
Attachment 238858 [details] pushed as 9c64f31 - region: Show restart notification only if strictly needed