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

[Alvaro del Castillo <acs lambdaux com>]

Hi!

Commit done!

But I plan to continue talking with richard in order to modifiy the code
if it is possible make things cleaner and simpler.

Cheers

El dom, 18-04-2004 a las 21:36, Richard Hult escribió:
> 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