Re: Patch for question callback

[Alexander Larsson <alexl redhat com>]

On Thu, 2004-04-08 at 14:19, Mattias Eriksson wrote:
> Hi,
> 
> Here is the patch I have been working on to add a new question callback
> to gnome-vfs and libgnomeui. 
> The main purpose was to make the sftp module ask the user what to do
> when the authenticity of a host cant be determined. 
> As you can see I currently just pass the message from sftp to the user,
> this is not so nice since it is a bad message and it is not
> translatable. I'm not much of an usability guy so I need some help with
> the message, so anyone with such talent feel free to help out.

I haven't tried the patches yet, but here are some comments:


gnome-vfs:

+	ret_in->message = corba_string_or_null_dup(question_in->message);

Missing space

+		//FIXME
+		//CORBA_sequence_set_release (ret_in->choices, TRUE);
+
	
Hmmm? Why is this commented out?


+		question_in->choices = g_new(char *, corba_in->choices._length+1);

Missing space

+		for(cnt=0; cnt<corba_in->choices._length; cnt++){

missing spaces

+			question_in->choices[cnt] = g_strdup(corba_in->choices._buffer[cnt]);

missing space

+	g_message("Anser is:%d\n", question_out->answer);

Leftover debug spew

+	question_out->answer = corba_out->answer,TRUE;

Eh?

+	char *message;		/* A message explaining the question to the user or 
+				 * NULL if there is no message */

We should probably have a primary and secondary message as in the HIG
(See HIG, figure 3.12).

+	char **choices;		/* The list of choices the user have to choose from,
+						   the list must be NULL terminated.
+						   The choices will be displayes so that choises[0]
+						   will be to the left and choises[n] will be to the 
+						   right.
+						   The last choice in the list should be affirmative
+						   to comply with the button ordering in gnome */

I would prefer if the affermative option was first, and the gnome
handler just reversed that. That makes more sense both from the
callers perspective, and for e.g. converting the list to a list of
choices on the console.

+		question_out->answer = -1; /* Set a default value */

Need to set the default value somewhere else. This is only run when
the ask callback goes through corba from the server, not when the
callback is in-process. A better place would be in the default
libgnomeui callback.



libgnomeui:

+static GtkWidget *create_question_dialog(char *msg, char **choices) 

type on line before function name, missing space

+	dialog = gtk_message_dialog_new(NULL, 0, GTK_MESSAGE_QUESTION, GTK_BUTTONS_NONE, msg);

missing space

+		for (cnt=0; choices[cnt] != NULL; cnt++) {

We should reverse the choices here instead of in the caller.

+			/* Maybe we should define some gnome-vfs stockbuttons and 
+			 * then replace them here with gtk-stockbuttons */

Yeah. It would be nice if one could use stock gtk things in the
choices, and it would still work in a commandline app...

+			gtk_dialog_add_button(GTK_DIALOG(dialog), choices[cnt], cnt);			

Missing space


+	present_question_dialog_nonblocking(info);

Missing space



sftp-method:
+				//FIXME: Don't use message. Create a better translatable 
+				//		 message to present to the user.
+				in_args.message = buffer;

What about something like:

Primary:
Error while logging in to %s

Secondary:
The identity of the remote computer seems to have changed since the
last time it was accessed. This can happen in normal situations,
such as a computer being upgraded. However, there is also a
possibility that someone is maliciously impersonating the remote
computer.

The identity sent by the remote host is %s. If you want to be sure it
is safe to continue, contact the system administrator.

                        [Cancel Login] [Log In Anyway]


+				in_args.choices = g_new(char *, 3);

Missing space
in_args.choices is never freed. A better approach would be to have a
local variable like this:

char *choices[3];
in_args.choices = choices;

+				//FIXME: Add stuff to translate the strings.
+				in_args.choices[0] = "Cancel";
+				in_args.choices[1] = "Accept New Host Key";
+				in_args.choices[2] = NULL;

Just wrap the string like _("string to be translated")


+					} else if (out_args.answer == 0) {
+						g_io_channel_write_chars (tty_channel, "no\n", -1, &len, NULL);
+					} else {

Hmmm. What error do we return then? Maybe we should make sure its a
sane one, like GNOME_VFS_ERROR_ACCESS_DENIED.



=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=
 Alexander Larsson                                            Red Hat, Inc 
                   alexl redhat com    alla lysator liu se 
He's a globe-trotting Republican cop possessed of the uncanny powers of an 
insect. She's a cynical nymphomaniac single mother who dreams of becoming 
Elvis. They fight crime!