Re: [Planner Dev] New patch for undo: Focus in - focus out undo

[Richard Hult <richard imendio com>]

On sön, 2004-04-18 at 13:03 +0000, Alvaro del Castillo wrote:
> Hi guys!

Hi!

Huge patch, dude ;)

+       g_value_set_string (&value, g_strdup (focus_in_name));

This will leak, no need for the strdup.

That's it, but I have a few general comments:

The focus in/out stuff seems a bit complex and hackish to me... Wouldn't
it be a lot simpler to just make the changes be done in focus out
_always_? Also the g_object_set/get_data should be done by adding the
necessary fields to DialogData instead, IMO.

Another thing that makes the undo stuff a bit unnecessarily complex is
the use of gvalues everywhere... Do we really need that in the dialogs
that don't use gvalues otherwise? I used it in the task-tree edited_cb
to begin with just because there is one callback that handles all cells,
and gvalues fits in there nicely. 

In those places where there's actually one callback/cmd per command,
then we should definitely use gchar *, gint, etc instead of gvalues. It
would make the code a lot clearer and simpler.

I realize that it might be a bit late to comment on that now though.
Just something to keep in mind...

The patch could be committed after the memleak is fixed. The other is
more on a "nice to have" level, and if we don't do it now, we should add
a bug on that for 0.13 because it would help maintainability of the code
base a lot in my opinion.

Thanks a lot!
/Richard

-- 
Imendio HB, http://www.imendio.com/