GNOME Bugzilla – Bug 63820
Refactor and document gdkgeometry-x11.c
Last modified: 2009-07-22 18:21:53 UTC
Much code is tripply duplicated betweeen: gdk_window_guffaw_scroll() _gdk_window_move_resize() gdk_window_premove/gdk_window_postmove() Combining the code would improve readability and maintainability as well as reducing line count. It's not compltely straightforward since the code duplication isn't exact; a good approach would be to functionalize the small repeated portions which would make it clear what is repeated in the larger sections. Also, there is a bad need for documentation of the idea of guffaw scrolling and the algorithms we are using here to emulate big windows using guffaw scrolling.
I agree on every word that you wrote, especially the last paragraph. Trying to fix gdkgeometry-win32.c once again without real understanding of the concept appears to be quite hard - though I'll probably give it a try ... One not so kind solution would be to ignore the 16bit limitation of the Win9x GDI, but it would require to update me my Notebooks OS :) Would it be a help for you if I put comments in the win32 backend scrolling where ever I have absolutely no clue why it is done this way ? :-) BTW: the 'big window torture test' currently tortures Gtk/win32 (on Win98) so much, that I need to reboot. Thanks in advance, Hans
Started some works on docs. Sun Nov 25 21:19:02 2001 Owen Taylor <otaylor@redhat.com> * gdk/x11/gdkgeometry-x11.c: Add long, but horribly sketchy comment about what is going on in this file. It is certainly possible to use literally the same algorithm on Win32 -- you should be able to emulate static gravity by using SetWindowPosition with SWP_NOCOPYBITS, then moving the children back again using SetWindowPosition with SWP_NOCOPYBITS. It wouldn't be a particularly efficient way of achieving things in many cases, however. For instance, if you were just scrolling you could frequently (though not always) simply use ScrollWindow. I think the basic idea of gdkgeometry-x11.c still holds on Win32; you want a mapping from 32 bit position to: - real window position - real/virtual offset - map state And you probably can just use the same one as used for X11. But the transition from state A to state B is going to be different on Windows; because the available primitives are different.
Not planning to do more on this prior to 2.0.0
Created attachment 86931 [details] [review] Patch 1 -- make list handling code easier to read I'm a bit bored and thought I could help with this. This first patch cleans up most of the list handling code throughout that file, and makes it more readable. 19 insertions(+), 59 deletions(-) More patches to come, unless I get scared too much by the code :P
Created attachment 87032 [details] [review] Patch 2 -- factored out translate_pos() Just factored out some common code.
Created attachment 87033 [details] [review] Patch 3 -- Added wrappers around XMoveWindow/XMoveResizeWindow Added wrappers around those functions that accept rectangles, GdkXPositionInfo's. This patch increases code size, but improves readability.
Created attachment 87034 [details] [review] Patch 4 -- added wrappers for XMapWindow/XUnmapWindow Factored out the tests that determine whether we need to map/unmap a window.
Comment on attachment 87032 [details] [review] Patch 2 -- factored out translate_pos() I think switching the order of src and dest would be slightly more natural, compare dest->x = src->x + obj->x; with translate_pos (src, dest, obj, ...)
Comment on attachment 87033 [details] [review] Patch 3 -- Added wrappers around XMoveWindow/XMoveResizeWindow I think we can do without the x-prefix here and avoid shorthands. How about just move(), move_relative(), move_resize() ? Looking at the definition of GdkXPositionInfo, I think you could get away without move_resize2, and just cast a (GdkXPositionInfo*) to a (GdkRectangle*).
The last patch looks fine.
Created attachment 87067 [details] [review] Patch 2 -- factored out translate_pos(), take 2 Here's patch 2, tweaked per comment #8.
Created attachment 87074 [details] [review] Patch 3 -- Added wrappers around XMoveWindow/XMoveResizeWindow, take 2 Updated patch 3 per comment #9.
Created attachment 87078 [details] [review] Patch 5 -- replace GDK_WINDOW calls by casts to GdkWindow Patch 1 (86931) accidently added one GDK_WINDOW call. The old code didn't cast at all, and I accidently used the GDK_WINDOW macro rather than just casting to GdkWindow*. This patch reverts that change. Also, in another chunk, it makes the code use a simple cast rather than the type checking macro. I don't see a reason why one scrolling routine should use different code than the other in that regard. So, if patch 1 goes in, this should go in, too, IMO.
Created attachment 87223 [details] [review] Patch 6 -- Factored out queue_translation_if_ltz() and _gtz()
Ping -- can anyone have a look at the last two patches?
I don't think the last patch is a big win in terms of clarity.
Seems like the refactoring has been mostly done by CSW, right? I'm going to close this unless you think there is further work to do on it.