Created attachment 107227 [details] [review]
Implemention GtkMountOperation

This implementation does not use a seperate GtkPasswordDialog thingy that libgnomeui has, but just builds up a custom dialog in the way it needs it to be. If we are going to add own GtkPasswordDialog later replace the current code with the GtkPasswordDialog.

Reading the patch it looks like this runs in-process? There's a couple of reasons related to password hygiene for not doing this. Specifically it's not a good idea to leak the passwords into the address space of any random process that happens to use gtk+ especially as you don't always have the source to the application.

Anyway, I summarized the concerns here

 http://mail.gnome.org/archives/gtk-devel-list/2007-December/msg00149.html

in a handwavey way. Alex's reply

 http://mail.gnome.org/archives/gtk-devel-list/2007-December/msg00169.html

has much more concrete ideas on how to achieve this. Thoughts?

I must say I really like the idea of having a trusted channel but I don't think it is that big of a problem right now since: 1) we don't don't have that now with the eel based password dialog nor 2) did we have such a secure path with gnome-vfs. Also we lack that whole GSecureMountOperation stuff right now. And with GSecureMountOperation being an interface we wouldn't break API/ABI if we later added that secure path functionality to GtkMountOperation (for Unix based systems). So we could have the not-so-secure version now, especially since we need that for the file-chooser gio-port, and also having it as a fallback when we dont have a GSecureMountOperation, and add the more secure version later. Opinions?

Personally I think it's probably fine to punt on the secure stuff as long as we can implement it later. I just wanted to raise the issue.

Punt the secure version for now. But this API should let us transparently add it.

Maybe we want a gtk_mount_operation_set_screen() ?
In case parent is NULL but you still want to specify where to pop up things?

Created attachment 107322 [details] [review]
Implement gtk_mount_operation_set_screen

Incremental patch to add gtk_mount_operation_[set|get]_screen. I think it also fixes minor stuff.

Looking over the first patch:

Some erroneous whitespace, like duplicate empty lines

+
+

which we generally avoid inside functions.


+  if (entry_widget == NULL)
+    return TRUE;
+
+  if (! GTK_IS_ENTRY (entry_widget))
+    return FALSE;

GTK_IS_ENTRY already checks for NULL, so the first check is redundant.


+  is_valid &= entry_has_valid_input (priv->username_entry);
+
+  if (is_valid)
+    is_valid &= entry_has_valid_input (priv->domain_entry);
+
+  if (is_valid)
+    is_valid &= entry_has_valid_input (priv->password_entry);

I'd probably write this as a single assignment, but not a big deal.


+  if (priv->anonymous == TRUE)

I hate comparisons involving booleans. This should just be

   if (priv->anonymous)


+  if ((can_anonymous = (flags & G_ASK_PASSWORD_ANONYMOUS_SUPPORTED)))

Please no assignments hidden in ifs


+  primary = strstr (message, "\n");

Hmm, slightly icky. Is this required by the GMountOperation api ?


+      primary = g_strndup (message, strlen (message) - strlen (primary));

primary - message  would give you the length of the first line without so
much strlen'ing, no ?


Looking at the incremental patch next:

Since you add a setter and getter, and handle overwriting an existing value,
it doesn't make much sense to restrict the screen property to be construct-only, I think.

And once you made the screen a regular property, it becomes obvious that maybe
parent doesn't need to be a construct-only either.


Next step: building and trying it, tomorrow...

Created attachment 107333 [details] [review]
New version of the bug

I hopefully addressed the most issues you raised with this new incarnation of the patch.
The "slightly icky" "primary = strstr (message, "\n");" statement is there to make the first line being bold and thus the "caption" of the dialog since you can only pass one string as message to GMountOperation::ask_question ().

Thanks for reviewing! ;-)

Created attachment 107334 [details] [review]
New version of the patch

Ewk, svn diff in the wrong directory. Sorry.

Few more comments from playing around with testmountoperation:

(lt-testmountoperation:29308): Gtk-CRITICAL **: gtk_mount_operation_set_parent: assertion `GTK_IS_WINDOW (parent)' failed

Should probably be   parent == NULL || GTK_IS_WINDOW (parent)
(and similar for screen)


Shouldn't the password entry be a _password entry_ ? (ie show invisible characters instead of what I type)


Running it with --no-pw-save still shows the pw saving options ? Similar for
--no-username and --no-password.

Created attachment 107398 [details] [review]
Next attempt ;-)

This one misses gtkmountoperation.h...

I managed to try it anyway, and it seems to work as expected now.
One further comments:

entry_has_valid_input doesn't really validate anything, so maybe it would be
more appropriately named entry_has_input ?

Created attachment 107457 [details] [review]
And another round ...

Yeah, I agree on the has_valid_input vs has_input naming. I fixed that. This time the header should also be included (I switch to using git and messed up obviously, sorry for that)

Looks fine now, as far as I'm concerned.
It is missing docs, but we can handle that afterwards.

Committed. Rename bug to reflect the need for documentation.

2008-03-17  Christian Kellner  <gicmo@gnome.org>

	Implement GtkMountOperation, a subclass of GMountOperation
	to be used with gio wherever there is the need to ask the
	user for credentials or questions while mounting a volume.
	This is bug #522245

	* gtk/gtkmountoperation.c:
	* gtk/gtkmountoperation.h:
	Implement GtkMountOperation.
	
	* gtk/gtk.h: Add gtkmountoperation.h
	* gtk/Makefile.am: Add gtkmountoperation.[hc]
	* gtk/gtk.symbols: Add symbols of GtkMountOperation.
	* tests/testmountoperation.c: Test program for it.
	* tests/Makefile.am: Add testmountoperation.

Looks good, works nicely. In order to be able to play with it, I have copied this code over to the URI plug-in in GIMP until we start to depend on the next GTK+ release.

There is one thing that I am unhappy with. In GIMP we are dealing with multiple processes and the password dialog raised by the URI plug-in should be transient to the progress window that is running in the main GIMP process. We have code to do this in libgimpui but it needs access to the dialog. Would it hurt much to add gtk_mount_operation_get_dialog()? We could then connect to "is-showing", retrieve the dialog and make it transient for the foreign window.

(In reply to comment #16)
> Would it hurt much
> to add gtk_mount_operation_get_dialog()? We could then connect to "is-showing",
> retrieve the dialog and make it transient for the foreign window.

For reasons stated in comment 2 you cannot assume the dialog runs in-process (it does right now but not in the future). What would work, however, is somehow the XID of the window you want it to be transient for (it's what I do for the PolicyKit-gnome auth dialogs).

So I think this is possible but the API would have to be a bit different.

Certainly it makes more sense to set the parent window on the GtkMountOperation object, and then it will use that to automatically set the transient for on the dialogs it shows?

Alexander, this doesn't work for out-of-process parent windows, at least not with the current API.

Why? Can't you get the xid of the out-of-process window?

Documentation has been added. 
If things are still unclear on the out-of-process front, please open another bug to track that.