GNOME Bugzilla – Bug 578578
"Manage Groups" dialog should have an Unlock button
Last modified: 2009-12-19 17:39:17 UTC
When using policy kit, if you go to the "Manage Groups" dialog without first doing an Unlock at the previous screen, you get a nearly useless dialog. Adding an Unlock button to this dialog box would make it useful. Other information: This is similar to #551617, just a different dialog.
Created attachment 132448 [details] [review] This patch adds the button This is the patch I put together to do it. It makes some interface changes to some higher level objects, so someone with a better big-picture idea of the codebase might want to review it to make sure isn't totally dumb. If this patch gets merged in, I plan on adding more unlock buttons to dialogs.
So, if I'm kind of the maintainer of the g-s-t, I'll start reviewing the patches you post. I'd really like to get this in, here are the details I'd rather get fixed before (not a big deal). I've not tested your code yet, but I trust you that it should work. It would be nice though that you explain the more you can what those functions do (in comments), and that you prepare a commit message explaining the changes. Then just use git format-patch and I'll commit that. Thanks! === modified file 'src/common/gst-dialog.c' > --- src/common/gst-dialog.c 2009-03-18 07:28:25 +0000 > +++ src/common/gst-dialog.c 2009-04-10 05:05:55 +0000 > @@ -192,6 +192,18 @@ > NULL); > } > > +void > +gst_dialog_link_unlock_button(GstDialog *dialog, GstPolKitButton *button) { > + GstDialogPrivate *priv; > + > + priv = GST_DIALOG_GET_PRIVATE (dialog); > + > + gst_polkit_button_link (button, priv->polkit_button); > + > + g_signal_connect_swapped (button, "changed", > + G_CALLBACK (gst_dialog_unlock), dialog); > +} What does exactly this new function does? Adding a comment would be nice. -#include "gst-tool.h" > - > #define GST_TYPE_DIALOG (gst_dialog_get_type ()) > #define GST_DIALOG(o) (G_TYPE_CHECK_INSTANCE_CAST ((o), GST_TYPE_DIALOG, GstDialog)) > #define GST_DIALOG_CLASS(c) (G_TYPE_CHECK_CLASS_CAST ((c), GST_TYPE_DIALOG, GstDialogClass)) > @@ -36,6 +34,11 @@ > typedef struct _GstDialogClass GstDialogClass; > typedef struct _GstDialogSignal GstDialogSignal; > > +#include "gst-tool.h" > +#ifdef HAVE_POLKIT > +#include "gst-polkit-button.h" > +#endif Any reason why you are moving the #include "gst-tool.h" around? GstPolKitAction * > -gst_polkit_action_new (const gchar *action, > - GtkWidget *widget) > +gst_polkit_action_new (const gchar *action) > [...] > > gboolean > -gst_polkit_action_authenticate (GstPolKitAction *action) > +gst_polkit_action_authenticate (GstPolKitAction *action, GtkWidget *trigger_widget) From what I understand, you're removing the widget from the action itself, and move it to the authentication. Why is this needed? Could you explain that in a comment somewhere? > if (GTK_WIDGET_TOPLEVEL (toplevel) && > GTK_WIDGET_REALIZED (toplevel)) > > === modified file 'src/common/gst-polkit-action.h' > --- src/common/gst-polkit-action.h 2008-11-06 07:56:42 +0000 > +++ src/common/gst-polkit-action.h 2009-04-10 04:39:44 +0000 > @@ -46,8 +46,7 @@ > > GType gst_polkit_action_get_type (void) G_GNUC_CONST; > > -GstPolKitAction * gst_polkit_action_new (const gchar *action, > - GtkWidget *widget); > +GstPolKitAction * gst_polkit_action_new (const gchar *action); > > G_CONST_RETURN gchar * gst_polkit_action_get_action (GstPolKitAction *action); > void gst_polkit_action_set_action (GstPolKitAction *action, > @@ -56,7 +55,8 @@ > PolKitResult gst_polkit_action_get_result (GstPolKitAction *action); > gboolean gst_polkit_action_get_authenticated (GstPolKitAction *action); > > -gboolean gst_polkit_action_authenticate (GstPolKitAction *action); > +gboolean gst_polkit_action_authenticate (GstPolKitAction *action, > + GtkWidget *trigger_widget); Indentation mistake, or am I misled by the presentation of the patch? There are other occurrences of this, including in the already existing code. Please note that you should use tabs as much as possible, that seems to be the coding style of the g-s-t. --- src/common/gst-polkit-button.c 2008-11-06 07:57:22 +0000 > +++ src/common/gst-polkit-button.c 2009-04-10 05:01:41 +0000 > @@ -20,6 +20,7 @@ > #include <glib/gi18n.h> > #include "gst-polkit-action.h" > #include "gst-polkit-button.h" > +#include "gst-dialog.h" > > #define GST_POLKIT_BUTTON_GET_PRIVATE(o) (G_TYPE_INSTANCE_GET_PRIVATE ((o), GST_TYPE_POLKIT_BUTTON, GstPolKitButtonPriv)) > > @@ -161,7 +162,7 @@ > GstPolKitButtonPriv *priv; > > priv = GST_POLKIT_BUTTON_GET_PRIVATE (button); > - priv->action = gst_polkit_action_new (NULL, GTK_WIDGET (button)); > + priv->action = gst_polkit_action_new (NULL); > > g_signal_connect_swapped (priv->action, "changed", > G_CALLBACK (action_state_changed), button); > @@ -233,7 +234,7 @@ > PolKitResult result; > > priv = GST_POLKIT_BUTTON_GET_PRIVATE (button); > - gst_polkit_action_authenticate (priv->action); > + gst_polkit_action_authenticate (priv->action, GTK_WIDGET (button)); > result = gst_polkit_action_get_result (priv->action); > > if (result == POLKIT_RESULT_UNKNOWN) { > @@ -265,6 +266,36 @@ > NULL); > } > > +void > +gst_polkit_button_insert_into_dialog (GstPolKitButton *button, GtkDialog *dialog) { > + GtkBox *action_area = GTK_BOX (gtk_dialog_get_action_area(dialog)); Space missing. ^^^ + GList *children = gtk_container_get_children (GTK_CONTAINER (action_area)); > + GList *iter; > + gint new_position = -1; > + > + for (iter = g_list_last (children); iter != NULL; iter = g_list_previous (iter)) { > + switch (gtk_dialog_get_response_for_widget (dialog, GTK_WIDGET (iter->data))) { > + case GTK_RESPONSE_NONE: > + case GTK_RESPONSE_OK: > + case GTK_RESPONSE_APPLY: > + case GTK_RESPONSE_CLOSE: > + break; > + default: > + new_position = g_list_position (children, iter); > + } > + if (new_position >= 0) { > + break; > + } > + } > + > + g_list_free(children); > + > + gtk_box_pack_end(action_area, GTK_WIDGET (button), FALSE, TRUE, 0); > + gtk_box_reorder_child(action_area, GTK_WIDGET (button), 1); > + > + gtk_widget_show(GTK_WIDGET (button)); Three spacing mistakes before parentheses. > + g_object_unref (priv->action); > + priv->action = g_object_ref (link_to_priv->action); > + update_button_state (button); > + > + g_object_notify (G_OBJECT (button), "action"); > +} > + > G_CONST_RETURN gchar * > gst_polkit_button_get_label (GstPolKitButton *button) > { > @@ -327,7 +376,7 @@ > gst_polkit_button_get_authenticated (GstPolKitButton *button) > { > GstPolKitButtonPriv *priv; > - > + Please remove the spaces in the 4 blank lines. Note that using "git config --global color.ui auto" will make them appear red, very useful! ;-) See http://live.gnome.org/Git/Developers. [...] dialog = gst_dialog_get_widget (tool->main_dialog, "groups_dialog"); > gtk_window_set_transient_for (GTK_WINDOW (dialog), GTK_WINDOW (tool->main_dialog)); > - while (gtk_dialog_run (GTK_DIALOG (dialog)) == GTK_RESPONSE_HELP); > + > +#ifdef HAVE_POLKIT > + unlock_button = gst_polkit_button_new (NULL, _("_Unlock")); > + gst_polkit_button_insert_into_dialog (unlock_button, GTK_DIALOG (dialog)); > + gst_dialog_link_unlock_button(tool->main_dialog, unlock_button); > +#endif > + > + while (gtk_dialog_run (GTK_DIALOG (dialog)) != GTK_RESPONSE_CLOSE); > + > gtk_widget_hide (dialog); > + Spaces in blank line too.
Re whitespace things - sorry. Usually, I'm a bit more careful reading through my diffs before submitting them. In my defense, I was hoping for some feedback on the higher level stuff... but you are totally right. > What does exactly this new function does? Adding a comment would be nice. Will do. > Any reason why you are moving the #include "gst-tool.h" around? No. Please include my excuse for the whitespace issues. Sorry :) > From what I understand, you're removing the widget from the action itself, and > move it to the authentication. Why is this needed? Could you explain that in a > comment somewhere? It is needed because an action has multiple widgets (one on each dialog). More plainly - why should an action be coupled with a widget? I figured that it makes more sense to separate out the two distinct parts than to have an object which holds a reference to a widget for its entire lifetime, even though it only uses this reference at one spot in the code, where the caller has easy access to this reference. I don't really see what I could put in the code that would help understanding, since it is simpler than the previous implementation. I'm always happy to make the code easier to understand, so if you have a suggestion for what a reader would want to see, then I'm happy to put it in the code. Thanks for the feedback. I'll get an updated patch here as soon as possible.
Created attachment 139283 [details] [review] Cleanup of the old patch > Any reason why you are moving the #include "gst-tool.h" around? Actually, yes there was a reason. Unless this comes after the struct definitions for GstDialog, then if you try to include gst-dialog.h without first including gst-tool.h (as I end up doing in gst-polkit-button.c), then it won't compile. > Indentation mistake, or am I misled by the presentation of the patch? There are > other occurrences of this, including in the already existing code. Please note > that you should use tabs as much as possible, that seems to be the coding > style of the g-s-t. It turned out that this one is ok too. The patch had all tabs, until the end where it was spaces, and when viewed in a text editor it lines up correctly (tab width is 8). My preference is to use tabs only for indentation, and spaces for alignment (so tab width doesn't matter)... it seems a bit more sane than just using tabs as big spaces. Feedback is always appreciated. Once we are happy with this guy, I'll see about a similar patch for the user properties page.
I've now tested your patch, and it works fine for me too. Nice! Though I've still a few remarks... ;-) - Still a few unwanted tabs in: gst_dialog_link_unlock_button() gst_polkit_button_insert_into_dialog() gst_polkit_button_link() on_manage_groups_clicked() - Compiler warnings: gst-dialog.c:203: attention : passing argument 2 of ‘gst_polkit_button_link’ from incompatible pointer type callbacks.c:391: attention : assignment from incompatible pointer type I guess you should define polkit_button in _GstDialogPrivate as a GstPolKitButton, and use the macro GTK_WIDGET () when needed to convert it. - The patch does not build when disabling PolicyKit: try './configure --enable-polkit=no && make'. Maybe we could drop support for building without PolicyKit, but that's another story we'd need to discuss. To create your new patch, please use 'git commit', fill the description as you want and then use 'git format-patch HEAD~'. This way I'll already have everything set to push it. On a broader look, it could be nice to unlock automatically when the user tries some actions, e.g. double-click on a user/group. Maybe we could forget the greyed out buttons/lists and make this behavior general. Bug 558976 wants this. You seem to master these mechanisms, how do you feel about that?
(In reply to comment #5) > I've now tested your patch, and it works fine for me too. Nice! Though I've > still a few remarks... ;-) > > - Still a few unwanted tabs in: > gst_dialog_link_unlock_button() > gst_polkit_button_insert_into_dialog() > gst_polkit_button_link() > on_manage_groups_clicked() Oh, blank line tabs. Gah. Sorry, my typical style is to give every line inside a block the same number of tabs, and use spaces to do vertical alignment. I'll fix it :) > > - Compiler warnings: > gst-dialog.c:203: attention : passing argument 2 of > ‘gst_polkit_button_link’ from incompatible pointer type > callbacks.c:391: attention : assignment from incompatible pointer type > > I guess you should define polkit_button in _GstDialogPrivate as a > GstPolKitButton, and use the macro GTK_WIDGET () when needed to convert it. Yeah, would you object to -Werror as a default in the future? (There are some other warnings that should probably be fixed) > > - The patch does not build when disabling PolicyKit: try './configure > --enable-polkit=no && make'. Maybe we could drop support for building without > PolicyKit, but that's another story we'd need to discuss. I'm guessing I'm doing it wrong, as my ./configure doesn't support --enable-polkit. I'll hack it or something to disable policy kit. I don't think dropping policykit support is a good idea at this stage. I haven't thought much about the idea... but it doesn't strike me as something that will significantly drop the amount of effort needed to maintain stuff (so I say after producing a patch that breaks when policykit is disabled :) > > To create your new patch, please use 'git commit', fill the description as you > want and then use 'git format-patch HEAD~'. This way I'll already have > everything set to push it. > Sure. > > On a broader look, it could be nice to unlock automatically when the user tries > some actions, e.g. double-click on a user/group. Maybe we could forget the > greyed out buttons/lists and make this behavior general. Bug 558976 wants this. > You seem to master these mechanisms, how do you feel about that? > I'm not totally sure about this one, I might put it to the usability list and see what they think. I think having the checkbuttons looking disabled would be a better solution, since you don't expect a dialog to pop up when you click on a checkbox. It might make more sense for things where you click a button (e.g. the "Properties" button for a group). But then, it would also be useful to have a way of showing which actions will require an unlock and which won't. I'm happy to keep working on all of this stuff though. I'll hopefully get around to fixing this patch this weekend. Andy
Last chance to get this in the upcoming release... Would you have some time to finish the patch? About testing without PolicyKit, you can simply add something like #undef HAVE_POLKIT in config.h, or some other file.
Created attachment 140965 [details] [review] Add a policy kit unlock button to the manage groups dialog Thanks for the poke. I've had a few incredibly hectic weeks and didn't have any time. This I believe fixes all the issues you had.
Created attachment 140966 [details] [review] Add a policy kit unlock button to the manage groups dialog (fixed email address)
Actually, I feel a little stupid now that you provided the final patch. In the meantime, I've worked on migrating to PolicyKit1, which is the new blessed framework, and this changes things a bit. I've removed GstPolKitButton, which is now replaced by PolkitLockButton from polkit-gnome. Quite easy. So that should make the patch simpler, but ATST I'm not sure it's very easy to move to it. Could you have a look at the patch at bug 592127 and give me your feeling? Sorry for that!
I've thought about this a little more while redesigning the UI, and I think we should simply prompt the user for authentication when clicking on Edit/Delete/New group. This is much simpler. Would you have a look at that? We need to add a new D-Bus call in the system-tools-backends' dispatcher for that, use it in liboobs, and then call it in every place we need it. I can work on some part of it if you like.
Ok. I've been quite busy recently, and I still haven't even got around to having a look at the redesigned UI (sorry). I'm looking like having some spare time in about a month... it is tough to plan that far in advance, so I might be totally wrong :) Anyway, without knowing what the UI is looking like, I'm likely to be talking rubbish... with that said, bringing up an unlocker whenever the user tries to do something that needs it sounds good... but if there are things like text entry boxes that are disabled when it is locked, then this would break. I also wonder if there should be some sort of convention to display items which will launch an unlocker slightly differently to those that won't. Perhaps some sort of little padlock embedded in the control somewhere or similar.
No problem, just keep in mind the release schedule [1]: major features should be committed before January 25th, and the GUI shouldn't change after February 8th. Showing a padlock on the buttons that require authentication would indeed be nice (it's in the design document). You could give a try at this, the problem is that we would have to support some kind of PolicyKit signals to be warned when authorizations are lost. The basic support I'd like to commit soon will only check that we are authenticated when clicking, which is much easier. (If you're on Ubuntu, you can install liboobs and gnome-system-tools 2.29.1 from Lucid even in Karmic, I think.) 1: http://live.gnome.org/TwoPointTwentynine