Re: Table menu patch for GtkMenu

[Owen Taylor <otaylor redhat com>]

On Sat, 2003-07-12 at 11:22, Kristian Rietveld wrote:
> Hello,
> 
> Before I can land the new combo box in gtk+ HEAD, I need to get my happy
> fun table menu patch for GtkMenu approved. So I attached this patch. A
> good chunk of the code is based on the GtkTable size
> allocation/requisition code. I guess it's a pretty sane patch, but maybe
> some won't like the proposed API.

The API is basically fine, though I'd like to see some explanation
of what "gtk_menu_occupied" is for, but I have various large-scale
thoughts about the implementation:

 - I don't really like the "two different algorithms" way you
   have things currently. It's confusing, and it also means
   that toggle and image menu items don't work if you use
   the table-based API.

   I don't really see why the "normal" GtkMenu can't simply
   be a table-based menu with one column.

 - If you are only supporting homogenous tables, then I think
   a much simpler implementation is possible than what you 
   have now.

   You can do the requisition in one pass ... for children
   than span multiple rows/columns, you just do:

    requisition = 
     MAX (requisition, (child_requisition.width + n_spanned - 1) /
n_spanned);

 - Key shortcuts need to be done through the binding system,
   not hard coded in key press handlers.

 - It would be nice to have child properties for the attach
   values ... as well as completeness, it provides a way
   of querying those properties "for free".

 - FIXME's that just say "This is entirely broken" are in my
   experience a terrible idea. Because what is obvious
   to you now, won't be obvious to you when you next run
   into your comment. Every FIXME should say what needs 
   fixing.

 - The plural of 'child' is 'children'.

Regards,
						Owen