Re: window state stuff

[Tim Janik <timj gtk org>]

On 23 Feb 2001, Havoc Pennington wrote:

> Hi,

ok, in principle the patch looks very good to me.
however, i managed to still catch a couple of things ;)


> struct _GdkEventWindowState
> {
>   GdkEventType type;
>   GdkWindow *window;
>   gint8 send_event;
>   GdkWindowState changed;

is this what the state should be changed to, or has the state
already changed to

>   GdkWindowState current;

and "changed" is just the old state.
if the former, that field should probably say new_state, if the latter
it should say old_state (or previous_state as the naming is in
GtkWidget::state_set).

> };

> +void
> +gdk_synthesize_window_state (GdkWindow     *window,
> +                             GdkWindowState unset_flags,
> +                             GdkWindowState set_flags)
> +{
> +  GdkEventWindowState temp_event;
> +  GdkWindowState old;
> +  
> +  g_return_if_fail (window != NULL);
> +  
> +  temp_event.window = window;
> +  temp_event.type = GDK_WINDOW_STATE;
> +  temp_event.send_event = FALSE;
> +  
> +  old = ((GdkWindowObject*)temp_event.window)->state;

sigh, please put a space after parenthesis (and owen prolly wants a
space before the asterisk).

> +gboolean
> +_gdk_wmspec_supported (GdkAtom property)

make this function public, progs like the tasklist prolly want to use it.
also it should be named _gdk_wmspec_supports() since you actively use it
for repetitive checks throughout the code.

> +{
> +  static GdkAtom wmspec_check_atom = 0;
> +  static GdkAtom wmspec_supported_atom = 0;
> +  static GdkAtom *atoms = NULL;
> +  static gulong n_atoms = 0;
> +  Atom type;
> +  gint format;
> +  gulong nitems;
> +  gulong bytes_after;
> +  Window xwindow;
> +  gulong i;
> +
> +  if (wmspec_check_window != None)
> +    {
> +      if (atoms == NULL)
> +        return FALSE;
> +
> +      i = 0;
> +      while (i < n_atoms)
> +        {
> +          if (atoms[i] == property)
> +            return TRUE;
> +          
> +          ++i;
> +        }
> +
> +      return FALSE;
> +    }
> +
> +  if (atoms)
> +    XFree (atoms);
> +
> +  atoms = NULL;
> +  n_atoms = 0;
> +  
> +  /* This function is very slow on every call if you are not running a
> +   * spec-supporting WM. For now not optimized, because it isn't in
> +   * any critical code paths, but if you used it somewhere that had to
> +   * be fast you want to avoid "GTK is slow with old WMs" complaints.
> +   */
> +  
> +  if (wmspec_check_atom == 0)
> +    wmspec_check_atom = gdk_atom_intern ("_NET_SUPPORTING_WM_CHECK", FALSE);
> +      
> +  if (wmspec_supported_atom == 0)
> +    wmspec_supported_atom = gdk_atom_intern ("_NET_SUPPORTED", FALSE);
> +  
> +  XGetWindowProperty (gdk_display, gdk_root_window,
> +		      wmspec_check_atom, 0, G_MAXLONG,
> +		      False, XA_WINDOW, &type, &format, &nitems,
> +		      &bytes_after, (guchar **)&xwindow);
> +
> +  if (type == None)
> +    return FALSE;
> +
> +  gdk_error_trap_push ();
> +
> +  /* Find out if this WM goes away, so we can reset everything. */
> +  XSelectInput (gdk_display, xwindow,
> +                StructureNotifyMask);
> +
> +  if (gdk_error_trap_pop ())
> +    return FALSE;
> +
> +  gdk_error_trap_push ();
> +  
> +  XGetWindowProperty (gdk_display, xwindow,
> +		      wmspec_check_atom, 0, G_MAXLONG,
> +		      False, XA_ATOM, &type, &format, &n_atoms,
> +		      &bytes_after, (guchar **)&atoms);
> +
> +  if (gdk_error_trap_pop ())
> +    return FALSE;
> +  
> +  wmspec_check_window = xwindow;
> +
> +  /* since wmspec_check_window != None this isn't infinite. ;-) */
> +  return _gdk_wmspec_supported (property);
> +}

you need to gdk_sync() before popping error traps, otherwise they don't
help a single bit.

> +static void
> +set_initial_hints (GdkWindow *window)
> +{
> +  GdkWindowObject *private;
> +  GdkAtom atoms[5];
> +  gint i;
> +  
> +  private = (GdkWindowObject*) window;
> +
> +  if (private->state & GDK_WINDOW_STATE_ICONIFIED)
> +    {
> +      XWMHints *wm_hints;
> +      
> +      wm_hints = XGetWMHints (GDK_WINDOW_XDISPLAY (window),
> +                              GDK_WINDOW_XID (window));
> +      if (!wm_hints)
> +        wm_hints = XAllocWMHints ();
> +
> +      wm_hints->flags |= StateHint;
> +      wm_hints->initial_state = IconicState;
> +      
> +      XSetWMHints (GDK_WINDOW_XDISPLAY (window),
> +                   GDK_WINDOW_XID (window), wm_hints);
> +      XFree (wm_hints);
> +    }
> +
> +  /* We set the spec hints regardless of whether the spec is supported,
> +   * since it can't hurt and it's kind of expensive to check whether
> +   * it's supported.
> +   */

most importantly, you also need to set the hints so they are in place
if the user fires up a _NET compliant wm during runtime.

> @@ -737,14 +809,36 @@
>    g_return_if_fail (window != NULL);
>  
>    private = (GdkWindowObject*) window;
> +
> +  /* You can't simply unmap toplevel windows. */
> +  switch (private->window_type)
> +    {
> +    case GDK_WINDOW_TOPLEVEL:
> +    case GDK_WINDOW_DIALOG:
> +    case GDK_WINDOW_TEMP: /* ? */
> +      gdk_window_withdraw (window);

so here you withdraw...

> +      return;
> +      break;
> +      
> +    case GDK_WINDOW_FOREIGN:
> +    case GDK_WINDOW_ROOT:
> +    case GDK_WINDOW_CHILD:
> +      break;
> +    }
> +  
>    if (!private->destroyed)
>      {
> -      private->mapped = FALSE;
> +      if (GDK_WINDOW_IS_MAPPED (window))
> +        gdk_synthesize_window_state (window,
> +                                     0,
> +                                     GDK_WINDOW_STATE_WITHDRAWN);

and send out wm state.

>  
> +      g_assert (!GDK_WINDOW_IS_MAPPED (window));
> +      
>        _gdk_window_clear_update_area (window);
>        
>        XUnmapWindow (GDK_WINDOW_XDISPLAY (window),
> -		    GDK_WINDOW_XID (window));
> +                    GDK_WINDOW_XID (window));
>      }
>  }
>  
> @@ -757,8 +851,17 @@
>    
>    private = (GdkWindowObject*) window;
>    if (!private->destroyed)
> -    XWithdrawWindow (GDK_WINDOW_XDISPLAY (window),
> -		     GDK_WINDOW_XID (window), 0);
> +    {
> +      if (GDK_WINDOW_IS_MAPPED (window))
> +        gdk_synthesize_window_state (window,
> +                                     0,
> +                                     GDK_WINDOW_STATE_WITHDRAWN);

eventhough you send out wm state from _withdraw directly already?

> +
> +      g_assert (!GDK_WINDOW_IS_MAPPED (window));
> +      
> +      XWithdrawWindow (GDK_WINDOW_XDISPLAY (window),
> +                       GDK_WINDOW_XID (window), 0);
> +    }
>  }
>  
>  void
> @@ -945,6 +1048,41 @@
>  }

> --- gtk/gtkwidget.c	2001/02/03 01:09:40	1.185
> +++ gtk/gtkwidget.c	2001/02/24 00:43:43
> @@ -99,6 +99,7 @@
>    CLIENT_EVENT,
>    NO_EXPOSE_EVENT,
>    VISIBILITY_NOTIFY_EVENT,
> +  WINDOW_STATE_EVENT,
>    DEBUG_MSG,
>    LAST_SIGNAL
>  };
> @@ -314,6 +315,7 @@
>    klass->focus_out_event = NULL;
>    klass->map_event = NULL;
>    klass->unmap_event = NULL;
> +  klass->window_state_event = NULL;
>    klass->property_notify_event = gtk_selection_property_notify;
>    klass->selection_clear_event = gtk_selection_clear;
>    klass->selection_request_event = gtk_selection_request;
> @@ -775,6 +777,14 @@
>  		    gtk_marshal_BOOLEAN__POINTER,
>  		    GTK_TYPE_BOOL, 1,
>  		    GTK_TYPE_GDK_EVENT);
> +  widget_signals[WINDOW_STATE_EVENT] =
> +    gtk_signal_new ("window_state_event",
> +		    GTK_RUN_LAST,
> +		    GTK_CLASS_TYPE (object_class),
> +		    GTK_SIGNAL_OFFSET (GtkWidgetClass, no_expose_event),
                                                       ^^^^^^^^^^^^^^^
                                                       BOOOOOOOOOOOM ;)
                                                       
> +		    gtk_marshal_BOOLEAN__POINTER,

if i'm not mistaken, GdkEvent is a boxed type, so use the
right marshaller here. i think i'll add extra checks to
TYPE_POINTER marshallers so people stop being so lax.

> +		    GTK_TYPE_BOOL, 1,
> +		    GTK_TYPE_GDK_EVENT);


>    if (window->frame)
>      gdk_window_show (window->frame);
>  }
> @@ -1398,7 +1420,8 @@
>    gtk_style_set_background (widget->style, widget->window, GTK_STATE_NORMAL);
>    if (window->frame)
>      gtk_style_set_background (widget->style, window->frame, GTK_STATE_NORMAL);
> -  
> +
> +  /* This is a bad hack to set the window background. */
>    gtk_window_paint (widget, NULL);

could you elaborate on _why_ this is a bad hack?

> +void
> +gtk_window_present (GtkWindow *window)
> +{
> +  GtkWidget *widget;
> +
> +  g_return_if_fail (GTK_IS_WINDOW (window));
> +
> +  widget = GTK_WIDGET (window);
> +
> +  if (widget->window != NULL)
> +    {
> +      gdk_window_show (widget->window);
> +      gdk_window_focus (widget->window,
> +                        gtk_get_current_event_time ());
> +
> +
> +      /* FIXME add desktop-moving stuff from WM spec */

and workarea setting stuff.

> +    }
> +
> +  gtk_widget_show (widget);
> +}

this doesn't seem right, you should first gtk_widget_show() the widget,
then, do the above gdk_window_ ops.

> Index: gtk/gtkwindow.h
> ===================================================================
> RCS file: /cvs/gnome/gtk+/gtk/gtkwindow.h,v
> retrieving revision 1.25
> diff -u -u -r1.25 gtkwindow.h
> --- gtk/gtkwindow.h	2001/01/08 17:04:17	1.25
> +++ gtk/gtkwindow.h	2001/02/24 00:43:43
> @@ -85,6 +85,11 @@
>    
>    guint has_frame : 1;
>  
> +  /* gtk_window_iconify() called before realization */
> +  guint iconify_initially : 1;
> +  guint stick_initially : 1;
> +  guint maximize_initially : 1;
> +  

i think having present_initially : 1; wouldn't be a too bad idea.

---
ciaoTJ