Re: [gtk-list] [patch] gtkviewport design bug.

[Owen Taylor <owt1 cornell edu>]

Patrice Fortier <Patrice.Fortier@aquarel.fr> writes:

> Sorry Owen but I really wanted to get rid of this bug :).

That's spelled '"bug"'. :-)

> In the current implementation the embbeding (?) window of the
> child widget is the widget->window of the viewport widget.
> So if you want to subclass the viewport to extend its functionnalities,
> like adding a widget, the widget will be added in the child widget's
> "window" instead of the main_window.
> This behaviour is totally normal as the viewport is the only
> widget in which the widget->window is misplaced: The widget->window
> is always the main window (and in viewport, it's main_window).

A way to have child widgets in something other than widget->window
is certainly useful. (As Jay has pointed out for CList). And
I'm not completely against changing the meaning of widget->window
for GtkViewport, if it is done cleanly, and makes sense.

But I'll stick to my guns, and say categorically that subclassing
to add extra widgets is a bad idea in GTK. This is largely
to do this, you have to duplicate and modify the size allocation
and drawing code, so your new widget becomes dependent on
the internals of the parent widget, breaking encapsulation,
duplicating code and making maitainance difficult.

Subclassing (in GTK) is good for:

 - Providing a common interface.
 - Implementing code sharing between siblings in the heirarchy
   by implementation inheritance.
 - Implementing composite widgets where the parent is a 
   container class.

It is occasionally useful for modifying the behavior of an
existing base class by overriding the default behaviors.
(The base class, as in all OO programming, must be designed
with this in mind.) GtkButton and GtkWindow are examples

But it is not good for adding extra doodads to existing widgets,
because of the problems I mentioned above. It is admittedly
appealing to try to do this, because you can use the derived
widget in place of the parent widget, but I would argue that
this is confusing interface inheritance with implementation
inheritance. (If GTK could support multiple inheritance,
then there would be a clean way of handling this:


       GtkTable            GtkViewport
                 \          /         \
                    GtkScrollbar =>  GtkRealViewport
                       
Where GtkScrollbar contains a GtkRealViewport and forwards
requests for viewport operations to that child. But 
as it is, you just have to grit your teeth and admit 
to the user of your class that a GtkScrollbar has a 
GtkViewport, but it is not a GtkViewport.)

Arguments about these types of issues have consumed vast amounts
of time and energy over the years, so I don't expect to convince
everyone (anyone?) but I would argue that this is pretty
close to the philosophy with which the GTK object system was
designed, so even if you feel that was a mistake, you will
get better results for GTK widget design if you temporarily
abandon your convictions.

> This patch corrects this. The main window is now widget->window,
> and the window embbeding the child widget is called bin_window
> (the child widget is called viewport->bin).
> I added the function gtk_viewport_reparent_bin_window() which tells
> a widget that its parent window isn't widget->parent->window but
> another one.
> This function is called when the realize() function is called and
> there is a child widget, or by the viewport_add() function when
> we assign a child widget to the viewport. 
> This function is based on the reparent() code, so I don't think it
> will break to much things :).

Unless the reparent code is broken. (Despite my, and Guillaume
Laurent's best efforts). Duplicating that code is really not
acceptable - it is complicated, and if it still has a lurking
bug somewhere, it is not good to have to fix it in two places.

Also, I would characterize the reparenting the child window as
a hack, and hacks should be kept as simple as possible.
 
I think the right way to do things is to add an extra

  parent_window

field to widgets (this bloats the GtkWidget structure yet further,
unfortunately - another approach would be to use a "parent_widget"
key and use parent->window if not set, but that will be slower).
This parent_window will be set when the parent is realized, and
when widgets are added to an already existing parent.

This should be pretty simple to implement, but probably is
too extensive a change to make before 1.0

If you really need to inherit from GtkViewport (you don't!), why don't
you use Jay's CList approach to reparent the extra widgets that you
are adding (instead of every child widget in every viewport). This
will likely be faster and more robust since you aren't reparenting the
entire widget tree in the GtkWidget, and will give us a consistent
approach until we come up with something better.

Regards,

Owen ("The official GTK design pontificator"? Time to write some code!)