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 723550 - region-panel: do not show Login button if timedated is not available
region-panel: do not show Login button if timedated is not available
Status: RESOLVED FIXED
Product: gnome-control-center
Classification: Core
Component: Region & Language
3.10.x
Other OpenBSD
: Normal normal
: ---
Assigned To: Control-Center Maintainers
Control-Center Maintainers
Depends on:
Blocks:
 
 
Reported: 2014-02-03 18:21 UTC by Antoine Jacoutot
Modified: 2014-02-04 14:45 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
do not display "Login" if we can't get priv->permission (1.60 KB, patch)
2014-02-03 18:21 UTC, Antoine Jacoutot
needs-work Details | Review
do not display "Login" if we can't get priv->permission v2 (2.16 KB, patch)
2014-02-04 14:11 UTC, Antoine Jacoutot
committed Details | Review

Description Antoine Jacoutot 2014-02-03 18:21:54 UTC
Created attachment 267988 [details] [review]
do not display "Login" if we can't get priv->permission

Hi.

Missing org.freedesktop.locale1 means priv->permission will not be set
and will trigger a segfault when used by set_login_button_visibility().
Unconditionally not display the Login button if timedated is not available
since it will be useless anyway.

Thoughts?
Comment 1 Rui Matos 2014-02-04 11:08:12 UTC
Review of attachment 267988 [details] [review]:

Thanks for the patch.

I think that checking if the permission is NULL right after we try to get it and returning immediately would be better. I'd also capture the GError from polkit_permission_new_sync() in that case and issue a warning.
Comment 2 Antoine Jacoutot 2014-02-04 11:18:42 UTC
(In reply to comment #1)
> Review of attachment 267988 [details] [review]:
> 
> Thanks for the patch.
> 
> I think that checking if the permission is NULL right after we try to get it
> and returning immediately would be better. I'd also capture the GError from
> polkit_permission_new_sync() in that case and issue a warning.

It that was my first idea. But IIRC I ended up with warning about GtkWidget being NULL or something at runtime.
I'll give it another try...
Comment 3 Antoine Jacoutot 2014-02-04 11:49:10 UTC
Yeah if I go your way I end up with this patch and this output (the WARNING is legit obviously, but the *-CRITICAL...):

--- panels/region/cc-region-panel.c.orig        Tue Feb  4 12:37:59 2014
+++ panels/region/cc-region-panel.c     Tue Feb  4 12:41:15 2014
@@ -1698,8 +1698,16 @@ setup_login_button (CcRegionPanel *self)
        CcRegionPanelPrivate *priv = self->priv;
         GDBusConnection *bus;
         gboolean loaded;
+        GError *error = NULL;
 
-        priv->permission = polkit_permission_new_sync ("org.freedesktop.locale1.set-locale", NULL, NULL, NULL);
+        priv->permission = polkit_permission_new_sync ("org.freedesktop.locale1.set-locale", NULL, NULL, &error);
+        if (priv->permission == NULL) {
+                g_warning ("Could not get 'org.freedesktop.locale1.set-locale' permission: %s",
+                           error->message);
+                g_error_free (error);
+                return;
+        }
+
         bus = g_bus_get_sync (G_BUS_TYPE_SYSTEM, NULL, NULL);
         g_dbus_proxy_new (bus,
                           G_DBUS_PROXY_FLAGS_GET_INVALIDATED_PROPERTIES



$ gnome-control-center                                         

(gnome-control-center:26694): region-cc-panel-WARNING **: Could not get 'org.freedesktop.locale1.set-locale' permission: GDBus.Error:org.freedesktop.PolicyKit1.Error.Failed: Action org.freedesktop.locale1.set-locale is not registered

(gnome-control-center:26694): Gtk-CRITICAL **: gtk_box_pack: assertion 'GTK_IS_WIDGET (child)' failed

(gnome-control-center:26694): GLib-GObject-CRITICAL **: g_object_ref: assertion 'G_IS_OBJECT (object)' failed

(gnome-control-center:26694): Gtk-CRITICAL **: gtk_size_group_add_widget: assertion 'GTK_IS_WIDGET (widget)' failed
Comment 4 Rui Matos 2014-02-04 12:46:48 UTC
(In reply to comment #3)

> $ gnome-control-center                                         
> 
> (gnome-control-center:26694): region-cc-panel-WARNING **: Could not get
> 'org.freedesktop.locale1.set-locale' permission:
> GDBus.Error:org.freedesktop.PolicyKit1.Error.Failed: Action
> org.freedesktop.locale1.set-locale is not registered

This is ok.

> (gnome-control-center:26694): Gtk-CRITICAL **: gtk_box_pack: assertion
> 'GTK_IS_WIDGET (child)' failed
> 
> (gnome-control-center:26694): GLib-GObject-CRITICAL **: g_object_ref: assertion
> 'G_IS_OBJECT (object)' failed
> 
> (gnome-control-center:26694): Gtk-CRITICAL **: gtk_size_group_add_widget:
> assertion 'GTK_IS_WIDGET (widget)' failed

This is because we are still trying to pack the login_button widget in the header bar. You could either skip that in _constructed() if (permission == NULL) or move the login_button setup before the permission initialization so that the widget is still constructed.
Comment 5 Antoine Jacoutot 2014-02-04 14:11:28 UTC
Created attachment 268074 [details] [review]
do not display "Login" if we can't get priv->permission v2

Yup. That updated patch seems to do the trick.
Comment 6 Rui Matos 2014-02-04 14:44:37 UTC
Review of attachment 268074 [details] [review]:

Thanks, this looks good, I'm going to push this with s/timedated/localed/ in the commit message and spin the 3.11.5 release.