Re: Scrollbars patch

[Harold L Hunt <huntharo at msu dot edu>]

Jehan,

Here is a new patch and the files that you are missing:
http://www.msu.edu/~huntharo/xwin/shadow/xwin-scrollbars-20020711-1128.diff.bz2
(12 KiB)
http://www.msu.edu/~huntharo/xwin/shadow/xwin-scrollbars-newfiles-20020711-1128.tar.bz2
(9 KiB)

Let me know what you think of the new patch.  Answers to questions, etc. are
below.

> > Jehan,
> >
> > Implementing a scrollbar patch is quite a bit more complex than you had
> > initially thought.
> >
> > I have done a ton of work on the patch and I've got things much more
> complete
> > now.  I haven't got time to describe all the changes I made, but here is
> the
> > patch against current CVS for you to look over:
> >
> > http://www.msu.edu/~huntharo/xwin/shadow/xwin-scrollbars-20020710.diff.bz2
> (12
> > KiB)
> >
> >
> > Okay, I'll give a few highlights:
> >
> > 1) You were passing FALSE for the fRedraw flag to SetScrollInfo in almost
> > every call that you made to the function.  This had multiple effects,
> > including causing the thumb position to lag behind the actual scroll
> location,
> > causing the thumb size to not update when you changed the page size, etc.
> 
> Actually, I set FALSE in all cases. If not, then I forgot :). If I did
> that, it because on my machine it was working well that way, so I didn't
> want to force Windows to paint the scrollbar twice.
> 

Charles Petzold's book says that you only want to pass FALSE if you will be
making other function calls that will result in the scrollbars being redrawn,
so that you can avoid excessive redrawing.  When we call SetScrollInfo with a
new position, that is it.  We only make that one call that could potentially
cause the scrollbar to redraw, so we have to pass TRUE.

> 
> > 2) The code for WM_VSCROLL and WM_HSCROLL made about 8 sets of calls each
> to
> > the same functions with slightly different parameters.  I changed the code
> to
> > calculate the parameters first, then make one set of calls to
> SetScrollInfo
> > and ScrollWindowEx, etc.
> 
> I thought about doing something like that and then I forgot.
> And I didn't look at the example in MSDN, I understood the message well
> enough and usually their examples suck if they compile at all!! (and
> this one isn't perfect either: they call UpdateWindow in one case but
> not the other, tsss!!!)
> 

I had to clean up the original code, it was spaghetti :)

> 
> > 3) I added a flag for fUserGaveHeightAndWidth to indicate that the user
> > explicity passed a height and width for a given screen.  Now when we are
> > creating a default-sized window, with -scrollbars, we make the window as
> large
> > as possible, and we shrink the underlying X visual to fit within the
> client
> > area of the Windows window, without displaying the scrollbars.  We show
> the
> > scrollbars if/when the user ever shrinks the window.
>  >
> > 4) When a user does specify a visual size with -scrollbars, we make the
> > initial window as large as possible and make the visual the same size as
> the
> > specified size.  We show the scrollbars only if necessary (i.e. we hide
> them
> > if the user passes -screen 0 800 600 -scrollbars on a 1024x768 display).
> 
> Hiding the scrollbars, that was already the case, wasn't it?
> 

No.  I never got the original code to stop displaying scrollbars if the
-scrollbars parameter was passed.  It didn't matter how large the original
window was, nor did it matter how you resized the window.  I could not get
those scrollbars to go away.

> 
> > 5) You no longer have to specify a width and height for a ``-screen
> scr_num
> > [width height]'' parameter, which allows you to do:
> > XWin -scrollbars -screen 0 -screen 1
> >
> > This would create two full-sized screens that are resizable but that will
> not
> > initially display scrollbars.
> 
> This has nothing to do with the scrollbar, does it? But still a good
> feature :)
> 

It really is related, because we have to make a branch on whether the user
explicity told us how large they wanted the X visual.  If they explicity
specified a visual size then we say, ``okay, that is how big the visual will
be, display scrollbars if necessary''.  On the other hand, if they just want
as big of a window as possible, (such as running ``XWin -scrollbars'') then we
will trim the visual size to fit inside of a window without scrollbars, but we
allow the window to be resized later, which would result in scrollbars being
displayed.

If I did not add the ``-screen 0'' functionality, then users would have to
pass a size, such as ``-scrollbars -screen 0 1024 768 -screen 1 1024 768'',
which would result in two windows that would initially have scrollbars and a
1024x768 visual.  The user would have to be very craft in order to figure out,
on their own, the largest size window that could be displayed without causing
scrollbars to initially be visible.  Allowing them to pass ``-scrollbars
-screen 0 -screen 1'' allows them to very easily create two windows that are
as large as possible with scrollbars initially hidden, but with the ability to
resize the window if desired.

> 
> > 6) I added processing for WM_GETMINMAXINFO, in which we update the maximum
> > tracking size for the window.  The processing in WM_SIZING attempted to do
> the
> > same thing, but in actuality it never did anything because it would never
> see
> > sizes larger than the initial window size.  The max tracking size is the
> > largest size that the window is allowed to have when it is not maximized.
> We
> > let the user make the window large enough to display the whole visual,
> even is
> > this means that the window will be larger than the current display (just
> > think, they can move the window around to see the part that they are
> > interested in... I'm not going to argue with someone that wants to do
> that).
> > This should allow multi-monitor users to create one huge window and
> stretch it
> > across a couple of monitors, if they so desire.
> 
> I didn't know this message, or actually I forgot. That's more elegant
> for sure.
> 

Thanks.

> 
> > 7) I added a check to make sure that specified screens are numbered
> > consecutively from 0.  Screens do not have to be described in order, but
> there
> > cannot be any gaps in the screen number sequence once all parameters have
> been
> > processed.  This prevents a user from doing ``XWin -screen 1 -scrollbars''
> and
> > then wondering why the window does not have scrollbars (or resizing
> support).
> >  This fixes an existing, but subtle, bug that no one seems to have
> stumbled
> > across yet.
> 
> Same comment as 5). :@p
> 

You busted me... but I had to document this somewhere.  Did you like how I
tried to spoof this as being related by passing ``-scrollbars'' in my example
command line?  :)

> 
> > 8) I added WM_MAXIMIZE to the window style when -scrollbars is passed.
> This
> > allows one to maximize the Cygwin/XFree86 window.  However, there are a
> few
> > problems here... such as, what is a maximized 800x600 window on a 1024x768
> > screen?  I dunno... try it, it is weird.
> 
> I wanted to but I can't compile for now (missing xf86openConfigFile,
> xf86readConfigFile, xf86closeConfigFile at link timeand I don't have
> time to look at that yet)
> But, a wild guess, isn't ptMaxSize in WM_GETMINMAXINFO for that? (MSDN,
> MINMAXINFO: ptMaxSize | when a window is maximized or resized, ...)
> 

I've included a tarball with the missing files above.

We can't make ptMaxSize larger than our visual size, because we could have an
800x600 visual on a 1024x768 display... we would end up with several thousand
blank pixels if we really maximized the window.  That is the weirdness that I
was talking about.

> 
> > 9) WarpCursor is messed up when you use mwm to switch to another virtual
> > desktop inside of a Cygwin/XFree86 window that is smaller than the
> underlying
> > visual and has scrollbars displayed.  In this case, WarpCursor will blast
> the
> > cursor to the location on the Windows display where the X location should
> > be... but that location may actually be scrolled off the Cygwin/XFree86
> > window.  In those cases I would like to be able to scroll the warp
> destination
> > into the Cygwin/XFree86 window, then warp accordingly.  Figuring out how
> to
> > get the warp destination into the current scroll viewport may be
> difficult.
> 
> Is it?
> 	GetScrollInfo(...)
> 	if ((XY < nPos) || (XY > nPos+Page)) {
> 	  nPos = max(0, min(nMax-nPage+1, XY-nPage/2))
> 	  SetScrollInfo(...)
> 	  GetScrollInfo(...)
> 	  dwOffset = nPos;
> 	  ScrollWindowEx(...)
> 	}
> if you rely on Windows to correct the scrolling you can simplify with
> 	nPos = XY-nPage/2
> instead of the min/max thingy
> 
> So either I'm missing something, or it's easier than you thought.
> 

I will give this a shot.  Thanks.

> 
> j1) in WM_SIZE, you check for fDecoration and fFullscreen for breaking.
> This is not necessary maybe even dangerous, especially in the
> nodecoration case. With nodecoration, I guess we can still receive a
> WM_SIZE if we change the screen resolution via the Display control panel
> or if we change the size of the taskbar.
> And the other hand, I don't thing we'll get any bug if we still do
> process WM_SIZE.
> Well, let's say that, as a matter of preference, I usually manage
> unlikely cases if the code necessary to do it doesn't make my code
> harder to read.
> 

I was assuming that you had intended for scrollbars to be allowed only when
not fullscreen and with decorations.  I know you didn't intend for scrollbars
to be displayed in fullscreen mode because you didn't modify
winCreateBoundingWindowFullScreen.  I don't see any reason why we should
disallow scrollbars when there are no window decorations.  I'll readd that
functionality in the near future.

> 
> j2) Just for detail and the pleasure of commenting (:p), you also have
> those lines
> 	/* Is the client area large enough to show the whole visual? */
> 	if (iWidth != s_pScreenInfo->dwWidth
> 	    || iHeight != s_pScreenInfo->dwHeight)
>    if iWidth is bigger than s_pScreenInfo->dwWidth (same for height),
> isn't the screen "large enough to show the whole visual"?
> 

iWidth can never be larger than s_pScreenInfo->dwWidth because the window can
never be larger than the maximum tracking size.  However, for clarity, I have
 changed this to iWidth < s_pScreenInfo->dwWidth.

> 
> j3) In your note in wincreatewnd.c, when you describe what is range and
> what is page, you say: "In other words, the page size is the size of the
> client area minus the space the scrollbars occupy". And as you said in
> winwndproc.c, the client area already takes into account the space
> occupied by the scrollbars.
> 

Yeah, I got confused by some MSDN Library docs.  I added a note that clarifies
this and blames MSDN Library for any future confusion :)

> 
> j4) Still in wincreatewnd.c, for better clarity, I would write something
> like:
> 	if (!(pScreenInfo->fScrollbars && pScreenInfo->fUserGaveHeightAndWidth)) {
> 	  /* except if the user specified the size of the visual *and*
> 	     uses scrollbars, we don't want the visual bigger than
> 	     the window */
> 	  pScreenInfo->dwWidth = rcClient.right - rcClient.left;
> 	  pScreenInfo->dwHeight = rcClient.bottom - rcClient.top;
> 	}
> 
> 	if (pScreenInfo->fScrollbars)
> 	  ...
> 	  si.nMax = pScreenInfo->dwWidth - 1;
> 	  SetScrollInfo (*phwnd, SB_HORZ, &si, FALSE);
> 	  ...
> 	  si.nMax = pScreenInfo->dwHeight - 1;
> 	  SetScrollInfo (*phwnd, SB_VERT, &si, FALSE);
> 	}
> 	/* no else anymore */
> 

Excellent.  I implemented just such a cleanup.

Then, I went further.  We don't even need to call SetScrollInfo in
winCreateBoundingWindowWindowed.  Rather, we just set nMin and nMax in our
calls to SetScrollInfo in our processing of WM_SIZE.  The overhead for doing
this in each WM_SIZE is negligible, and the resulting cleanup is appreciable.
 I have simply #if 0'd the cleaned-up code in winCreateBoundingWindowWindowed,
just so we have that code, published in a diff somewhere, to fall back on if
we ever find a reason to.

> 
> j5) in the case of WM_VSCROLL, SB_BOTTOM should actually be nMax-nPage+1
> (same for WM_HSCROLL and SB_RIGHT)
> 

Charles Petzold doesn't seem to think so in his book.  I think he was relying
on Windows to trim the scroll position to be the maximum allowed position. 
However, I went ahead and changed this so that others are not confused.

> 
> j6) in WM_GETMINMAXINFO, you assume that we have decoration. Can't we
> receive this event if we resize the screen? (and same reason than in j1)
> 

See my answer to j1.  I will fix this in the near future.

> 
> > Ah... that's enough for today.  I'm tired.
> 
> Welcome to the club, Good night! ;p
> 

What do you know, it is the next day and I am still tired! :)

Harold