Created attachment 5985 [details] [review]
construct-time properties for GtkMessageDialog

There's one GTK_DIALOG() cast in there with a missing space before the
parens, otherwise patch looks OK to me. It would be nice if the
"message_type" prop were dynamic instead of CONSTRUCT_ONLY (for the
benefit of Glade), but not a showstopper IMO. The buttons property
doesn't really need to be dynamic since Glade can add buttons in the
same way it does for the GtkDialog base class, i.e. the buttons prop
is just a convenience thing.

Was that an OK to patch?

Here's a revised patch that makes "message_type" readable too.

Created attachment 6020 [details] [review]
revised gtkmessagedialog construct properties patch

Comments:
 
 - It's helpful to remove extraneous portions like the
   docs/ diffs from patches before submitting them.

 - _gtk_message_dialog_get_message_type doesn't need the
   _ prefix; that is only for non-static functions that
   shouldn't be exported. Conventions for static functions
   vary ... calling it "message_dialog_get_message_type()"
   would be fine.

 - if(strcmp(stock_id, GTK_STOCK_DIALOG_INFO) == 0) needs
   a space between the if and the (. See 
   pango/docs/TEXT/coding-style for details on more
   coding style issues.

 - get_message_type() should g_assert success, not g_warning();
   failure here indicates a bug in GTK+.

 - gtk_message_dialog_add_buttons() needs a doc comment
   and a .h file prototype if it is meant to be a public
   function.

 - message_dialog / dialog would be more traditional than
   dialog / dialog_base.

 - Using CONSTRUCT and CONSTRUCT_ONLY properties here as
   you have the patch will result in things being  
   set _twice_, once when gtk_type_new() is called, once
   afterwards. gtk_message_dialog() will need to be
   switched over to using g_object_new() and passing
   in the parameters.

If you make a new version of the patch with the 
g_object_new() and missing doc comment fixed, I'll take care of fixing
the small details and applying.

> It's helpful to remove extraneous portions like the
> docs/ diffs from patches before submitting them.

I had to delete the sgml files manually then do another cvs update.
Hopefully there's a good reason for putting generated files in cvs.

> gtk_message_dialog_add_buttons() needs a doc comment
> and a .h file prototype if it is meant to be a public
> function.

It's not meant to be public, because GtkButtonsType doesn't seem like
a nice way to add buttons expect as a convenience at instantiation.
And there was no apparent need for such a public function to be added.

> Using CONSTRUCT and CONSTRUCT_ONLY properties here as
> you have the patch will result in things being  
> set _twice_, once when gtk_type_new() is called, once
> afterwards.

I must still not understand this stuff. I thought that all properties
used in g_object_new() had to be CONSTRUCT or CONSTRUCT_ONLY. How else
would glib know that it should set the defaults for these but not for
other properties? However, I have removed these like you ask.

This patch should take care of everything else.

Created attachment 6026 [details] [review]
GtkMessageDialog properties patch v3

The doc tmpl files are not autogenerated, they are autoupdated;
you often can get fake diffs if you have the wrong gtk-doc
version, or you changed something and changed back, so unless
the diffs you have in that directory are related to what
you changed, it is often best to blow the directory away
and recheckout. But the point I was making was they had nothing
to do with your patch, so it's best edited out.

Re CONSTRUCT properties ... what I was saying that if you
make these CONSTRUCT properties, then you have to use them
in gtk_message_dialog_new(); not that they shouldn't be
construct properties.

Applied with formatting fixes and change back to
CONSTRUCT properties.

Thu Nov 15 11:59:35 2001  Owen Taylor  <otaylor@redhat.com>

	* gtk/gtkmessagedialog.c: Patch from Murray Cumming to add
	"message_type" and "buttons" CONSTRUCT properties. (#64069)