Re: Mahjongg Patch

[Havoc Pennington <hp redhat com>]

Hi,

Thanks for doing the patch, and for having the courage to post it for
people to look at, unlike all the weenies out there. ;-)

Issues I saw are appended.

Havoc

Josh Barrow <drleary mac com> writes:
> +#define GTK_DISABLE_DEPRECATED
> +#define G_DISABLE_DEPRECATED
> +#define GNOME_DISABLE_DEPRECATED

It might have been easier to put these in the Makefile.am CFLAGS
(-DG_DISABLE_DEPRECATED etc.) instead of in each file.

> +	 /* There isn't a GTK_STOCK_MENU_BLANK, what should I use here? */
> +	 /*

Why is it using blank instead of simply no image at all?

> -                        gtk_signal_connect (GTK_OBJECT(item), "activate",
> +                        g_signal_connect (GTK_OBJECT(item), "activate",
>                                              (GtkSignalFunc)set_tile_selection, s); 

Hmm, GtkSignalFunc should be marked deprecated. Right thing is
G_CALLBACK(set_title_selection). Bugzilla on GtkSignalFunc would be good.

I would personally change GTK_OBJECT() to G_OBJECT(), most people
would probably just remove GTK_OBJECT().

> +                        mb = gtk_message_dialog_new (GTK_WINDOW (window),
> +						     GTK_DIALOG_MODAL | GTK_DIALOG_DESTROY_WITH_PARENT,
> +					       	     GTK_MESSAGE_INFO,
> +						     GTK_BUTTONS_OK,
> +						     "Sorry, there are no more moves left.");
>                          gtk_widget_show (mb); 


I don't think this dialog gets destroyed if you click OK. I usually do
this:

  g_signal_connect (G_OBJECT (dialog), "response",
                    G_CALLBACK (gtk_widget_destroy), NULL);  

  
>  	pref_dialog = gtk_dialog_new ();
>  	d = GTK_DIALOG(pref_dialog);
> - 	gtk_signal_connect (GTK_OBJECT(pref_dialog), "close",
> + 	g_signal_connect (GTK_OBJECT(pref_dialog), "close",
>  			    (GtkSignalFunc)pref_cancel, NULL); 
>

There's no "close" signal on GtkDialog, this must warn at runtime.
  
> -static void prefs_clicked_callback(GnomeDialog * dialog, gint button_number, 
> +static void prefs_clicked_callback(GtkWidget * dialog, gint button_number, 
>  			gpointer data)
>  {
>          switch (button_number) {
> @@ -1151,13 +1141,13 @@
>                  apply_preferences();
>                  break;
>          };


Shouldn't the "case" statements in the switch have become some sort of
enum value instead of the ints they probably were in the original?

> +	pref_dialog = gtk_dialog_new_with_buttons ("Preferences",
> +						    GTK_WINDOW (window),
> +						    GTK_DIALOG_MODAL | GTK_DIALOG_DESTROY_WITH_PARENT,
> +						    GTK_STOCK_CANCEL,
> +						    GTK_RESPONSE_CANCEL,
> +						    GTK_STOCK_OK,
> +						    GTK_RESPONSE_OK,
> +						    NULL);
>  
...
> +        d = GTK_DIALOG(pref_dialog);
> +        g_signal_connect(GTK_OBJECT(pref_dialog), "clicked",
>                             GTK_SIGNAL_FUNC(prefs_clicked_callback),
>                             NULL);

No "clicked" signal on GtkDialog, need "response" - another runtime
warning, right? No committing code that warns! ;-) (not on purpose
anyway ;-)

>                     
> -        gtk_signal_connect (GTK_OBJECT(pref_dialog), "close",
> +        g_signal_connect (GTK_OBJECT(pref_dialog), "close",
>  			    (GtkSignalFunc)pref_cancel, NULL); 
>  

No "close" either, only "destroy"
  
> -	gnome_dialog_set_parent(d, GTK_WINDOW(window));
>          gtk_widget_show_all (pref_dialog);
> +
> +	response = gtk_dialog_run (GTK_DIALOG (pref_dialog));
> +
> +	if (response == GTK_RESPONSE_OK)
> +	{
> +		apply_preferences();
> +	}
> +        pref_cancel(NULL, NULL);
>  }


How does this interact with the callbacks that were also connected?
(Assuming they're changed to be connected to signals that exist?)

>                  switch ((game_state)data) 
>                          {
>                          case RESTART_GAME : 
> -                                confirm_text = "Really restart this game ?"; 
> +                                confirm_text = "Really restart this game?"; 
>                                  break;
>                          case QUIT_GAME : 
> -                                confirm_text = "Really exit Gnome Mahjongg ?"; 
> +                                confirm_text = "Really exit Gnome Mahjongg?"; 
>                                  break;
>                          case NEW_GAME:
>                          case SELECT_GAME:
> -                                confirm_text = "Really start a new game ?";
> +                                confirm_text = "Really start a new game?";
>                                  break;
>                          default: 
> -                                confirm_text = "Serious internal error";
> +                                confirm_text = "Serious internal error!";
>                                  break;
>                          }

Not your fault, but shouldn't all those strings have _() for
translation, while we're changing stuff?

> +
> +			gnome_app_ok_cancel_modal (GNOME_APP(window), confirm_text,
> +                                           	   confirm_callback,
> data);

Wow, this dumb function should have been deprecated. The code should
be rewritten probably to just use a dialog with more descriptive
buttons, to match the UI guidelines.
  
> -void seed_dialog_clicked_cb (GnomeDialog * dialog, gint button_number, 
> +void seed_dialog_clicked_cb (GtkWidget * dialog, gint button_number,
>                               gpointer data)
>  {
>          switch (button_number) {
> -        case 0: /* OK button */
> +        case 0: /* Cancel Button */
> +                break;
> +        case 1: /* OK button */
>                  srand (atoi (gtk_entry_get_text (GTK_ENTRY (data))));
>                  new_game ();
>                  break;
> -
> -        case 1: /* Cancel Button */
> -                break;
> -

Again, if we're now using GtkDialog there should be symbolic names
such as GTK_RESPONSE_WHATEVER instead of the bare integers.

However looking later it appears this callback isn't used anymore, so
there's probably a warning about unused static function to be cleaned
up by nuking this function.

  	
> -	gtk_signal_connect (GTK_OBJECT (tiles[i].canvas_item), "event",
> +	g_signal_connect (GTK_OBJECT (tiles[i].canvas_item), "event",

                          G_OBJECT or nothing rather than GTK_OBJECT

>  	gtk_widget_push_colormap (gdk_rgb_get_cmap ());

This line can go away, though those functions aren't deprecated,
they are useless in this context.
  
>  	gtk_widget_pop_colormap ();

Clearly this line dies at the same time.

> +					     "Sorry, I can't find\na
> playable configuration.");

You lost the _() for translation on this string when redoing the code.

Havoc