Re: non-attached style detached in gtkinvisible.

[Owen Taylor <otaylor redhat com>]

Alexander Larsson <alla lysator liu se> writes:

> On 20 Dec 2000, Owen Taylor wrote:
> 
> > I'm not entirely happy with the way that this patch has little bits
> > and #ifdefs all over the GtkWindow code.
> >
> > Here's my idea about how to handle it in a cleaner way.
> >
[...]
> 
> Ok. How about this one then.

With the following comments, this looks OK to commit, though you need
API doc comments first.

I would suggest a prettying things up a little visually:

 - use GDK_STATE_PRELIGHT for unfocused windows and GTK_STATE_SELECTED for
   focused windows

 - Don't draw the close button as a shadow-out box - the two levels of 
   bevel are too much, especially with your narrow borders. I'd suggest
   a flat style->fg_gc[] empty rectangle with an x in it.

 - Up the font size for the title by a few points.
 
> @@ -1025,11 +1061,17 @@ gtk_window_show (GtkWidget *widget)
>        allocation.width  = width;
>        allocation.height = height;
>        gtk_widget_size_allocate (widget, &allocation);
> -
> -      if (GTK_WIDGET_REALIZED (widget))
> -	gdk_window_resize (widget->window, width, height);
> -      else
> +
> +      if (!GTK_WIDGET_REALIZED (widget))
>  	gtk_widget_realize (widget);
> +
> +      gtk_decorated_window_calculate_frame_size (window);

Hmmm, this isn't very nice, both from the perspective of keeping dependencies
between gtkwindow.c and gtkdecoratedwindow.c low and from the perspective
of keeping things working right.

It doesn't look like things work when you change the decorations after showing 
the window.

Maybe you should add a callback when the decorations (and title?) change
to your _gdk_window_set_child_handler () function?

> +      if (window->frame)
> +	gdk_window_resize (window->frame,
> +			   width + window->frame_left + window->frame_right,
> +			   height + window->frame_top + window->frame_bottom);

If you make gtk_window_set_frame_decorations() work on a realized widget,
then this can go away.

> +      gdk_window_resize (widget->window, width, height);

Hmmm, do we really want to be doing this in the case where the window
is not previously realized? I don't think so. There is no optimization
away of this call for toplevel windows in the case of sizes not changing
because the size could have changed inbetween. You probably need
a gboolean was_realized variable.

> @@ -1092,6 +1136,9 @@ gtk_window_unmap (GtkWidget *widget)
>    window->use_uposition = TRUE;
>    window->resize_count = 0;
>    window->handling_resize = FALSE;
> +
> +  if (window->frame)
> +    gdk_window_withdraw (window->frame);

Either you should not call withdraw on widget->window in this case, or you should
make gdk/x11/gdkwindow-x11.c/gdk_window_unmap() check to see if the type is
GDK_WINDOW_CHILD before calling XWithdrawWindow().

> +  if (window->has_frame)
> +    {
> +      attributes.width = widget->allocation.width + window->frame_left + window->frame_right;
> +      attributes.height = widget->allocation.height + window->frame_top + window->frame_bottom;
> +      attributes.event_mask = (GDK_EXPOSURE_MASK |
> +			       GDK_KEY_PRESS_MASK |
> +			       GDK_ENTER_NOTIFY_MASK |
> +			       GDK_LEAVE_NOTIFY_MASK |
> +			       GDK_FOCUS_CHANGE_MASK |
> +			       GDK_STRUCTURE_MASK |
> +			       GDK_BUTTON_MOTION_MASK |
> +			       GDK_POINTER_MOTION_HINT_MASK |
> +			       GDK_BUTTON_PRESS_MASK |
> +			       GDK_BUTTON_RELEASE_MASK);
> +
> +      attributes_mask = GDK_WA_VISUAL | GDK_WA_COLORMAP;
> +      attributes_mask |= (window->title ? GDK_WA_TITLE : 0);
> +      attributes_mask |= (window->wmclass_name ? GDK_WA_WMCLASS : 0);
> +
> +      window->frame = gdk_window_new (NULL, &attributes, attributes_mask);
> +      gdk_window_set_user_data (window->frame, widget);
> +
> +      attributes.window_type = GDK_WINDOW_CHILD;
> +      attributes.x = window->frame_left;
> +      attributes.y = window->frame_right;
> +      attributes.width = widget->allocation.width;
> +      attributes.height = widget->allocation.height;
> +      attributes.event_mask = gtk_widget_get_events (widget);
> +      attributes.event_mask |= (GDK_EXPOSURE_MASK |
> +				GDK_KEY_PRESS_MASK |
> +				GDK_ENTER_NOTIFY_MASK |
> +				GDK_LEAVE_NOTIFY_MASK |
> +				GDK_FOCUS_CHANGE_MASK |
> +				GDK_STRUCTURE_MASK);
> +
> +      attributes_mask = GDK_WA_X | GDK_WA_Y | GDK_WA_VISUAL | GDK_WA_COLORMAP;
> +
> +      widget->window = gdk_window_new (window->frame, &attributes, attributes_mask);
> +      gdk_window_set_user_data (widget->window, window);
> 
> +
> +      widget->style = gtk_style_attach (widget->style, widget->window);
> +      gtk_style_set_background (widget->style, widget->window, GTK_STATE_NORMAL);
> +      gtk_style_set_background (widget->style, window->frame, GTK_STATE_NORMAL);
> +    }
> +  else
> +    {
> +      attributes.width = widget->allocation.width;
> +      attributes.height = widget->allocation.height;
> +      attributes.event_mask = gtk_widget_get_events (widget);
> +      attributes.event_mask |= (GDK_EXPOSURE_MASK |
> +				GDK_KEY_PRESS_MASK |
> +				GDK_ENTER_NOTIFY_MASK |
> +				GDK_LEAVE_NOTIFY_MASK |
> +				GDK_FOCUS_CHANGE_MASK |
> +				GDK_STRUCTURE_MASK);
> +
> +      attributes_mask = GDK_WA_VISUAL | GDK_WA_COLORMAP;
> +      attributes_mask |= (window->title ? GDK_WA_TITLE : 0);
> +      attributes_mask |= (window->wmclass_name ? GDK_WA_WMCLASS : 0);
> +
> +      widget->window = gdk_window_new (NULL, &attributes, attributes_mask);
> +      gdk_window_set_user_data (widget->window, window);
> +
> +      widget->style = gtk_style_attach (widget->style, widget->window);
> +      gtk_style_set_background (widget->style, widget->window, GTK_STATE_NORMAL);
> +    }

I think it is worth doing some combining between these two cases - you should
be able to have

  if (window->frame)
    {
       [...]

       attributes.window_type = GDK_WINDOW_TOPLEVEL;
       attributes.x = window->frame_left;
       attributes.y = window->frame_right;
       parent_window = window->frame;
    }
   else
    parent_window = NULL;

   attributes.width = widget->allocation.width;
   attributes.height = widget->allocation.height;
   attributes.event_mask = gtk_widget_get_events (widget);
   attributes.event_mask |= (GDK_EXPOSURE_MASK |
     	                     GDK_KEY_PRESS_MASK |
                             GDK_ENTER_NOTIFY_MASK |
			     GDK_LEAVE_NOTIFY_MASK |
			     GDK_FOCUS_CHANGE_MASK |
   			     GDK_STRUCTURE_MASK);

   attributes_mask = GDK_WA_VISUAL | GDK_WA_COLORMAP;
   attributes_mask |= (window->title ? GDK_WA_TITLE : 0);
   attributes_mask |= (window->wmclass_name ? GDK_WA_WMCLASS : 0);

   widget->window = gdk_window_new (parent_window, &attributes, attributes_mask);
   gdk_window_set_user_data (widget->window, window);

   widget->style = gtk_style_attach (widget->style, widget->window);
   gtk_style_set_background (widget->style, widget->window, GTK_STATE_NORMAL);

Or something like that.

>  static void
> +gtk_window_unrealize (GtkWidget *widget)
> +{
> +  GtkWindow *window;
> +
> +  g_return_if_fail (widget != NULL);
> +  g_return_if_fail (GTK_IS_WINDOW (widget));
> +
> +  window = GTK_WINDOW (widget);
> +
> +  if (window->frame)
> +    {
> +      gdk_window_set_user_data (window->frame, NULL);
> +      gdk_window_destroy (window->frame);
> +      window->frame = NULL;
> +    }
> +
> +  gtk_decorated_window_unrealize (window);

Can you do this as a signal connection? I think it would be good to minimize
the number of calls from GtkWindow to GtkDecoratedWindow.

>  static gint
> +gtk_window_event (GtkWidget *widget, GdkEvent *event)

I think it would be good to make sure that this always, always called before
any user handlers for the GtkWindow. The way to do this is to 
connect it as a signal handler for ::event in gtk_window_init() rather
than having it as a default handler. 

[ For bonus points, figure out how to make this work for:

 gtk_widget_ref (window);
 gtk_widget_destroy (window);
 gtk_widget_show (window);

Which is supposed to be valid now, though probably won't work for a lot
of widgets until someone starts testing it. ]

> +  GtkWindow *window;
> +  gboolean return_val;
> +
> +
> +  g_return_val_if_fail (widget != NULL, FALSE);
> +  g_return_val_if_fail (GTK_IS_WINDOW (widget), FALSE);
> +  g_return_val_if_fail (event != NULL, FALSE);
> +
> +  window = GTK_WINDOW (widget);
> +
> +  if (window->frame && (event->any.window == window->frame) &&
> +      (event->type != GDK_KEY_PRESS) && (event->type != GDK_KEY_RELEASE))

What about FOCUS_IN / FOCUS_OUT?

Also, it's probably good to be a little dirty here and rewrite the
event so that event->window == widget->window for key/focus events not
redirected:

 g_object_unref (event->window);
 event->window = widget->window;

Otherwise, you risk confusing some apps.

> +void
> +gtk_window_set_frame_dimensions (GtkWindow *window,
> +				 gint       left,
> +				 gint       top,
> +				 gint       right,
> +				 gint       bottom)
> +{
> +  GtkAllocation *allocation;
> +  GtkWidget *widget;
> +
> +  g_return_if_fail (window != NULL);
> +  g_return_if_fail (GTK_IS_WINDOW (window));
> +
> +  window->frame_left = left;
> +  window->frame_top = top;
> +  window->frame_right = right;
> +  window->frame_bottom = bottom;
>  }

This one, at least, should work after the widget is realized.

> +  gint (* frame_event) (GtkWidget *widget,
> +			GdkEvent  *event);

Should be gboolean. [ And same for your handlers for this event ]

> @@ -126,6 +137,13 @@ void       gtk_window_set_geometry_hints
>  void       gtk_window_set_default_size         (GtkWindow           *window,
>  						gint                 width,
>  						gint                 height);
> +/* gtk_window_set_has_frame () must be called before realizeing the window_*/
                                                              ^
Spelling.

> gtkwindow-decorate.h:
> ---------------------
> 
> /* GTK - The GIMP Toolkit
>  * Copyright (C) 1995-1997 Peter Mattis, Spencer Kimball and Josh MacDonald

On new files, you should put the correct copyright notice, so in this
case, Copyright (C) 2001 Red Hat, Inc.

And it is also good to list the author below below the license statement.

> /* GTK - The GIMP Toolkit
>  * Copyright (C) 1995-1997 Peter Mattis, Spencer Kimball and Josh MacDonald

Same here.

> #ifdef GDK_WINDOWING_FB
> #define DECORATE_WINDOWS
> #endif
> 
> #ifdef DECORATE_WINDOWS
> 
> #ifdef GDK_WINDOWING_FB
> #include "linux-fb/gdkfb.h"
> #endif

Hmmm, you conditionalize here, but then use functions from this file unconditionally
below?
 
> void
> gtk_decorated_window_calculate_frame_size (GtkWindow *window)
> {
>   GdkWMDecoration decorations;
>   GtkWindowDecoration *deco = get_decoration (window);
> 
>   if (_gdk_window_get_decorations (GTK_WIDGET (window)->window,
> 				   &decorations))

Underscored prefixed functions won't even be exported from GDK - they are 
library-private. Perhaps we should just make this XP and public - having
getters to go along with setters is pretty customary.

>     {
>       if ((decorations & GDK_DECOR_BORDER) &&
> 	  (decorations & GDK_DECOR_TITLE))
> 	deco->decorated = TRUE;
>       else
> 	deco->decorated = FALSE;
>     }
>   else
>     deco->decorated = (window->type != GTK_WINDOW_POPUP);
> 
>   if (deco->decorated)
>     gtk_window_set_frame_dimensions (window,
> 				     DECORATION_BORDER_LEFT,
> 				     DECORATION_BORDER_TOP,
> 				     DECORATION_BORDER_RIGHT,
> 				     DECORATION_BORDER_BOTTOM);

Hadn't you better set the frame dimensions to 0,0,0,0 otherwise?

> void
> gtk_decorated_window_realize (GtkWindow   *window)
> {
>   GtkWindowDecoration *deco = get_decoration (window);
>   GtkWidget *widget = GTK_WIDGET (window);
> 
>   deco->title_layout = gtk_widget_create_pango_layout (widget,
> 						       (window->title)?window->title:"");
> 
>   pango_layout_set_font_description (deco->title_layout,
> 				     pango_font_description_from_string(DECORATION_TITLE_FONT));
> 
>   _gdk_window_set_child_handler (window->frame,
> 				 gtk_decorated_window_inner_change,
> 				 gtk_decorated_window_inner_get_pos,
> 				 window);

Same comment about _gdk here. This probably should be gdk_fb_set_child_handler
or something.
 
>   gtk_decorated_window_paint (widget, NULL);

I wouldn't recommend this. The paint in GtkWindow proper is a pecularity left over
from the initial attempts at theming to try and prevent some flashing when showing
a window. But that is a hack and probably should be addressed in some other way.
 
> /* Copied from gtkwindow.c */
> typedef struct {
>   GdkGeometry    geometry; /* Last set of geometry hints we set */
>   GdkWindowHints flags;
>   gint           width;
>   gint           height;
> } GtkWindowLastGeometryInfo;
> 
> typedef struct {
>   /* Properties that the app has set on the window
>    */
>   GdkGeometry    geometry;	/* Geometry hints */
>   GdkWindowHints mask;
>   GtkWidget     *widget;	/* subwidget to which hints apply */
>   gint           width;		/* Default size */
>   gint           height;
> 
>   GtkWindowLastGeometryInfo last;
> } GtkWindowGeometryInfo;

>       if ((w > 0) && (h > 0))
> 	{
> 	  GtkWindowGeometryInfo *info = (GtkWindowGeometryInfo *)gtk_object_get_data (GTK_OBJECT (window), "gtk-window-geometry");

Urgh. As you can imagine, I don't like this. We should cleanly export
what is needed from gtkwindow.c which seems to be
gtk_window_constrain_size, which handles gridding, aspect ratio, etc,
as well as min/max size.

But in general, looks very good.
                                        Owen