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 _______________________________________________ gtk-devel-list mailing list gtk-devel-list@gnome.org http://mail.gnome.org/mailman/listinfo/gtk-devel-list
