After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 63820 - Refactor and document gdkgeometry-x11.c
Refactor and document gdkgeometry-x11.c
Status: RESOLVED FIXED
Product: gtk+
Classification: Platform
Component: Backend: X11
1.3.x
Other Linux
: Normal normal
: Medium feature
Assigned To: Owen Taylor
Owen Taylor
Depends on:
Blocks:
 
 
Reported: 2001-11-05 15:57 UTC by Owen Taylor
Modified: 2009-07-22 18:21 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Patch 1 -- make list handling code easier to read (5.51 KB, patch)
2007-04-24 19:02 UTC, Tilman Sauerbeck
committed Details | Review
Patch 2 -- factored out translate_pos() (3.54 KB, patch)
2007-04-25 20:36 UTC, Tilman Sauerbeck
reviewed Details | Review
Patch 3 -- Added wrappers around XMoveWindow/XMoveResizeWindow (5.21 KB, patch)
2007-04-25 20:40 UTC, Tilman Sauerbeck
reviewed Details | Review
Patch 4 -- added wrappers for XMapWindow/XUnmapWindow (3.14 KB, patch)
2007-04-25 20:42 UTC, Tilman Sauerbeck
committed Details | Review
Patch 2 -- factored out translate_pos(), take 2 (3.48 KB, patch)
2007-04-26 15:43 UTC, Tilman Sauerbeck
committed Details | Review
Patch 3 -- Added wrappers around XMoveWindow/XMoveResizeWindow, take 2 (4.99 KB, patch)
2007-04-26 16:22 UTC, Tilman Sauerbeck
committed Details | Review
Patch 5 -- replace GDK_WINDOW calls by casts to GdkWindow (834 bytes, patch)
2007-04-26 16:37 UTC, Tilman Sauerbeck
committed Details | Review
Patch 6 -- Factored out queue_translation_if_ltz() and _gtz() (3.03 KB, patch)
2007-04-29 08:31 UTC, Tilman Sauerbeck
rejected Details | Review

Description Owen Taylor 2001-11-05 15:57:26 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.
Comment 1 Hans Breuer 2001-11-17 22:31:30 UTC
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
Comment 2 Owen Taylor 2001-11-26 02:53:47 UTC
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.
Comment 3 Owen Taylor 2002-01-18 03:48:52 UTC
Not planning to do more on this prior to 2.0.0
Comment 4 Tilman Sauerbeck 2007-04-24 19:02:24 UTC
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
Comment 5 Tilman Sauerbeck 2007-04-25 20:36:40 UTC
Created attachment 87032 [details] [review]
Patch 2 -- factored out translate_pos()

Just factored out some common code.
Comment 6 Tilman Sauerbeck 2007-04-25 20:40:52 UTC
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.
Comment 7 Tilman Sauerbeck 2007-04-25 20:42:15 UTC
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 8 Matthias Clasen 2007-04-26 05:08:22 UTC
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 9 Matthias Clasen 2007-04-26 05:25:11 UTC
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*).
Comment 10 Matthias Clasen 2007-04-26 05:27:10 UTC
The last patch looks fine.
Comment 11 Tilman Sauerbeck 2007-04-26 15:43:49 UTC
Created attachment 87067 [details] [review]
Patch 2 -- factored out translate_pos(), take 2

Here's patch 2, tweaked per comment #8.
Comment 12 Tilman Sauerbeck 2007-04-26 16:22:11 UTC
Created attachment 87074 [details] [review]
Patch 3 -- Added wrappers around XMoveWindow/XMoveResizeWindow, take 2

Updated patch 3 per comment #9.
Comment 13 Tilman Sauerbeck 2007-04-26 16:37:50 UTC
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.
Comment 14 Tilman Sauerbeck 2007-04-29 08:31:13 UTC
Created attachment 87223 [details] [review]
Patch 6 -- Factored out queue_translation_if_ltz() and _gtz()
Comment 15 Tilman Sauerbeck 2007-05-12 09:58:06 UTC
Ping -- can anyone have a look at the last two patches?
Comment 16 Matthias Clasen 2007-06-03 04:30:04 UTC
I don't think the last patch is a big win in terms of clarity. 
Comment 17 Cody Russell 2009-07-22 18:21:39 UTC
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.