Re: rendering-cleanup-next

[Benjamin Otte <otte gnome org>]

Whew, lots of things to reply to. Here we go...

On Sat, Sep 11, 2010 at 5:16 PM, Havoc Pennington <hp pobox com> wrote:
> A round of rebase/squash might be nice which would make it easier to
> review, for example c5c08bafb94e794a88ef5d650999f46b419429ed could
> squish into 9badb81a7ed62af1cdf11eb31c7e94293a683429 (I was pretty
> confused by the None there at first)
>
Ooops. I had assumed the first commit had landed in master already.
I'll squash that in the next rebase.

> I'm skeptical of removing ability to set window background to None.
> This feature can be important to avoid visual artifacts on X11. The
> API should maybe change to not involve a "NULL" pixmap; conceptually,
> what this means is "window system should not auto-clear or auto-paint
> the background on exposes" - I don't know if any other platform has
> the concept. A new API could be
> gdk_window_set_paint_background(gboolean) rather than set_pixmap(NULL)
>
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.

> I think it would be nicer than send_expose, if you kept expose_event,
> but had gtk_widget_real_expose_event() do the GDK_EXPOSE case from
> gtk_main_do_event() and then also have the code in send_expose(). i.e.
> the default handler for expose_event should set things up and call
> draw().
>
> It seems to me kind of bizarre to treat expose events differently from
> the others. Right now all GdkWindow events are on Widget. I would
> think what makes sense is to keep them all there, until widget->window
> becomes optional, and then move them all to some sort of
> GtkWidgetWithWindow iface where widgets that subscribe to a
> GdkWindow's events would get these but not widgets in general.
>
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(). Then,
I don't think that it's wise to treat expose handling the same way as
input handling anyway, because it's just different.
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).
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.

> 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. So I don't spend much time taking care of formatting things
properly. Also, I don't think GTK has a very strict policy on code
formatting these days other than "make sure it looks like all the
other code".

> Shouldn't gtk_widget_is_drawable() just die? It seems to me the draw()
> method can be called when not mapped or visible. In fact it can be
> useful to do that if you're trying to render offscreen or to pdf.
> (Maybe we want to pedantically require visible, but I don't think we
> should have to be mapped, which implies in a toplevel window system
> window.)
>
> gtk_widget_draw() is documented as requiring the widget to be
> drawable, but I don't see why draw() in its current form needs widgets
> to be mapped.
>
This was a somewhat harder decision for me to take. I kept it as-is
for now for one simple reason: It's easier to relax this requirement
later than it is to make it stricter.
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.

Benjamin