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

[Richard Hult <richard imendio com>]

Hi, 

Thanks for the huge patch ;)

> Index: src/planner-project-properties.c
> ===================================================================
> RCS file: /cvs/gnome/planner/src/planner-project-properties.c,v
> retrieving revision 1.7
> diff -u -b -B -p -r1.7 planner-project-properties.c
> --- a/src/planner-project-properties.c  2 May 2004 16:21:28
> -0000       1.7
> +++ b/src/planner-project-properties.c  9 May 2004 19:32:37 -0000
> 
> @@ -52,6 +53,48 @@ typedef struct {
>         GtkWidget     *remove_property_button;
>  } DialogData;
>  
> +/* Start of REDO/UNDO structures */
> +
> +typedef struct {
> +       PlannerCmd   base;
> +       
> +       PlannerWindow     *window;

Is this used anywhere? Couldn't find anything, should probably be
removed from all the cmds.

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

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

> @@ -971,6 +1062,39 @@ mpp_property_removed (MrpProject  *proje
>         g_free (find_data);
>  }
>  
> +static void
> +mpp_property_changed (MrpProject  *project, 
> +                   GType        object_type,
> +                   MrpProperty *property,
> +                   GtkWidget   *dialog)
> +{
> +       DialogData       *data = DIALOG_GET_DATA (dialog);
> +       GtkTreeModel     *model;
> +       PropertyFindData *find_data;
> +       
> +       if ( object_type == MRP_TYPE_PROJECT) {

No space after "(".

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

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

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

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

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