Re: [PATCH]: awt menubars

[Thomas Fitzsimmons <fitzsim at redhat dot com>]

Hi,

It seems I didn't CC this to the list when I originally sent it to Kim.

On Thu, 2004-01-15 at 13:01, Kim Ho wrote:
> Hi,
> 
> This patch gets menu bars working.


Great!


> Adding, removing, and frame inset
> adjustments. MenuBar's menu "children" are notified corectly in the
> addNotify of the parent bar and ditto for the menuitem's menu
> "children". The addition/removal of the menu bar to the frame is handled
> by two separate methods and the frame's insets are adjusted (to make
> room for the bar) on a size allocate event from the menubar.
> 
> Cheers,
> 
> Kim
> 


OK, a few comments:


> 2004-01-15  Kim Ho  <kho@redhat.com>
>                                                                         
>         * gnu/java/awt/peer/gtk/GtkFramePeer.java (getMenuBarHeight):
>       Added MenuBarPeer parameter.
>         (removeMenuBarPeer): New native method.
>         (setMenuBar): Call remove if menu bar is null. Adjust insets
>         appropriately.
>         (postSizeAllocateEvent): New method. Called when menu bar size
>       is allocated. Adjust insets and redo layout.
>         (GtkFramePeer): Set menu bar during frame creation.
>         (postConfigureEvent): Adjust position and size to accomodate
>         menu bar.
>         * java/awt/Frame.java (setMenuBar): addNotify to create menu
>       bar.
>         * java/awt/Menu.java (addSeparator): Don't use static separator.
>         (addNotify): Create the peer if it doesn't exist and call
>       addNotify for the menu's items.
>         * java/awt/MenuBar.java (addNotify): Create this menu bar's
>       menus.
>         * java/awt/MenuItem.java (addNotify): Create the peer if it
>         doesn't exist.
>         * jni/gtk-peer/gnu_java_awt_peer_gtk_GtkMainThread.c (gtkInit):
>         Add class for gtkFramePeer and methodID for
>       postSizeAllocateEvents.
>         * jni/gtk-peer/gnu_java_awt_peer_gtk_GtkWindowPeer.c
>         (removeMenuBarPeer): New method. Remove menu bar on the current
>         frame.
>         (setMenuBarPeer): Add the menu bar to the current frame and the
>         callback for size-allocate events on the menu bar.
>         (getMenuBarHeight): Add menu bar parameter.
>         (menubar_resize_cb): New callback method for postSizeAllocate
>       events.        
>       * jni/gtk-peer/gtkpeer.h: New jmethodID postSizeAllocateEventID.
> 

> Index: ChangeLog
> ===================================================================
> RCS file: /cvs/gcc/gcc/libjava/ChangeLog,v
> retrieving revision 1.2557
> diff -u -r1.2557 ChangeLog
> --- ChangeLog   15 Jan 2004 03:41:47 -0000      1.2557
> +++ ChangeLog   15 Jan 2004 17:49:17 -0000
> @@ -1,3 +1,33 @@
> +2004-01-15  Kim Ho  <kho@redhat.com>
> +
> +       * gnu/java/awt/peer/gtk/GtkFramePeer.java (getMenuBarHeight): Added
> +       MenuBarPeer parameter.
> +       (removeMenuBarPeer): New native method.
> +       (setMenuBar): Call remove if menu bar is null. Adjust insets
> +       appropriately.
> +       (postSizeAllocateEvent): New method. Called when menu bar size is
> +       allocated. Adjust insets and redo layout.
> +       (GtkFramePeer): Set menu bar during frame creation.
> +       (postConfigureEvent): Adjust position and size to accomodate
> +       menu bar.
> +       * java/awt/Frame.java (setMenuBar): addNotify to create menu bar.
> +       * java/awt/Menu.java (addSeparator): Don't use static separator.
> +       (addNotify): Create the peer if it doesn't exist and call addNotify 
> +       for the menu's items.
> +       * java/awt/MenuBar.java (addNotify): Create this menu bar's menus.
> +       * java/awt/MenuItem.java (addNotify): Create the peer if it
> +       doesn't exist.
> +       * jni/gtk-peer/gnu_java_awt_peer_gtk_GtkMainThread.c (gtkInit):
> +       Add class for gtkFramePeer and methodID for postSizeAllocateEvents.
> +       * jni/gtk-peer/gnu_java_awt_peer_gtk_GtkWindowPeer.c
> +       (removeMenuBarPeer): New method. Remove menu bar on the current 
> +       frame.
> +       (setMenuBarPeer): Add the menu bar to the current frame and the 
> +       callback for size-allocate events on the menu bar.
> +       (getMenuBarHeight): Add menu bar parameter.
> +       (menubar_resize_cb): New callback method for postSizeAllocate events.
> +       * jni/gtk-peer/gtkpeer.h: New jmethodID postSizeAllocateEventID.
> +
>  2004-01-14  Kelley Cook  <kcook@gcc.gnu.org>
>  
>         * configure.in: Add in AC_PREREQ(2.13)
> Index: gnu/java/awt/peer/gtk/GtkFramePeer.java
> ===================================================================
> RCS file: /cvs/gcc/gcc/libjava/gnu/java/awt/peer/gtk/GtkFramePeer.java,v
> retrieving revision 1.8
> diff -u -r1.8 GtkFramePeer.java
> --- gnu/java/awt/peer/gtk/GtkFramePeer.java     13 Jan 2004 20:54:46 -0000      1.8
> +++ gnu/java/awt/peer/gtk/GtkFramePeer.java     15 Jan 2004 17:49:17 -0000
> @@ -1,5 +1,5 @@
>  /* GtkFramePeer.java -- Implements FramePeer with GTK
> -   Copyright (C) 1999, 2002 Free Software Foundation, Inc.
> +   Copyright (C) 1999, 2002, 2004 Free Software Foundation, Inc.
>  
>  This file is part of GNU Classpath.
>  
> @@ -53,16 +53,41 @@
>      implements FramePeer
>  {
>    int menuBarHeight = 0;
> -  native int getMenuBarHeight ();
> +  private MenuBarPeer menuBar;
> +  native int getMenuBarHeight (MenuBarPeer bar);
>  
>    native public void setMenuBarPeer (MenuBarPeer bar);
> +  native public void removeMenuBarPeer (MenuBarPeer bar);
>  
>    public void setMenuBar (MenuBar bar)
>    {
> -    if (bar == null)
> -      setMenuBarPeer (null);
> -    else
> -      setMenuBarPeer ((MenuBarPeer) bar.getPeer ());
> +    if (bar == null && menuBar != null)
> +    {    
> +      removeMenuBarPeer(menuBar); 
> +      menuBar = null;
> +      insets.top -= menuBarHeight;
> +      menuBarHeight = 0;      
> +      awtComponent.doLayout();
> +    }
> +    else if (bar != null)
> +    {
> +      if (menuBar != null)
> +        removeMenuBarPeer(menuBar);
> +      menuBar = (MenuBarPeer) ((MenuBar) bar).getPeer();
> +      setMenuBarPeer(menuBar);      
> +    }
> +  }
> +
> +  protected void postSizeAllocateEvent()
> +  {
> +    if (menuBar != null)
> +    {
> +      if (menuBarHeight != 0)
> +        insets.top -= menuBarHeight;
> +      menuBarHeight = getMenuBarHeight(menuBar);
> +      insets.top += menuBarHeight;
> +    }
> +    awtComponent.doLayout();
>    }
>  
>    public GtkFramePeer (Frame frame)
> @@ -74,6 +99,7 @@
>    {
>      // Create a normal decorated window.
>      create (GDK_WINDOW_TYPE_HINT_NORMAL, true);
> +    setMenuBar(((Frame) awtComponent).getMenuBar());
>    }
>  
>    public void getArgs (Component component, GtkArgList args)
> @@ -102,10 +128,27 @@
>      g.translate (-insets.left, -insets.top);
>      return g;
>    }
> -
> -  // FIXME: When MenuBars work, override postConfigureEvent and
> -  // setBounds to account for MenuBar dimensions.
> -
> +  
> +  protected void postConfigureEvent (int x, int y, int width, int height)
> +  {
> +    int frame_x = x - insets.left;
> +    int frame_y = y - insets.top + menuBarHeight;
> +    int frame_width = width + insets.left + insets.right;
> +    int frame_height = height + insets.top + insets.bottom - menuBarHeight;


There should probably be a comment before the previous line, making it
clear that if there is a menu bar in the frame, then the menu bar's
height is accounted for in the top inset and so menuBarHeight must be
subtracted from the total height (and if there is no menu bar then
menuBarHeight will be 0).  Otherwise this line seems strange, like the
frame's height depends on whether or not it has a menu bar.


> +    if (frame_x != awtComponent.getX()
> +        || frame_y != awtComponent.getY()
> +        || frame_width != awtComponent.getWidth()
> +        || frame_height != awtComponent.getHeight())
> +      {
> +        setBoundsCallback ((java.awt.Window) awtComponent,


You should import java.awt.Window rather than fully specifying it here. 
That way it's easy to tell what classes are used in a given file just by
checking the imports list.


> +                           frame_x,
> +                           frame_y,
> +                           frame_width,
> +                           frame_height);
> +      }
> +    awtComponent.validate();
> +  }
> +  
>    protected void postMouseEvent(int id, long when, int mods, int x, int y, 
>                                 int clickCount, boolean popupTrigger)
>    {
> Index: java/awt/Frame.java
> ===================================================================
> RCS file: /cvs/gcc/gcc/libjava/java/awt/Frame.java,v
> retrieving revision 1.18
> diff -u -r1.18 Frame.java
> --- java/awt/Frame.java 19 Sep 2003 19:27:58 -0000      1.18
> +++ java/awt/Frame.java 15 Jan 2004 17:49:18 -0000
> @@ -1,5 +1,5 @@
>  /* Frame.java -- AWT toplevel window
> -   Copyright (C) 1999, 2000, 2002 Free Software Foundation, Inc.
> +   Copyright (C) 1999, 2000, 2002, 2004 Free Software Foundation, Inc.
>  
>  This file is part of GNU Classpath.
>  
> @@ -342,6 +342,8 @@
>  setMenuBar(MenuBar menuBar)
>  {
>    this.menuBar = menuBar;
> +  if (menuBar != null)
> +    menuBar.addNotify(); 
>    if (peer != null)
>      ((FramePeer) peer).setMenuBar(menuBar);
>  }
> Index: java/awt/Menu.java
> ===================================================================
> RCS file: /cvs/gcc/gcc/libjava/java/awt/Menu.java,v
> retrieving revision 1.13
> diff -u -r1.13 Menu.java
> --- java/awt/Menu.java  11 Jun 2003 10:37:47 -0000      1.13
> +++ java/awt/Menu.java  15 Jan 2004 17:49:18 -0000
> @@ -1,5 +1,5 @@
>  /* Menu.java -- A Java AWT Menu
> -   Copyright (C) 1999, 2002 Free Software Foundation, Inc.
> +   Copyright (C) 1999, 2002, 2004 Free Software Foundation, Inc.
>  
>  This file is part of GNU Classpath.
>  
> @@ -294,7 +294,7 @@
>  public void
>  addSeparator()
>  {
> -  add(separator);
> +  add(new MenuItem("-"));


Shouldn't this call peer.addSeparator()?  We should eventually implement
GtkMenuPeer.addSeparator using GTK's separators, rather than hyphen menu
items.  That can be done as a separate patch though.


>  }
>  
>  /*************************************************************************/
> @@ -376,8 +376,14 @@
>  public void
>  addNotify()
>  {
> -  if (peer != null)
> +  if (peer == null)
>      peer = getToolkit().createMenu(this);
> +  java.util.Enumeration e = items.elements();
> +  while (e.hasMoreElements())
> +  {
> +    MenuItem mi = (MenuItem)e.nextElement();
> +    mi.addNotify();
> +  }    
>    super.addNotify ();
>  }
>  
> Index: java/awt/MenuBar.java
> ===================================================================
> RCS file: /cvs/gcc/gcc/libjava/java/awt/MenuBar.java,v
> retrieving revision 1.10
> diff -u -r1.10 MenuBar.java
> --- java/awt/MenuBar.java       2 Jan 2003 00:14:22 -0000       1.10
> +++ java/awt/MenuBar.java       15 Jan 2004 17:49:18 -0000
> @@ -1,5 +1,5 @@
>  /* MenuBar.java -- An AWT menu bar class
> -   Copyright (C) 1999, 2000, 2001, 2002 Free Software Foundation, Inc.
> +   Copyright (C) 1999, 2000, 2001, 2002, 2004 Free Software Foundation, Inc.
>  
>  This file is part of GNU Classpath.
>  
> @@ -263,6 +263,12 @@
>  {
>    if (getPeer() == null)
>      setPeer((MenuComponentPeer)getToolkit().createMenuBar(this));
> +  Enumeration e = menus.elements();
> +  while (e.hasMoreElements())
> +  {
> +    Menu mi = (Menu)e.nextElement();
> +    mi.addNotify();
> +  }
>  }
>  
>  /*************************************************************************/
> Index: java/awt/MenuItem.java
> ===================================================================
> RCS file: /cvs/gcc/gcc/libjava/java/awt/MenuItem.java,v
> retrieving revision 1.14
> diff -u -r1.14 MenuItem.java
> --- java/awt/MenuItem.java      4 Dec 2003 19:31:01 -0000       1.14
> +++ java/awt/MenuItem.java      15 Jan 2004 17:49:18 -0000
> @@ -1,5 +1,5 @@
>  /* MenuItem.java -- An item in a menu
> -   Copyright (C) 1999, 2000, 2001, 2002, 2003 Free Software Foundation, Inc.
> +   Copyright (C) 1999, 2000, 2001, 2002, 2003, 2004 Free Software Foundation, Inc.
>  
>  This file is part of GNU Classpath.
>  
> @@ -361,7 +361,7 @@
>  public void
>  addNotify()
>  {
> -  if (peer != null)
> +  if (peer == null)
>      peer = getToolkit ().createMenuItem (this);
>  }
>  
> Index: jni/gtk-peer/gnu_java_awt_peer_gtk_GtkMainThread.c
> ===================================================================
> RCS file: /cvs/gcc/gcc/libjava/jni/gtk-peer/gnu_java_awt_peer_gtk_GtkMainThread.c,v
> retrieving revision 1.12
> diff -u -r1.12 gnu_java_awt_peer_gtk_GtkMainThread.c
> --- jni/gtk-peer/gnu_java_awt_peer_gtk_GtkMainThread.c  13 Jan 2004 20:54:46 -0000      1.12
> +++ jni/gtk-peer/gnu_java_awt_peer_gtk_GtkMainThread.c  15 Jan 2004 17:49:19 -0000
> @@ -1,5 +1,5 @@
>  /* gtkmainthread.c -- Native implementation of GtkMainThread
> -   Copyright (C) 1998, 1999, 2002 Free Software Foundation, Inc.
> +   Copyright (C) 1998, 1999, 2002, 2004 Free Software Foundation, Inc.
>  
>  This file is part of GNU Classpath.
>  
> @@ -60,6 +60,7 @@
>  jmethodID postListItemEventID;
>  jmethodID postTextEventID;
>  jmethodID postWindowEventID;
> +jmethodID postSizeAllocateEventID;
>  
>  JNIEnv *gdk_env;
>  
> @@ -82,7 +83,7 @@
>    char *homedir, *rcpath = NULL;
>  /*    jclass gtkgenericpeer; */
>    jclass gtkcomponentpeer, gtkchoicepeer, gtkwindowpeer, gtkscrollbarpeer, gtklistpeer,
> -    gtkmenuitempeer, gtktextcomponentpeer, window;
> +    gtkmenuitempeer, gtktextcomponentpeer, window, gtkframepeer;
>  
>    NSA_INIT (env, clazz);
>  
> @@ -151,6 +152,8 @@
>                                       "gnu/java/awt/peer/gtk/GtkMenuItemPeer");
>    gtktextcomponentpeer = (*env)->FindClass (env,
>                                       "gnu/java/awt/peer/gtk/GtkTextComponentPeer");
> +  gtkframepeer = (*env)->FindClass (env,
> +                                    "gnu/java/awt/peer/gtk/GtkFramePeer");
>  /*    gdkColor = (*env)->FindClass (env, */
>  /*                             "gnu/java/awt/peer/gtk/GdkColor"); */
>  /*    gdkColorID = (*env)->GetMethodID (env, gdkColor, "<init>", "(III)V"); */
> @@ -193,6 +196,9 @@
>    postTextEventID = (*env)->GetMethodID (env, gtktextcomponentpeer,
>                                              "postTextEvent",
>                                              "()V");
> +  postSizeAllocateEventID = (*env)->GetMethodID (env, gtkframepeer,
> +                                                 "postSizeAllocateEvent",
> +                                                 "()V");


We've started putting lookup code like this in the GTK callback where
the JNI call is made.  See GtkFileDialogPeer's cancel_clicked callback
for an example.


>    global_gtk_window_group = gtk_window_group_new ();
>  }
>  
> Index: jni/gtk-peer/gnu_java_awt_peer_gtk_GtkWindowPeer.c
> ===================================================================
> RCS file: /cvs/gcc/gcc/libjava/jni/gtk-peer/gnu_java_awt_peer_gtk_GtkWindowPeer.c,v
> retrieving revision 1.12
> diff -u -r1.12 gnu_java_awt_peer_gtk_GtkWindowPeer.c
> --- jni/gtk-peer/gnu_java_awt_peer_gtk_GtkWindowPeer.c  13 Jan 2004 20:54:46 -0000      1.12
> +++ jni/gtk-peer/gnu_java_awt_peer_gtk_GtkWindowPeer.c  15 Jan 2004 17:49:19 -0000
> @@ -1,5 +1,5 @@
>  /* gtkwindowpeer.c -- Native implementation of GtkWindowPeer
> -   Copyright (C) 1998, 1999, 2002 Free Software Foundation, Inc.
> +   Copyright (C) 1998, 1999, 2002, 2004 Free Software Foundation, Inc.
>  
>  This file is part of GNU Classpath.
>  
> @@ -73,6 +73,8 @@
>                                             GdkEventProperty *event,
>                                             jobject peer);
>  
> +static void menubar_resize_cb (GtkWidget *widget, GtkAllocation *alloc, jobject peer);
> +


This line should be wrapped at 80 characters.


>  /*
>   * Make a new window.
>   */
> @@ -358,45 +360,61 @@
>  }
>  
>  JNIEXPORT void JNICALL
> -Java_gnu_java_awt_peer_gtk_GtkFramePeer_setMenuBarPeer
> +Java_gnu_java_awt_peer_gtk_GtkFramePeer_removeMenuBarPeer
>    (JNIEnv *env, jobject obj, jobject menubar)
>  {
> -  void *wptr, *mptr;
> -  GtkBox *box;
> +  void *wptr;
> +  GtkWidget *box;
> +  GtkWidget *mptr;
>  
> -  if (!menubar) return;
> +  wptr = NSA_GET_PTR (env, obj);
> +  mptr = NSA_GET_PTR (env, menubar);
> +  
> +  gdk_threads_enter ();
>  
> +  box = GTK_BIN (wptr)->child;
> +  gtk_container_remove (GTK_CONTAINER (box), GTK_WIDGET (mptr));  
> +  
> +  gdk_threads_leave();
> +}  
> +  
> +JNIEXPORT void JNICALL
> +Java_gnu_java_awt_peer_gtk_GtkFramePeer_setMenuBarPeer
> +  (JNIEnv *env, jobject obj, jobject menubar)
> +{
> +  void *wptr;
> +  GtkWidget *mptr;
> +  GtkWidget *box;
> +  jobject *gref = NSA_GET_GLOBAL_REF (env, obj);
> +  
>    wptr = NSA_GET_PTR (env, obj);
>    mptr = NSA_GET_PTR (env, menubar);
> +  
> +  gdk_threads_enter ();
>  
> -  if (!mptr) return; /* this case should remove a menu */
> +  g_signal_connect (G_OBJECT (mptr), "size-allocate", 
> +                    G_CALLBACK (menubar_resize_cb), *gref);    
> +  box = GTK_BIN (wptr)->child;             
> +  gtk_box_pack_start (GTK_BOX (box), mptr, 0, 0, 0);
> + 
> +  gtk_widget_show (mptr);
>  
> -  gdk_threads_enter ();
> -  box = GTK_BOX (GTK_BIN (wptr)->child);
> -  gtk_box_pack_start (box, GTK_WIDGET (mptr), 0, 0, 0);
> + 
>    gdk_threads_leave ();
>  }
>  
>  JNIEXPORT jint JNICALL
>  Java_gnu_java_awt_peer_gtk_GtkFramePeer_getMenuBarHeight
> -  (JNIEnv *env, jobject obj)
> +  (JNIEnv *env, jobject obj, jobject menubar)
>  {
> -  void *ptr;
> -  GList *children;
> -  jint height = 0;
> -
> -  ptr = NSA_GET_PTR (env, obj);
> +  GtkWidget *ptr;
> +  jint height;
> +  
> +  ptr = NSA_GET_PTR (env, menubar);
>  
>    gdk_threads_enter ();
> -  children = gtk_container_children (GTK_CONTAINER (GTK_BIN (ptr)->child));
> -  if (g_list_length (children) == 2)
> -    {
> -      GtkWidget *menubar = GTK_WIDGET (children->data);
> -      height = menubar->allocation.height;
> -
> -    }
> +  height = ptr->allocation.height;
>    gdk_threads_leave ();
> -
>    return height;
>  }
>  
> @@ -696,4 +714,12 @@
>                                 (jint) extents[1]); /* right */
>  
>    return FALSE;
> +}
> +
> +static void menubar_resize_cb (GtkWidget *widget, GtkAllocation *alloc, jobject peer)


This line needs to be wrapped.


> 
> 
> +{
> +  gdk_threads_leave();
> +  (*gdk_env)->CallVoidMethod (gdk_env, peer,
> +                              postSizeAllocateEventID);
> +  gdk_threads_enter();
>  }
> Index: jni/gtk-peer/gtkpeer.h
> ===================================================================
> RCS file: /cvs/gcc/gcc/libjava/jni/gtk-peer/gtkpeer.h,v
> retrieving revision 1.12
> diff -u -r1.12 gtkpeer.h
> --- jni/gtk-peer/gtkpeer.h      23 Dec 2003 19:24:00 -0000      1.12
> +++ jni/gtk-peer/gtkpeer.h      15 Jan 2004 17:49:19 -0000
> @@ -1,5 +1,5 @@
>  /* gtkpeer.h -- Some global variables and #defines
> -   Copyright (C) 1998, 1999 Free Software Foundation, Inc.
> +   Copyright (C) 1998, 1999, 2004 Free Software Foundation, Inc.
>  
>  This file is part of GNU Classpath.
>  
> @@ -400,6 +400,7 @@
>  extern jmethodID postListItemEventID;
>  extern jmethodID postTextEventID;
>  extern jmethodID postWindowEventID;
> +extern jmethodID postSizeAllocateEventID;
>  
>  extern jmethodID syncAttrsID;
>  extern jclass gdkColor;


Tom