Re: menu-patch-2

[Owen Taylor <otaylor redhat com>]

Tim Janik <timj@gtk.org> writes:

> from menu-patch-2:
> -gtk.defs: @MAINT@ makeenums.pl gtk-boxed.defs
> +gtk.defs: @MAINT@ makeenums.pl gtk-boxed.defs $(gtkinclude_HEADERS) ../gdk/gdktypes.h
> 
> this one really gave me a hard time. due to circular dependencies,
> the gtktypebuiltins.h didn't get rebuild and that caused all base types
> to be messed up which left me with warnings in gtksignal.c's collect
> params on *every* signal emission.
> so the baseline is, don't do that (at least not this way) ;)

The Makefiles generation of gtktypebuiltins.* is completely screwed in
any case. The above doesn't work, but the way we handle it in the
CVS tree is screwed too. The above was just a desperate attempt to try
and get it to work. (Especially with srcdir != builddir, it has
problems)
 
> some remarks:
> 
> the activation of menu items with submenus is screwed, as a result
> the radio items in testgtk's menu test don't get adjusted anymore if
> they contain submenus.

What is the point of radio menu items with submenus? I have no idea
how they are supposed to work, so didn't figure out that they were
wrong. Should menu items with submenus really be activatable? (I don't
think it is hard to fix up in any case - actually with the
"activate_item" signal, the default handler for "activate" could
be removed.)

> in item factory you retrive the accel group for menu shortcuts from
> gtk_accel_groups_from_object()->data. this is highly unreliable
> and should better be featured by the itemfactory itself, or at least
> be set via object-data.

"highly unreliable?" Only if people start adding miscellanous
accel groups to menus, which seems very unliky.

> tearing off a menu and then selecting it from the menubar again,
> doesn't take it off screen but merely renders it two times.
> i even managed to get some gdk warnings regarding a NULL
> window, and some "Menu not on screen".

I don't know about the warnings, but about it appearing in
two places, I think my response is best expressed in German:
 
 Und? 

;-) What are the possibilities:

 - The menu leaps from the places where you put the tearoff
   menu to the menu when you pull it down, then leaps back
   when you release the menu.

 - The menu item for a torn off menu is disabled or just doesn't
   work.   

 - The torn-off menu stays visible, and the menu pulls down
   in its normal place as well.

Of the three, I thought the third was by far the best, so
I implemented it. (Think GTK+ widgets can't appear two
places at once? Read the source. I would definitely class
the solution as gross-but-cool)

> i guess you don't destroy the temporary window correctly, or
> rely on ints existance where you shouldn't. also i think you
> should better catch the ::destroy signal on the tearoff window
> to reparent the menu. i need to look at your code, but i guess
> there are a few problems with multiple parent-relashion ships
> to window, or something like that...

Nope. The warnings probably come from some weirdness with
the way some code detects a window being popped up. But the
window is always only a child of one window, and the tearoff
window isn't destroyed. 

> also for some strange reason, the background color for insensitive
> items is gtk's normal background color now, which looks quite
> ugly, but i guess that is caused by the overlaying styles...

How could it be caused by the overlayed styles? Or have you
applied both sets of patches to the same tree? I haven't
tried that...

But wasn't the background color for insensitive items always
GTK's normal background color!?  In any case, that is how,
say insensitive buttons appear with the menu patches (which
don't affect them) and without the style patches.
 
> if gle is used to examine a tearoff window, i get segfaults in
> libc_free(), though it is possible that there's an error in gle
> as well, i guess that something doesn't work correctly with
> memory allocations on tearoffs.
> 
> hm, after examining the underline letter calculation in
> gtk_accel_label_refetch() i figured, where memory was screwed.
> after playing around with it a bit, i ended with the following:
> 
>       for (; slist; slist = slist->next)
>         {
>           entry = slist->data;
> 
>           if (entry->accel_flags & GTK_ACCEL_VISIBLE)
>             {
>               if ((entry->accel_group == accel_label->underline_group) &&
>                   (entry->accelerator_mods == accel_label->underline_mods))
>                 {
>                   gint uline_pos = -1;
> 
>                   if (!pattern)
>                     {
>                       gchar *p;
>                       gboolean seen_bound;
> 
>                       p = label->label;
>                       seen_bound = TRUE;
>                       while (*p)
>                         {
>                           gchar c;
> 
>                           c = tolower (*p);
>                           if (c == entry->accelerator_key)
>                             {
>                               if (uline_pos < 0 || seen_bound)
>                                 uline_pos = (guint) (p - label->label);
>                               if (seen_bound)
>                                 break;
>                             }
>                           seen_bound = c < 'a' || c > 'z';
>                           p++;
>                         }
>                     }
> which seems to work quite reliable even for "Save As".

Hmmm, if you replace

                          seen_bound = c < 'a' || c > 'z';

With:

 seen_bound = !islower(c)

It should handle 99% or so of the cases correctly. But it
seems a little odd to take the string where the translator
has put the underline where they want it, create an accelerator
from it, and have the AccelLabel try to guess where that 
accelerator is, when we could just easily create the pattern
with the ItemFactory to begin with...

Regards,
                                        Owen