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

[Alvaro del Castillo <acs lambdaux com>]

Hi!

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));
> 

Hmmm, g_value_unset doesn't free the string memory?

> This will leak, no need for the strdup.
> 
> That's it, but I have a few general comments:
> 

Great!

> 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_? 

Hmmm, so the changes won't be done in the variable until de focus get
put of the field? I haven't followed this way because it was to change
the way planner works in several places: don't change the variable until
de focus gets out of the field. Yes, with this change, it will be easy
for the undo system to work. I can make this changes in Planner, but to
be consistent we need to change it in several places. What do you think?

> Also the g_object_set/get_data should be done by adding the
> necessary fields to DialogData instead, IMO.
> 

Ok, I have done it this way because it is a very short live variable and
associated with a very specific field so ... but it is clean to put it
in DialogData. My fear is DialogData getting complex.

> 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. 

I have used it because if we use specific methods for example, for all
the different properties with different type, we will have lots of code
that do very similar things but using different value type.

> 
> 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.
> 

Sure, I have to check it, but I have the feeling that we will duplicate
a lot of very similar code.

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

This is what I have in mind: use as little code as possible to implement
things. But you know, I have followed and maybe, it is time to rethink
it a little.

> 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.
> 

Ok, I will commit and start to think about the above issues.

Thanks richard!

Cheers

> Thanks a lot!
> /Richard