Re: Clean-up of scroll.c (1/4)

[Alexandre Julliard]

"Andrew M. Johnston"  writes:
> - return (info->fMask & SIF_ALL) != 0;
> -
> - return (info->fMask & SIF_ALL);

[...]
> - if (lpMin) *lpMin = infoPtr->minVal;
> - if (lpMax) *lpMax = infoPtr->maxVal;
> - return TRUE;
> - TRACE("hwnd=%p nBar=%d lpMin=%p lpMax=%p\n", hwnd, nBar, lpMin, lpMax);
> -
> - if (lpMin) *lpMin = infoPtr ? infoPtr->minVal : 0;
> - if (lpMax) *lpMax = infoPtr ? infoPtr->maxVal : 0;
> -
> - return (BOOL)infoPtr;

Please don't do this kind of gratuitous changes. Boolean functions
should return TRUE or FALSE, unless you have evidence that Windows
does it differently. Yes, I know any non-zero value is true in C, but
you can't rely on every Windows programmer to get it right.

Clean-up of scroll.c (1/4)

[Andrew M. Johnston]

> > - return (info->fMask & SIF_ALL) != 0;
> > -
> > - return (info->fMask & SIF_ALL);

Will Fix
> > - if (lpMin) *lpMin = infoPtr->minVal;
> > - if (lpMax) *lpMax = infoPtr->maxVal;
> > - return TRUE;
> > - TRACE("hwnd=%p nBar=%d lpMin=%p lpMax=%p\n", hwnd, nBar, lpMin,
> >
> > lpMax); +
> > - if (lpMin) *lpMin = infoPtr ? infoPtr->minVal : 0;
> > - if (lpMax) *lpMax = infoPtr ? infoPtr->maxVal : 0;

Note the code as in this area CVS seems to have a bug.  In the case of a zero 
infoPtr it zeros the return pointers not the return value as I am sure is 
intended.  A patch like the one below shown the likely intended code.
--- wine-20020710/controls/scroll.c	Sat Jun  1 07:06:46 2002
+++ wine/controls/scroll.c	Wed Jul 24 16:52:54 2002
@@ -1730,8 +1730,8 @@

if (!(infoPtr = SCROLL_GetScrollInfo( hwnd, nBar )))
     {
-        if (lpMin) lpMin = 0;
-        if (lpMax) lpMax = 0;
+        if (lpMin) *lpMin = 0;
+        if (lpMax) *lpMax = 0;
         return FALSE;
     }
     if (lpMin) *lpMin = infoPtr->MinVal;
> > - return (BOOL)infoPtr;

Will Fix

Andrew

Clean-up of scroll.c (1/4)

[Alexandre Julliard]

"Andrew M. Johnston"  writes:
> Note the code as in this area CVS seems to have a bug.  In the case of a zero 
> infoPtr it zeros the return pointers not the return value as I am sure is 
> intended.  A patch like the one below shown the likely intended code.

Yes that's definitely what was intended. Though note that even with
that fix the implementation is still not right, most of the GetScroll*
functions should be sending messages to the window for the SB_CTL case
instead of accessing the data structure directly.

Clean-up of scroll.c (1/4)

[Andrew Johnston]

On Saturday 01 March 2003 02:29, Alexandre Julliard wrote:
> Yes that's definitely what was intended. Though note that even with
> that fix the implementation is still not right, most of the GetScroll*
> functions should be sending messages to the window for the SB_CTL case
> instead of accessing the data structure directly.

I am not sure I understand correctly.  Sending a message to the window for
SB_CTL scroll bars seems to run the possibility of an infinite recursion.  The
message handling code simply uses the API functions.  Is the following what is desired?

Andrew

Index: controls/scroll.c
===================================================================
RCS file: /home/wine/wine/controls/scroll.c,v
retrieving revision 1.62
diff -u -r1.62 scroll.c
--- controls/scroll.c	14 Jan 2003 23:41:01 -0000	1.62
+++ controls/scroll.c	4 Mar 2003 07:50:54 -0000
@@ -1743,15 +1743,15 @@
 {
     SCROLLBAR_INFO *infoPtr;

-    if (!(infoPtr = SCROLL_GetScrollInfo( hwnd, nBar )))
+    /* Refer SB_CTL requests to the window */
+    if (!(infoPtr = SCROLL_GetScrollInfo(hwnd, nBar)) && nBar == SB_CTL)
+        return SendMessageA(hwnd, SBM_GETRANGE, (WPARAM)lpMin, (LPARAM)lpMax);
+    else
     {
-        if (lpMin) lpMin = 0;
-        if (lpMax) lpMax = 0;
-        return FALSE;
+        if (lpMin) *lpMin = infoPtr ? infoPtr->minVal : 0;
+        if (lpMax) *lpMax = infoPtr ? infoPtr->maxVal : 0;
     }
-    if (lpMin) *lpMin = infoPtr->minVal;
-    if (lpMax) *lpMax = infoPtr->maxVal;
-    return TRUE;
+    return infoPtr ? TRUE : FALSE;
 }

This e-mail and any attachment is for authorised use by the intended recipient(s) only.  It may contain proprietary material, confidential information and/or be subject to legal privilege.  It should not be copied, disclosed to, retained or used by, any other party.  If you are not an intended recipient then please promptly delete this e-mail and any attachment and all copies and inform the sender.  Thank you.

Clean-up of scroll.c (1/4)

[Alexandre Julliard]

Andrew Johnston  writes:
> I am not sure I understand correctly.  Sending a message to the window for
> SB_CTL scroll bars seems to run the possibility of an infinite recursion.  The
> message handling code simply uses the API functions.  Is the following what is desired?

No, the message handling has to be changed too of course. Instead of
having the message handler call the API function it has to be done the
other way around, with the API function sending a message and the
handler doing the work by accessing the info structure directly.

Clean-up of scroll.c (1/4)

[Andrew Johnston]

On Wednesday 05 March 2003 01:17, Alexandre Julliard wrote:
> No, the message handling has to be changed too of course. Instead of
> having the message handler call the API function it has to be done the
> other way around, with the API function sending a message and the
> handler doing the work by accessing the info structure directly.

Something like this?

Andrew

Index: controls/scroll.c
===================================================================
RCS file: /home/wine/wine/controls/scroll.c,v
retrieving revision 1.62
diff -u -r1.62 scroll.c
--- controls/scroll.c   14 Jan 2003 23:41:01 -0000      1.62
+++ controls/scroll.c   5 Mar 2003 02:24:05 -0000
@@ -1192,6 +1192,27 @@
 }

+/*************************************************************************
+ *           SCROLL_GetScrollRange
+ *
+ *  Internal worker function for the API function
+ *
+ * RETURNS STD
+ */
+static BOOL SCROLL_GetScrollRange(
+HWND hwnd, /* [in] Handle of window */
+INT nBar, /* [in] One of SB_HORZ, SB_VERT, or SB_CTL  */
+LPINT lpMin, /* [out] Where to store minimum value */
+LPINT lpMax /* [out] Where to store maximum value */)
+{
+    SCROLLBAR_INFO *infoPtr = SCROLL_GetScrollInfo(hwnd, nBar);
+    
+    if (lpMin) *lpMin = infoPtr ? infoPtr->minVal : 0;
+    if (lpMax) *lpMax = infoPtr ? infoPtr->maxVal : 0;
+    
+    return infoPtr ? TRUE : FALSE;
+}
+
 /***********************************************************************
  *           ScrollBarWndProc
  */
@@ -1367,7 +1388,7 @@
         return 0;

case SBM_GETRANGE:
-        GetScrollRange( hwnd, SB_CTL, (LPINT)wParam, (LPINT)lParam );
+        SCROLL_GetScrollRange( hwnd, SB_CTL, (LPINT)wParam, (LPINT)lParam );
         return 0;

case SBM_ENABLE_ARROWS16:
@@ -1741,17 +1762,11 @@
 LPINT lpMin, /* [out] Where to store minimum value */
 LPINT lpMax /* [out] Where to store maximum value */)
 {
-    SCROLLBAR_INFO *infoPtr;
-
-    if (!(infoPtr = SCROLL_GetScrollInfo( hwnd, nBar )))
-    {
-        if (lpMin) lpMin = 0;
-        if (lpMax) lpMax = 0;
-        return FALSE;
-    }
-    if (lpMin) *lpMin = infoPtr->minVal;
-    if (lpMax) *lpMax = infoPtr->maxVal;
-    return TRUE;
+    /* Refer SB_CTL requests to the window */
+    if (nBar == SB_CTL)
+        return SendMessageA(hwnd, SBM_GETRANGE, (WPARAM)lpMin, (LPARAM)lpMax);
+    else
+        return SCROLL_GetScrollRange(hwnd, nBar, lpMin, lpMax); 
 }

Clean-up of scroll.c (1/4)

[Alexandre Julliard]

Andrew Johnston  writes:
> Something like this?

Yes exactly. Though I'm wondering if the SBM_GETRANGE message
shouldn't be returning TRUE then. And we'll need to do that for most
of the other GetScroll* functions too.