Re: [Planner Dev] Planner enhancement - Advanced link tasks dialog.

[Richard Hult <richard imendio com>]

On tor, 2004-04-15 at 02:11 +0100, Lincoln Phipps wrote:
> Attached is the latest incarnation of the "Advanced link tasks dialog"
> thats been discussed recently.

Cool!

A few comments:

Please wait with the icon, same comment as for the others.

The tooltips that were changed to "Quickly ..." should be left like it
was, IMO.

I'm not sure about the wording "Advanced Link Tasks", sounds a bit odd
to me (should be something like advanced linking to be correct english,
but that's not good either). Maybe we can just have "Link Tasks...", not
sure if that's a good idea though.


+static MrpRelationType
+planner_task_link_input_dialog_get_selected (GtkWidget *option_menu)
+{
+       GtkWidget *menu;
+       GtkWidget *item;
+       MrpRelationType  *ret;
+
+       menu = gtk_option_menu_get_menu (GTK_OPTION_MENU (option_menu));
+       if (!menu) {
+               return 0;
+       }
+
+       item = gtk_menu_get_active (GTK_MENU (menu));
+
+       ret = g_object_get_data (G_OBJECT (item), "data");
+
+       return (GPOINTER_TO_INT ( ret ));
+}

This isn't right, it should be 

MrpRelationType type;

type = GPOINTER_TO_INT (g_object_...)

return type;

I'd also like to minimize the amount of long multiline comments, no need
to retell what the code does, we should have them when we need to
explain some rationale or workaround or FIXME. Otherwise the important
stuff tend to drown a bit.


For things like:

+       /*      break;  we fall through */
+
+       case GTK_RESPONSE_DELETE_EVENT:
+       case GTK_RESPONSE_CANCEL:
+               gtk_widget_destroy (dialog);
+               break;

I'd really prefer to put the destroy outside the switch since it should
always be done. Minimizes the risk of bugs if/when the fallthrough is
changed later on.

There are also a few style issues, like no space before "(" and no space
after ",". I'm sorry I can't point to the specific occurrences, I can
probably fix those when applying the patch if you don't feel like
looking for them.

Oh, one more thing, try to keep the source list in the makefile sorted,
and don't forget to add the new dialog c + glade files to po/POTFILES.in

Otherwise all nice :)
Thanks!!
Richard

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