Re: rendering-cleanup-next

[Havoc Pennington <hp pobox com>]

Hi,

On Sat, Sep 11, 2010 at 1:50 PM, Benjamin Otte <otte gnome org> wrote:
> The problem here is that I want to get rid of proxying the ugly X11
> background API via GDK. Neither Windows nor Quartz have anything
> similar. So my approach was to use a cairo_pattern_t in the API and
> then say "The backends are responsible for mapping it to the system
> backgrounds as well as possible." as we overpaint it during exposes
> anyway. So hat we do in the gdk_x11_window_set_background() call is
> open for debate.

Does GDK have enough information to know when to set background to None?
The patch
d3802ca Remove calls that try to set GDK_NO_BG

basically removes hints to GDK that the background should not be
repainted by the OS. I don't know how GDK would know this.

I don't know exactly what the API should be. Maybe set_hint_no_clear()
or set_autopaint() or whatever it is. The main thing is, I think pre-
this patch, things probably did not flicker as much, and post- this
patch, they are probably flickery and uglier. So that's a user-visible
regression. It just doesn't seem like a good idea to me to regress
that.

The API for this may be something that's a no-op on Windows and/or
Mac, I don't know.

I don't really know the right API, but regressing the user-visible
would suck, that's the .02 here.

I would have to dig into more details to say "here is exactly what
would look uglier and how" but I'm just assuming the existing "set
background to None" calls had some purpose.

> First of all, expose events are already different from all other
> events today in GTK2. One of them is sent via
> gtk_widget_send_expose(), the other via gtk_widget_send_event().

But send_expose() was only required because no-window widgets get
exposes ... but with your patch they no longer need to.

In GTK 1.2, iirc, expose-event was the raw GdkWindow event and there
was a draw() signal. In 2.0, we consolidated on expose-event and
generated expose-event for no-window widgets. With 3.0 and your patch,
what would make sense to me is to make expose-event again be just the
raw window system event, and no-window widgets therefore never get
expose-event. send_expose() would not be needed because expose-event
would have zero pre-processing; the pre-processing would all be in
front of draw(), not in front of expose-event.  expose-event becomes
just the raw thing from GdkWindow. Back to the future!

> Also, I do think that having two events (draw and expose-event) on
> GtkWidget is confusing, in particular because one of them is only
> emitted sometimes (gtk_container_propagate_draw() would not cause
> expose events).

To be clear, I'm talking about making expose-event show up in _less_
of the code.

I would say, expose-event should be defined to be exactly what comes
from GdkWindow, no pre-processing or special handling _at all_, and it
only happens on window widgets. In the GTK of the future, this
expose-event thing would be like the other obscure list of events that
only window widgets ever get (destroy-notify, visibility-notify, blah
blah). These events should all be lifted up out of GtkWidget somehow
and only be present on windowed widgets; maybe via an interface, I
don't know.

A subset of events that are actually of interest to no-window widgets
would then need to go to all widgets - motion events, a few others.
Possibly the actual event struct even differs.

What would be super-clean would be that all dealing with GdkWindow
events (and GdkWindow in general) lives walled off in a
GtkWidgetWithWindow class. GtkWidget itself should not have any such
knowledge.

Anyhow so where I'm coming from on this, is that it then makes sense
to start moving all the event handling stuff into GtkWidget, in the
event-handling methods. But taking events out of the GtkWidget API,
and taking them out of code that applies generically to all widgets in
gtkmain.c etc. This is all in preparation for then taking most of the
event vfuncs and signals and copying them out of GtkWidget and into
some sort of GtkWidgetWithWindow.

Specifically looking at the patch, first the code in
http://git.gnome.org/browse/gtk+/tree/gtk/gtkmain.c?h=rendering-cleanup-next#n1674
i.e. GDK_EXPOSE switch case in gtkmain.c, this could just be in
GtkWidget (to be later lifted into the GtkWidgetWithWindow thing).

Then looking at gtk_widget_send_expose(),
http://git.gnome.org/browse/gtk+/tree/gtk/gtkwidget.c?h=rendering-cleanup-next#n4863

I think this should not ever work on no-window widgets at all;
no-window widgets can't get an expose, because expose-event is a
GdkWindow thing. So given that it doesn't work on no-window widgets,
make it the expose-event default handler. And then when the
expose-event vfunc and signal are lifted up and out into
GtkWidgetWithWindow, this thing comes with it.

Why not just delete gtk_widget_send_expose(). No reason for GTK 3 to
have that function I don't think.

> What I want to have is a GdkWindow::event signal instead of the magic
> gdk_window_set_user_data() function. Then your code could just connect
> to that signal and do whatever it wants with expose events.

That makes sense, but I would say there's still a default handler or
shared code (such as in a GtkWidgetWithWindow base class) that
connects to this signal and does generic stuff. So in there you'd have
a default handler for expose-event that converted it into draw() calls
and did the double-buffering setup and whatever.

The advantages of keeping an expose_event vfunc (and signal) are:
- WidgetWithWindow subclasses don't have to do a giant switch statement
- subclasses can override or chain up to the default handler
- apps can connect to the signal without having to subclass

It's true that you can do everything from just one
GtkWidgetWithWindow::event signal, with a switch() inside it, that's
sort of a separate question whether to have one vfunc/signal for all
events or do the explode-into-signal-per-event-type thing.

Anyway in this world where GtkWidgetWithWindow contains all
GdkWindow/GdkEvent stuff, and GtkWidget has none, you would not want
the app or a subclass to connect to the GdkWindow directly. You'd want
it to hook into and override or chain up to the GtkWidgetWithWindow
handler for each event. At least, it seems pretty likely that would be
useful.

What I'm trying to say is I think expose-event (and the existing docs
and stuff on it that the rendering branch deletes) really ought to
exist on GtkWidgetWithWindow, so it makes sense to keep it for now,
get the right stuff into it for window widgets, and then when all
window-related stuff is yanked out of GtkWidget, the expose-event
stuff will already be nicely consolidated and ready to come along.

>> There's a fair bit of trailing whitespace in the patch. Maybe turn on
>> trailing-whitespace-highlighting in your editor.
>>
> I'm of the "if the tools don't fix it, it isn't broken" school of
> thought.

Emacs can be set up to fix it - you aren't using some suck editor are you :-P

The problem with trailing whitespace (and using tabs) is that you get
whitespace changes in diffs, or have to be tip-toeing around to avoid
them, one or the other. If GTK doesn't have a policy against... it
sure ought to. (Surely gnome.org has some kind of git hook that can be
enabled to block trailing whitespace in new commits... )

Anyhow, sure, if GTK has no policy that's fine. I assumed it had a
sensible policy...

> That said, from my experiences a widget must be drawable for it to
> render properly. As long as a widget is not visible, it will not be
> allocated a proper size and render wrong. If it isn't realized, it
> hasn't created its windows and will therefor mess up its rendering. So
> realized and visible are very required already. Dropping the mapped
> requirement would be possible, but the current code only maps and
> realizes child widgets when mapping. A realized and visible treeview
> for example does not render headers. This could be changed, but would
> require redefining how children get realized/mapped with their
> parents.
> So the only sane way to get a proper rendering from a widget currently
> seems to be to require that it is drawable.

This makes sense. (Though I think it's a situation to be fixed up eventually.)

I would say to draw, in a future where only toplevels and "weird embed
stuff" have GdkWindow, the invariants might include:

* if and only if drawing to the window system, the widget must be
realized. (we're expecting for no-window widgets that realization
usually doesn't do anything, though)
* the widget must have been size-allocated at least once and not have
the need-request or need-allocate flags set
* as a result it's OK to call get_allocated_width/height (or whatever
those end up being called)

Things that could be relaxed in the future:

* that the widget has to be inside a container. should be able to draw
an orphan widget as long as you size allocate it.
(this implies relaxing the "must be mapped" requirement obviously
since you need a parent in order to be mapped)
* that the widget has to be visible; show/hide change whether a widget
will be mapped, but I don't see why they matter for drawing

Havoc