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

[Lincoln Phipps <lincoln phipps openmutual net>]

Richard Hult wrote:
> 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.

OK - will do that.
> 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.

Yes - maybe we do "Link Tasks" and "Link Tasks..." and I'll change the
verbs etc to LinkTasksDialog

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


OK - will clean this up. I cribed code from other parts of Planner
and tweaked where neccessary (like casting).
> 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.

I'll remove these.

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

OK - me being lazy. I like how case fall through but yes its a chance
of leaks so shouldn't experiment here like this.

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

I'll check this - I know this is a Planner style; it should be me who
fixes my patch to suite the style guides.

> 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

I didn't know about that or the po/POTFILES.in  - will add this too.

> Otherwise all nice :)
> Thanks!!
> Richard

I'm pleased as its a lot (for me) of new code that actually compiles
fine and works nicely. If I'm forwarding too many patches/enhancements
towards you for review then please email suggesting a revised schedule
of work. These first few patches really were to learn GNOME/Gtk/C stuff
so should be able to work faster/better and on more complex things
soon (like that Resource column sort issue :)

Rgds,
Lincoln.