Re: rendering-cleanup-next

[Havoc Pennington <hp pobox com>]

Awesome! Some stuff I noticed looking through the branch:

*

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)

*

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)

I think d3802ca "Remove calls that try to set GDK_NO_BG on their
windows" probably results in some things being more ugly.
(flashing/flicker)

This almost goes away as an issue if dropping all subwindows, but I
think setting None on toplevels will still be important sometimes. I
guess in future-composited-desktop-world this also fades in importance
but I'm not sure on when/whether/details.

Anyway - the commit message "undefined behavior is not a good idea"
seems like you're missing the point of this - the idea is not to leave
the window bg undefined, it's to wait and let the app repaint it,
instead of first having window system repaint then the app repaints
(which inherently flickers).

*

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.

The way you have it here, there's no way for people to customize the
raw event handling. Say for example I'm doing a GL-based widget, or
who knows what other weird thing, maybe my app uses Skia, maybe I want
to get the raw expose event. If that makes sense for the app and I
don't need draw-to-pdf, I should be able to do it. Maybe I have a
legacy app that has a pile of Xlib drawing code.

An alternative, which I think is hackier, would be to make
gtk_cairo_get_event() public.

Anyway the ultimate goal would be for only toplevel GtkWindow plus
wonky widgets (GL, embeds) to have the event signals. But whichever
widgets keep the event signals, should still have the expose signal,
imo.

*

I still think passing width, height to draw() is weird. If I were just
reading this API, I would *strongly* tend to guess that it was the
damage rectangle or else the size I'm supposed to paint to, but it
isn't. It's a redundant copy of allocation size. Plus, you're clipping
to allocation _anyway_ so in most cases I don't even _need_ the
allocation size. So the width/height here are just _extra_ typing, not
saving me typing allocation.width,height. App developers basically
_should_ ignore these. So why are they here?

These parameters just confuse and raise questions. If they are defined
to be allocation.width,height, they should not exist. App devs are
going to think "OK, it can't be that" and think it has to be damage
region, or think they are supposed to support drawing to an arbitrary
size, or otherwise try to rationalize the existence of these pointless
redundant parameters. But they don't have a rationale ;-) Things with
no point are confusing. Gratuitous cognitive load.

If I need to know the allocation, then I'll look at the allocation.

*

There's a fair bit of trailing whitespace in the patch. Maybe turn on
trailing-whitespace-highlighting in your editor.

*

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.


Havoc