Re: Menu changes (fwd)

[Owen Taylor <otaylor redhat com>]

Tim Janik <timj@gtk.org> writes:

> hm, seems like this mail didn't get through...
> 
> ---
> ciaoTJ
> 
> ---------- Forwarded message ----------
> Date: Wed, 22 Jul 1998 19:25:22 +0200 (CEST)
> From: timj@localhost.
> To: gtk-devel-list@redhat.com
> Subject: Re: Menu changes
> 
> On Tue, 21 Jul 1998, Owen Taylor wrote:
> 
> > 
> > [ Cc'd to gtkdev since I suspect most people aren't subscribed
> >   to the new list yet ]
> > 
> > I've been working on extensive changes to menus in GTK+. 
> > 
> > The two major areas I've worked on so far are:
> > 
> >  - tearoff menus. (You add a GtkTearoffMenuItem as the first item
> >    in the menu)
> > 
> >  - Keyboard navigation. (Both with arrow keys, and using "underline
> >    accelerators). 
> > 
> > A preview patch is at:
> > 
> >  ftp://ftp.labs.redhat.com/pub/otaylor/menu-patch.gz
> 
> unfortunately, the patch is cluttered with marshalling things, it seems
> like gtk+-stock already was uptodte with the cvs version that stripped the
> GDK_EVENT stuff, therefore i couldn't really apllie it and just tried
> to read the diff as is. could you provide another patch that is synced, so i
> get a chance to actually try this stuff out? ;)

OK, new version is at:

  ftp://ftp.labs.redhat.com/pub/otaylor/menu-patch-2.gz
 
> > I haven't commited it yet, because I don't like everything
> > about the way things work yet.
> > 
> > Particular things I think need improvement:
> > 
> >  * Unless they make sense (as in the GIMP), changing entries to have
> >    no-modifier accelerators should be disabled, because they are
> >    too easy to set accidentally while navigating the menus.
> 
> i guess we can add a special hint, e.g. "<FrozenAccels>" or somesuch to the
> ->item_type, so we get stuff like
> "<CheckItem><FrozenAccels>".

Blech, no. We don't want to disable setting accelerators, we just
don't want people to set up accelerators accidentally when they
are trying to switch to a different menu or something - so I
was thinking we should have a way of disabling setting accelerators
when someone types an unmodified key. (And possible disable
it by default)
 
> >  * Currently, underline patterns are set by the AccelLabel code,
> >    which watches for accelerators in a given accelerator group
> >    with given modifiers. But this gets wrong "Save &As" displaying
> >    it as "S_ave As" instead of "Save _As". I think the ItemFactory
> >    code should probably just be setting a pattern for the AccelLabel,
> >    and then not set the VISIBLE flag for the corresponding 
> >    accelerators.
> 
> hm, the VISIBLE flag should still be TRUE. setting it to FALSE would mean
> to not draw the underline.

The point is, that having the AccelLabel draw the underline in response
to an accelerator being set just doesn't work (see the above 
"Save &As" example). So, I was thinking that the accelerator should
be invisible, and the underline should just be set on the AccelLabel
as a fixed pattern (through GtkLabel::pattern). 
 
> >  [ I'm using '&' instead of '_' because I think it is probably
> >    less common in actual menu text. ]
> 
> i'd like to use '_' instead, so i can have menu items like
> "Save & Exit". actually i never used '_' in an item name.

Perhaps, easy enough to change. \ escapes should probably be
supported. (Or perhaps '__' to produce a '_')

> >  * The way that menu items are activated and pop up their submenus
> >    is pretty screwy:
> > 
> >    - There is an "activate_item" signal that is triggered by the
> >      underline style accelerators. This does some special 
> >      handling with respect to submenus.
> > 
> >    - There is a gtk_menu_shell_activate_item() function which is
> >      used when the user hits Space or Return on an entry
> > 
> >    - There is a gtk_menu_show_select_item() function which is called
> >      when the user arrows onto an entry, which takes care of popping
> >      up submenus.
> > 
> >    - The default handler for GtkMenuItem::activate also has code
> >      to popup a submenu. (I think this could be removed now...)
> > 
> >    I need to rework this to be a bit more consistent and unified.
> 
> [...] i need to actually try this stuff out...
> 
> >   * Currently, there is a that problem when menus pop up underneath
> >     the cursor, they get selected. This needs to be fixed.
> > 
> > I also plan on working on displaying menus that are too big to
> > display on the screen. My current plan is that such menus will
> > pop up as array menus (multiple columns), and then if they
> > are torn off, change back to a single column with a scrollbar.
> 
> wouldn't it be easier to always have a scrollbar and have the normal
> policy for it (AUTOMATIC/ALWAYS)?

Scrollbars aren't very useful for normal pulldown menus. You have
to pull it down, release, then adjust the scrollbar, then click.
An array menu should be faster to access. (And getting the grab
code right for the scrolled window is a pain, though possible,
as I know from GtkCombo)
 
> > I'd appreciate if those who know GtkItemFactory and the menuing
> > code (in particular, Tim) would take a look at the patch and
> > see if they like what it is doing. Also, if people want to
> > try it out in a --disable-shared tree and see if they like
> > the way the keyboard navigation works. (The new "Item Factory"
> > test is a good place to look)
> 
> erm, what does "--disable-shared" have to do with this?

You shouldn't be installing this, and if you don't compile
with --disable-shared, you will link against your installed
version of GTK+. (On Linux, anyways). No?
 
> > 
> > Regards,
> >                                         Owen
> > 
> > The complete ChangeLog entry follows:
> > 
> 
> > 	* gtk/gtklabel.[ch]: Add support for a pattern arg - 
> >           which is a string. If an '_' appears in this string,
> > 	  the corresponding position in the label is underlined.
> 
> for the argument stuff:
>     case ARG_PATTERN:
> -      gtk_label_set_pattern (label, GTK_VALUE_STRING (*arg));
> +      gtk_label_set_pattern (label, GTK_VALUE_STRING (*arg) ? GTK_VALUE_STRING (*arg) : "");
>       break;

I don't think so. A NULL value for the pattern is perfectly legitimate
and handled properly in the code. (it is the default, in fact). It
saves a ton of 1 byte malloc allocations. Of course, some languages
can't represent a NULL string, but some can. (In Perl, it maps
naturally to 'undef')

>  also, an appropriate get_arg implementation needs to be there.

Ah, missed that.
 
> > 
> > 	* gtk/gtkmenu.[ch]: Make menus no longer a toplevel widget.
> > 	  Instead, they create a GtkWindow and add themselves
> > 	  to that. (When torn off, another new feature, they
> > 	  create another GtkWindow to hold the torn off menu)
> > 
> > 	  New function gtk_menu_set_tearoff_state()
> 
> regarding the binding sets, its easier to do:
> 
> +  binding_set = gtk_binding_set_by_class (class),
> -  binding_set = gtk_binding_set_new ("GtkMenu");
> -  gtk_binding_set_add_path (binding_set, GTK_PATH_CLASS,
> -                           "GtkMenu", GTK_PATH_PRIO_GTK);
> 
> gtk_binding_set_by_class() sets the path already, and i'd rather like
> to see all internal per-class bidings run through this function, and
> keep gtk_binding_set_add_path() for use from gtkrc.c only.

You see, we need docs on this stuff. Wait a second, I was writing
those... I've made this change in menu-patch-2
 
> about the tearoff_window, i think you should catch the delete_event for
> this window, though i might be wrong since i can't actuall try this out.

It is caught, but the signal handler is set up in GtkTearoffMenuItem.c
so you missed it. (Yes, there is a lot of weird dependency going on
between the files with this stuff, though no more so than there was
before in the menuing code)

Regards,
                                        Owen