Attached is a patch against cvs for
debian-installer/tools/cdebconf/src/modules/frontend/gtk/gtk.c
that corrects a number of the problems discussed below. Unfortunately,
I still can't test, as I mentioned in an earlier email, so I would
appreciate if someone could verify this patch before applying it.
On Thu, Sep 05, 2002 at 02:50:26PM +0200, Martin Sj?gren wrote:
> tor 2002-09-05 klockan 02.31 skrev Michael Cardenas:
>
> Now I've tested it, and I get a segfault after the password dialog.
> Let's have a look at some of the code. I hope my comments are useful ;)
>
Okay, some of these errors are flat out embarassing. Guess I was in a
hurry to finish this project this weekend.
> > + gPassword = malloc(strlen(entrytext));
> > + strcpy(gPassword, entrytext);
>
> Why not just strdup?
>
ok, fine.
> > + g_print ("delete event occurred\n");
> > +
> > + /* Change TRUE to FALSE and the main window will be destroyed with
> > + * a "delete_event". */
> > +
> > + return TRUE;
> > +}
>
> As I said, clicking Finish on a dialog box doesn't close the dialog box.
> I'm not sure this is good or bad. I think it gets cluttered fast.
>
The dialogs get closed for me when I hit the next button. I disabled
the close button to force you to use the next/back buttons. Are they
not working?
> > + g_signal_connect (G_OBJECT (boolButton), "clicked",
> > + G_CALLBACK (check_callback), NULL);
>
> Instead of global variables, why not pass in a pointer to ans instead of
> NULL? That's the 'data' argument in the callback, right?
>
As I stated in my original email with my patch, I was planning on
removing the globals, and I just used them to get the project
started. They have all been replaced with callback pointers and
structures where necessary now. But, as I cannot test, can you or
someone please verify that these changes work?
> > + if(gBool)
> > + {
> > + ans = 1;
> > + }
> > + else
> > + {
> > + ans = 0;
> > + }
>
> Meaning you wouldn't have to do this...
>
right, this was lame and it's gone now.
> > + choiceButtons = (GtkWidget **)malloc( (count * (sizeof(GtkWidget *)) + 1) );
>
> Hmm, shouldn't that be count * (sizeof(GtkWidget*) + 1)? You want n+1
> pointers, don't you?
>
I do. So, I think it should be
( (count + 1) * (sizeof(GtkWidget *)) )
> > + for(i = 0; i < count; i++)
> > + choiceButtons[i] = (GtkWidget *)malloc(sizeof(GtkWidget *));
>
> Eh? Why do you want to do this? You want to use gtk_*_new functions to
> do this.
>
This is lame. I was thinking that I wanted to allocate space to hold
each of the pointers, but of course, the original call does this. Doh!
> > + choiceButtons[count + 1] = 0;
>
> And this should be [count], since it's zero-based, right?
>
Yes, another lame mistake. Been writing rules files and shell scripts
too much lately!
> > + choiceButtons[i] = gtk_check_button_new_with_label ( (gchar *)choices[i] );
>
> And you "overwrite" the pointers here, so your malloc was pretty
> unnecessary in the first place :)
>
right.
> > + g_signal_connect (G_OBJECT (choiceButtons[i]), "clicked",
> > + G_CALLBACK (multicheck_callback), choiceButtons);
>
> This is where you'd want a higher-level language so you could pass in a
> list of pairs instead <wink> Seriously, a list of structs with both
> widget and result value might be nice?
done.
>I don't know how allergic others
> are to global variables, but I'm pretty damn allergic ;)
me too. I wanted to see if anyone had any pointers on this "c file as
class" thing. I was thinking maybe I could put the data in the
frontend_module struct or something. Silly me.
>Another idea
> would be to actually create our own full-blown gtk2 type and create an
> object for it to make it even more object-oriented. Just an idea :)
>
this may be overkill. I don't have any aversion to keeping it close to
c.
> > + for (i = 0; i < count; i++)
> > + free(choiceButtons[i]);
>
> This is where it segfaults.
>
Which makes perfect sense now because I'm freeing the widget's created
by gtk. I would expect it to freak out.
> > + //add actual password field
> > + entry = gtk_entry_new ();
> > + gtk_entry_set_max_length (GTK_ENTRY (entry), 50);
> > + g_signal_connect (G_OBJECT (entry), "activate",
> > + G_CALLBACK (passwd_callback),
> > + entry);
>
> Err. Why pass in entry here? Won't entry be 'widget' in the callback?
>
Another stupid mistake. fixed.
> > +
> > + strcpy(passwd, gPassword);
>
> This segfaults if gPassword is NULL.
>
True. Fixed. Also, should I add some logic as to what a valid password
is? or is the template capable of handling that?
>
> Many of the comments for multiselect apply here too.
>
> > +static int gtkhandler_string(struct frontend *obj, struct question *q)
>
> And here, for passwd.
>
Applied to both.
> > + text = gtk_text_buffer_get_text(buffer, &start, &end, FALSE);
> > +
> > + printf("text captured: %s.\n", text);
> > +
> > + strcpy(buf, text);
>
> This segfaults too. But this time I did enter text!
>
I'll have to look into why. The textbuffer is tricky. Once I get my
test environment going, I'll be able to debug this.
Thank you very much for all of your comments and for your close
attention to detail.
michael
--
michael cardenas | lead software engineer | lindows.com | hyperpoem.net
"Every day, priests minutely examine the Dharma.
And endlessly chant complicated sutras.
Before doing that, though, they should learn
How to read the love letters sent by the wind, and rain, the snow and moon."
- Ikkyu
Index: gtk.c
===================================================================
RCS file: /cvs/debian-boot/debian-installer/tools/cdebconf/src/modules/frontend/gtk/gtk.c,v
retrieving revision 1.1
diff -u -r1.1 gtk.c
--- gtk.c 5 Sep 2002 12:30:23 -0000 1.1
+++ gtk.c 6 Sep 2002 05:46:54 -0000
@@ -67,12 +67,12 @@
#define _(x) x
#endif
-int gBool;
-int gSelectNum;
-char *gPassword;
-int gMultiChoices[100];
-gboolean gBack, gNext;
-GtkWidget *gRadio;
+struct multicheck_data
+{
+ int multiChoices[100];
+ GtkWidget **choices;
+};
+
/*
* Function: passwd_callback
@@ -82,14 +82,17 @@
* Assumptions:
*/
void passwd_callback( GtkWidget *widget,
- GtkWidget *entry )
+ GtkWidget *data )
{
const char *entrytext;
- entrytext = gtk_entry_get_text (GTK_ENTRY (entry));
-
- gPassword = malloc(strlen(entrytext));
- strcpy(gPassword, entrytext);
+ char **callstring;
+
+ callstring = (char **)data;
+
+ entrytext = gtk_entry_get_text (GTK_ENTRY (widget));
+ *callstring = strdup(entrytext);
+
printf("Password captured.\n");
}
@@ -103,15 +106,19 @@
void check_callback( GtkWidget *widget,
gpointer data )
{
+ gboolean *gbool;
+
+ gbool = (gboolean *)data;
+
if (gtk_toggle_button_get_active (GTK_TOGGLE_BUTTON (widget)))
{
/* If control reaches here, the toggle button is down */
- gBool = 1;
+ *gbool = TRUE;
} else {
/* If control reaches here, the toggle button is up */
- gBool = 0;
+ *gbool = FALSE;
}
}
@@ -126,10 +133,13 @@
gpointer data )
{
int i, val;
- GtkWidget **choices;
gboolean done;
+ struct multicheck_data *call_data;
+ GtkWidget **choices;
+
+ call_data = (struct multicheck_data *)data;
+ choices = call_data->choices;
- choices = (GtkWidget **)data;
i = 0;
done = FALSE;
@@ -149,7 +159,7 @@
{
if(*choices == widget)
{
- gMultiChoices[i] = val;
+ call_data->multiChoices[i] = val;
done = TRUE;
}
@@ -170,12 +180,15 @@
void select_callback( GtkWidget *widget,
gpointer data )
{
+ GtkWidget **radio;
+
+ radio = (GtkWidget **)data;
if (gtk_toggle_button_get_active (GTK_TOGGLE_BUTTON (widget)))
{
/* If control reaches here, the toggle button is down */
printf("selected: %p\n", widget);
- gRadio = widget;
+ *radio = widget;
} else {
@@ -187,39 +200,6 @@
/*
- * Function: choice_callback
- * Input: none
- * Output: none
- * Description: respond to button press and determing which button was pressed
- * Assumptions:
- */
-void choice_callback( GtkWidget *widget,
- gpointer data )
-{
- int i;
- GtkWidget **choices;
- gboolean done;
-
- choices = (GtkWidget **)data;
- i = 0;
- done = FALSE;
-
- while(*choices && !done)
- {
- if(*choices == widget)
- {
- gSelectNum = i;
- done = TRUE;
- }
-
- i++;
- choices++;
- }
-
-}
-
-
-/*
* Function: next
* Input: none
* Output: none
@@ -229,9 +209,6 @@
void next( GtkWidget *widget,
gpointer data )
{
- gNext = TRUE;
- gBack = FALSE;
-
g_print ("next\n");
gtk_main_quit();
}
@@ -240,14 +217,18 @@
* Function: back
* Input: none
* Output: none
- * Description: go back to previous question
- * Assumptions:
+ * Description: go back to previous question.
+ * Assumptions: the question handler loop will increment before continuing
+ * so move back two questions in order to move back one.
*/
void back( GtkWidget *widget,
gpointer data )
{
- gBack = TRUE;
- gNext = FALSE;
+ struct question **pq;
+
+ pq = (struct question **)data;
+
+ *pq = (*pq)->prev->prev;
g_print ("back\n");
gtk_main_quit();
@@ -334,7 +315,7 @@
{
backButton = gtk_button_new_with_label ("Back");
g_signal_connect (G_OBJECT (backButton), "clicked",
- G_CALLBACK (back), NULL);
+ G_CALLBACK (back), &q);
gtk_box_pack_start(GTK_BOX(box), backButton, FALSE, 5, 5);
gtk_widget_show (backButton);
}
@@ -358,75 +339,6 @@
/*
- * Function: getwidth
- * Input: none
- * Output: int - width of screen
- * Description: get the width of the current terminal
- * Assumptions: doesn't handle resizing; caches value on first call
- *
- * TODO: implement this to get the size of the X desktop
-
-static const int getwidth(void)
-{
- static int res = 80;
- static int inited = 0;
- int fd;
- struct winsize ws;
-
- if (inited == 0)
- {
- inited = 1;
- if ((fd = open("/dev/tty", O_RDONLY)) > 0)
- {
- if (ioctl(fd, TIOCGWINSZ, &ws) == 0)
- res = ws.ws_col;
- close(fd);
- }
- }
- return res;
-}
-*/
-
-
-/*
- * Function: wrap_print
- * Input: const char *str - string to display
- * Output: none
- * Description: prints a string to the screen with word wrapping
- * Assumptions: string fits in <500 lines
- * Simple greedy line-wrapper
-
-static void wrap_print(const char *str)
-{
-
- int i, lc;
- char *lines[500];
-
- lc = strwrap(str, getwidth() - 1, lines, DIM(lines));
-
- for (i = 0; i < lc; i++)
- {
- printf("%s\n", lines[i]);
- DELETE(lines[i]);
- }
-}
-*/
-
-/*
- * Function: texthandler_displaydesc
- * Input: struct frontend *obj - UI object
- * struct question *q - question for which to display the description
- * Output: none
- * Description: displays the description for a given question
- * Assumptions: none
- *
-static void texthandler_displaydesc(struct frontend *obj, struct question *q)
-{
- wrap_print(question_description(q));
- wrap_print(question_extended_description(q));
-}*/
-
-/*
* Function: gtkhandler_boolean
* Input: struct frontend *obj - frontend object
* struct question *q - question to ask
@@ -436,12 +348,12 @@
*/
static int gtkhandler_boolean(struct frontend *obj, struct question *q)
{
- int ans = -1;
int def = -1;
const char *defval;
GtkWidget *window;
GtkWidget *boolButton;
GtkWidget *hboxtop, *hboxbottom, *bigbox;
+ gboolean gbool;
defval = question_defaultval(q);
if (defval)
@@ -465,7 +377,7 @@
boolButton = gtk_check_button_new_with_label ( (gchar *)question_description(q) );
g_signal_connect (G_OBJECT (boolButton), "clicked",
- G_CALLBACK (check_callback), NULL);
+ G_CALLBACK (check_callback), &gbool);
gtk_box_pack_start(GTK_BOX(hboxtop), boolButton, FALSE, 5, 5);
gtk_widget_show (boolButton);
@@ -484,16 +396,7 @@
gtk_main ();
- if(gBool)
- {
- ans = 1;
- }
- else
- {
- ans = 0;
- }
-
- question_setvalue(q, (ans ? "true" : "false"));
+ question_setvalue(q, (gbool ? "true" : "false"));
return DC_OK;
}
@@ -518,16 +421,15 @@
GtkWidget *window;
GtkWidget *hboxtop, *hboxbottom, *bigbox;
GtkWidget **choiceButtons;
+ struct multicheck_data call_data;
+
+ memset(call_data.choices, 0, sizeof(call_data.multiChoices));
count = strchoicesplit(question_choices(q), choices, DIM(choices));
dcount = strchoicesplit(question_defaultval(q), defaults, DIM(defaults));
- choiceButtons = (GtkWidget **)malloc( (count * (sizeof(GtkWidget *)) + 1) );
-
- for(i = 0; i < count; i++)
- choiceButtons[i] = (GtkWidget *)malloc(sizeof(GtkWidget *));
-
- choiceButtons[count + 1] = 0;
+ choiceButtons = (GtkWidget **)malloc( (count + 1) * (sizeof(GtkWidget *)) );
+ choiceButtons[count] = 0;
if (dcount > 0)
for (i = 0; i < count; i++)
@@ -535,7 +437,7 @@
if (strcmp(choices[i], defaults[j]) == 0)
{
selected[i] = 1;
- gMultiChoices[i] = 1;
+ call_data.multiChoices[i] = 1;
}
//create the window for the question
@@ -548,13 +450,15 @@
add_common_buttons(window, hboxbottom, bigbox, q);
+ call_data.choices = choiceButtons;
+
for (i = 0; i < count; i++)
{
//create a button for each choice here and init the global place to store them
choiceButtons[i] = gtk_check_button_new_with_label ( (gchar *)choices[i] );
g_signal_connect (G_OBJECT (choiceButtons[i]), "clicked",
- G_CALLBACK (multicheck_callback), choiceButtons);
+ G_CALLBACK (multicheck_callback), &call_data);
gtk_box_pack_start(GTK_BOX(hboxtop), choiceButtons[i], FALSE, 5, 0);
gtk_widget_show (choiceButtons[i]);
@@ -581,7 +485,7 @@
for (i = 0; i < count; i++)
{
- if(gMultiChoices[i])
+ if(call_data.multiChoices[i])
{
if(j)
{
@@ -593,9 +497,8 @@
}
}
- for (i = 0; i < count; i++)
- free(choiceButtons[i]);
-
+ free(choiceButtons);
+
printf("%s", answer);
question_setvalue(q, answer);
@@ -662,6 +565,7 @@
char passwd[256] = {0};
GtkWidget *window, *hboxbottom, *hboxtop, *bigbox;
GtkWidget *entry;
+ char *callstring;
//create the window for the question
window = gtk_window_new (GTK_WINDOW_TOPLEVEL);
@@ -674,9 +578,10 @@
//add actual password field
entry = gtk_entry_new ();
gtk_entry_set_max_length (GTK_ENTRY (entry), 50);
+
g_signal_connect (G_OBJECT (entry), "activate",
G_CALLBACK (passwd_callback),
- entry);
+ &callstring);
gtk_box_pack_start (GTK_BOX (hboxtop), entry, TRUE, TRUE, 0);
gtk_widget_show (entry);
@@ -696,7 +601,10 @@
gtk_main();
- strcpy(passwd, gPassword);
+ if(callstring)
+ strcpy(passwd, callstring);
+ else
+ strcpy(passwd, "");
question_setvalue(q, passwd);
return DC_OK;
@@ -725,14 +633,15 @@
GtkWidget *hboxtop, *hboxbottom, *bigbox;
GtkWidget **choiceButtons;
GtkWidget *firstButton, *nextButton;
+ GtkWidget *radio;
+ int selectNum;
+
+ selectNum = 0;
count = strchoicesplit(question_choices(q), choices, DIM(choices));
dcount = strchoicesplit(question_defaultval(q), defaults, DIM(defaults));
- choiceButtons = (GtkWidget **)malloc( (count * (sizeof(GtkWidget *)) + 1) );
-
- for(i = 0; i < count; i++)
- choiceButtons[i] = (GtkWidget *)malloc(sizeof(GtkWidget *));
+ choiceButtons = (GtkWidget **)malloc( (count+1) * ( sizeof(GtkWidget *) ) );
choiceButtons[count + 1] = 0;
@@ -742,7 +651,7 @@
if (strcmp(choices[i], defaults[j]) == 0)
{
selected[i] = 1;
- gSelectNum = i;
+ selectNum = i;
}
//create the window for the question
@@ -771,12 +680,12 @@
choiceButtons[i] = nextButton;
g_signal_connect (G_OBJECT (nextButton), "clicked",
- G_CALLBACK (select_callback), choiceButtons);
+ G_CALLBACK (select_callback), &radio);
gtk_box_pack_start (GTK_BOX (hboxtop), nextButton, TRUE, TRUE, 0);
gtk_widget_show (nextButton);
- if(i == gSelectNum)
+ if(i == selectNum)
{
printf("setting default to %d\n", i);
GTK_WIDGET_SET_FLAGS (nextButton, GTK_CAN_DEFAULT);
@@ -803,9 +712,9 @@
while(*choiceButtons)
{
- if(*choiceButtons == gRadio)
+ if(*choiceButtons == radio)
{
- gSelectNum = i;
+ selectNum = i;
break;
}
else
@@ -815,22 +724,18 @@
}
}
- printf("selected: %d\n", gSelectNum);
+ printf("selected: %d\n", selectNum);
//once the dialog returns, handle the result
printf("freeing %d buttons\n", count);
- //FIXME: why does this crash?!
- /* for (i = 0; i < count; i++)
- {
- free(choiceButtons[i]);
- }*/
+ free(choiceButtons[i]);
- printf("selectnum: %d\n", gSelectNum);
+ printf("selectnum: %d\n", selectNum);
- if(gSelectNum >= 0 && gSelectNum < count)
+ if(selectNum >= 0 && selectNum < count)
{
- strcpy(answer, choices[gSelectNum]);
+ strcpy(answer, choices[selectNum]);
}
printf("answer: %s\n", answer);
@@ -856,6 +761,7 @@
char passwd[256] = {0};
GtkWidget *window, *hboxbottom, *hboxtop, *bigbox;
GtkWidget *entry;
+ char *callstring;
//create the window for the question
window = gtk_window_new (GTK_WINDOW_TOPLEVEL);
@@ -867,10 +773,11 @@
//add actual text entry field
entry = gtk_entry_new ();
+
gtk_entry_set_max_length (GTK_ENTRY (entry), 50);
g_signal_connect (G_OBJECT (entry), "activate",
G_CALLBACK (passwd_callback),
- entry);
+ &callstring);
gtk_box_pack_start (GTK_BOX (hboxtop), entry, TRUE, TRUE, 0);
gtk_widget_show (entry);
@@ -889,7 +796,10 @@
gtk_main();
- strcpy(passwd, gPassword);
+ if(callstring)
+ strcpy(passwd, callstring);
+ else
+ strcpy(passwd, "");
question_setvalue(q, passwd);
@@ -1045,8 +955,8 @@
return ret;
//I think we need to go back two because the loop
- if(gBack && !gNext)
- q = q->prev->prev;
+ //if(gBack && !gNext)
+ //q = q->prev->prev;
break;
}
Attachment:
pgpphJBJnYvWs.pgp
Description: PGP signature