Re: Patch: ghex improvements.

[Michael Meeks <michael ximian com>]

Hi David,

On Tue, 2003-09-23 at 12:29, David Hammerton wrote:
> I decided to fix a few of the huge deficiencies in ghex. I'm a big fan
> of khexedit when using kde, so I hacked up ghex to add a few of the best
> features to it.

	Great - this is really useful (to me at least), but also all people who
get to struggle with binary files now and then.

> However, I've never really done any gtk/gnome coding before.
>
> I'm pretty sure this patch is fine, but perhaps someone could review it?
> (I'm especially not totally sure of the ref counting thing, it makes me
> nervous.)

	The patch looks great;

+    /* Do I have to free this gtk box manually? */
+    vbox = gtk_vbox_new(FALSE, 0);

	No you don't; since you pack this into another container. To make life
easier for C programmers we have the concept of 'floating' widgets - ie.
these widgets have a 'sort of' magic 0 reference count; when you pack
them in a container - they are then owned properly by the container.
This saves doing: a = new_foo(); pack (a); unref (a); which would suck
slightly.

+    gtk_signal_emit_by_name(GTK_OBJECT(gh), "cursor_moved"); /* update
dialog etc */

	We tend not to comment what individual lines do - but rather comment
functions at their head to describe the behavior of that function; this
tends to encourage nice, small, neat, simple functions (at least that's
the hope).

+static void hex_dialog_destroy (GtkObject *object)
+{

	This method must chain to it's parent; otherwise the GtkContainer will
not get destroyed' and thus none of it's children will - ie. a huge
leak. It's usual to chain to the parent by having something like:

static GtkObjectClass *parent_class;

class_init (klass)
{
	parent_class = g_type_class_peek_parent (klass)
}

	And then in the overridden method do:

	parent_class->destroy (object); etc.

	However if this method does nothing but print - it's prolly best to bin
it, and upd. class_init.

	Otherwise it looks great to me. It'd be nice to have an option to turn
off the display below the hex/ascii pane in the main view though -
partly because that may save the documentation authors from some pain
but also, that vspace is sometimes useful to have hex in.

> So, if some gnome developer could review this patch, and commit it if
> its OK, that would be great! I've tested it pretty heavily.

	I guess fixing up those few things and sending the patch to Jaka or
Chema is the way to go.

> You can see a screenshot of the changes here:
> http://craz.net/pics/ghex2_craz.jpg

	Looks good indeed.

> I intend to be doing a few more patches for ghex at some stage in the
> future, where should I send the said patches?

	Hopefully Chema/Jaka can get you setup with a CVS account, and you can
hack off into the blue yonder :-) If they don't respond in a week, poke
me.

	Great work,

		Michael.

-- 
 michael ximian com  <><, Pseudo Engineer, itinerant idiot