Re: [Planner Dev] Task/Resource and Project property redo/undo patch.

["lincoln phipps openmutual net" <lincoln phipps openmutual net>]

Richard Hult wrote:

> Hi, 
>
> Thanks for the huge patch ;)

The reason its so large is because of the large amount of set or read
of property values a number of times and thats just a whole bunch of case
statements. See later in this email.

> > +
> > +typedef struct {
> > +       PlannerCmd   base;
> > +       
> > +       PlannerWindow     *window;
>
>
> Is this used anywhere? Couldn't find anything, should probably be
> removed from all the cmds.

Youre probably right on the window - I'll check and remove
if I don't find it.

> > +       MrpProject        *project;
> > +       gchar            *name; 
> > +       MrpPropertyType   type;
> > +       gchar            *label;
> > +       gchar            *description;
> > +       GType             owner;
> > +       gboolean          user_defined;   
> > +} MrpPropertyCmdAdd;
>
>
> Indentation looks off here, all variables should be aligned. This goes
> for other places as well.

OK - will clean up.

> > +typedef struct {
> > +       PlannerCmd   base;
> > +       
> > +       PlannerWindow     *window;
> > +       MrpProject        *project;
> > +       gchar            *name; 
> > +       MrpPropertyType   type;
> > +       gchar            *label;
> > +       gchar            *description;
> > +       GType             owner;
> > +       gboolean          user_defined; 
> > +       gchar            *old_text;  
> > +} MrpPropertyCmdRemove;
> > +
> > +typedef struct {
> > +       PlannerCmd        base;
> > +
> > +       PlannerWindow     *window;
> > +       MrpProject        *project;
> > +       MrpProperty      *property;
> > +       gchar            *old_text;
> > +       gchar            *new_text;
> > +} MrpPropertyCmdValueEdited;
>
>
>
> > @@ -379,6 +461,11 @@ mpp_connect_to_project (MrpProject *proj
> >                                 G_CALLBACK (mpp_property_removed),
> >                                 dialog,
> >                                 0);
> > +       g_signal_connect_object (project,
> > +                                "property_changed",
> > +                                G_CALLBACK (mpp_property_changed),
> > +                                dialog,
> > +                                0);
> > }
> >
> > static void  
> > @@ -885,6 +972,10 @@ mpp_property_added (MrpProject  *project
> >                    MrpProperty *property,
> >                    GtkWidget   *dialog)
> > {
> > +       if (!dialog) {
> > +               return;  /* This happens when signal for
> > property-added wasn't intended for this dialog */
>
>
> Hm, is dialog really NULL ever here? Sounds like a bug if that's the
> case.

Ah - I can see the bug in mpp_property_added (); the
existing code tries to use dialog data before its checked
to see if its a dialog - I'll fix the current code.


> > +       
> > +       if ( object_type == MRP_TYPE_PROJECT) {
>
>
> No space after "(".

OK - will check formating.

> > @@ -1148,16 +1272,15 @@ mpp_add_property_button_clicked_cb (GtkB
> >                        type = mpp_property_dialog_get_selected (w);
> >
> >                        if (type != MRP_PROPERTY_TYPE_NONE) {
> > -                               property = mrp_property_new (name, 
> > +
> > +                               cmd = (MrpPropertyCmdAdd*)
> > mrp_property_cmd_add (data->main_window,
> > +                                                                                       data->project, 
> > +                                                                                       MRP_TYPE_PROJECT, 
> > +                                                                                       name, 
>
>
> It's a bit hard to see just from the patch, but if you're not using the
> cmd, then there's no need to assign it. Also goes for other places.

I thought that was our convention ?

> > +static void
> > +mrp_property_cmd_add_undo (PlannerCmd *cmd_base)
> > +{
> > +       MrpPropertyCmdAdd *cmd;
> > +               
> > +       cmd = (MrpPropertyCmdAdd*) cmd_base;
> > +
> > +       if (cmd->name != NULL) {
> > +
>
>
> Stray empty line, and also, is this check necessary? Will the name ever
> be NULL here? If we've already checked it in _do, it can't be here I
> guess.

I don't know how the underlying cmd manager stuff works and
wanted to check that we had valid data before proceeding
further into my code.

>       
>
> > +static 
> > +PlannerCmd *
> > +mrp_property_cmd_add   (PlannerWindow *window,
> > +                       MrpProject      *project,
> > +                       GType           owner,
> > +                       const gchar     *name,
> > +                       MrpPropertyType type,
> > +                       const gchar     *label,
> > +                       const gchar     *description,
> > +                       gboolean        user_defined)
> > +{
> > +       PlannerCmd      *cmd_base;
> > +       MrpPropertyCmdAdd  *cmd;
> > +
> > +       cmd = g_new0 (MrpPropertyCmdAdd, 1);
>
>
> You should use planner_cmd_new here and everywhere else instead.

Ah - OK - thats new - I'll check how newer undo code does this.

> > +static gboolean
> > +mrp_property_cmd_value_edited_do (PlannerCmd *cmd_base)
> > +{
> > +       MrpPropertyCmdValueEdited *cmd;
> > +       MrpPropertyType         type;
> > +       MrpProject              *project;
> > +       MrpProperty             *property;
> > +       gchar                   *new_text;
> > +       gfloat                  fvalue;
> > +       
> > +       cmd = (MrpPropertyCmdValueEdited*) cmd_base;
> > +       
> > +       project = cmd->project;
> > +       property = cmd->property;
> > +       new_text = cmd->new_text;
> > +       
> > +       if (!cmd->property) {
> > +               return FALSE;
> > +       }
>
>
> Is this check necessary?


Same reason as before - check what data we were given before using it.
.
>       
>
> > +       type = mrp_property_get_property_type (property);
> > +       switch (type) {
> > +       case MRP_PROPERTY_TYPE_STRING:
> > +               mrp_object_set (MRP_OBJECT (project),
> > +                               mrp_property_get_name (property), 
> > +                               new_text,
> > +                               NULL);
> > +               break;
>
>
> [...]
>
> Is this the same code as above? If it's used in more than one place, we
> should refactor and put it in a separate function and reuse it
> everywhere. Same for the other places where this is done.

And you are right ... ;)  It does need refactoring. That'll reduce
the size of this patch dramatically as I cut+paste quite a lot of
existing code many times over.

I propose to create the following routines in mrp-property.c,

const gchar *mrp_get_property_value_as_string (MrpProperty *property);
boolean     *mrp_set_property_value_from_string (MrpProperty *property, gchar *string);

and then refactor current code (and patch) to use that.

> OK, time to get some sleep now. The rest of the files are somewhat
> unrelated to this, even if they all contain bits of property code, so
> I'll review the rest later.
>
> Regards,
> Richard