View Issue Details

IDProjectCategoryView StatusLast Update
0036429LazarusLCLpublic2019-12-13 11:42
ReporterBrunoKAssigned To 
PrioritynormalSeverityminorReproducibilityhave not tried
Status newResolutionopen 
Platformintel 386/64OSwindowsOS Version[XP..10]
Product Version2.0.7 (SVN)Product Build 
Target VersionFixed in Version 
Summary0036429: Behavior of ScrollBar Enabled property vs hwnd.IsWindowEnabled during scroll position change.
DescriptionSee 0034014
Steps To ReproduceSee 0034014
Additional InformationProposed patch tested on windows and cross verified on GTK2. Not tested on QT. For additional info look comments in patch file.
Note : I have found many places in the trunk where ScrollBar got a bit hacked due to this issue.
Hope concerned developers will have a sharp critical eye on this.
TagsNo tags attached.
Fixed in Revision
LazTarget
WidgetsetWin32/Win64
Attached Files
  • scrollbar_on_r62380.patch (4,340 bytes)
    Index: lcl/include/scrollbar.inc
    ===================================================================
    --- lcl/include/scrollbar.inc	(revision 62379)
    +++ lcl/include/scrollbar.inc	(working copy)
    @@ -91,40 +91,59 @@
     end;
     
     procedure TCustomScrollBar.SetParams(APosition, AMin, AMax, APageSize: Integer);
    +  { Was position changed as a consequence of SetScrollInfo }
    +  procedure UpdatePositionChange;
    +  begin
    +    if (APosition<>FPosition) then begin
    +      FPosition := APosition;
    +      Change;
    +    end;
    +  end;
     var
       ScrollInfo: TScrollInfo;
    +  lfMask : UINT = 0;
     begin
       if AMax < AMin then
         raise EInvalidOperation.Create(rsScrollBarOutOfRange);
    +
    +  { Test if SetParams was called thru a .SetPosition, before eventual adjustment }
    +  if (FPosition <> APosition) then
    +    lfMask := lfMask or SIF_POS;
    +  { Check position within range }
       if APosition < AMin then APosition := AMin;
       if APosition > AMax then APosition := AMax;
    +
       if APageSize < 0 then APageSize := 0;
    -  if (FMin <> AMin) or (FMax <> AMax) or (APageSize <> FPageSize) then
    -  begin
    +
    +  { Test values that changed }
    +  if (FMin <> AMin) or (FMax <> AMax) then begin
         FMin := AMin;
         FMax := AMax;
    +    lfMask := lfMask or SIF_Range;
    +  end;
    +  if (lfMask and SIF_POS)<>0 then begin // TODO : NotRightToLeft, please Review
    +    if not NotRightToLeft then          //        this code for correctness
    +      APosition := FMax - APosition;
    +  end;
    +  if (FPageSize <> APageSize) then begin
    +    lfMask := lfMask or SIF_Range;
         FPageSize := APageSize;
    -    if HandleAllocated then
    -    begin
    -      ScrollInfo.fMask := SIF_PAGE or SIF_Range;
    +  end;
    +  if HandleAllocated then begin
    +    if ((lfMask and (SIF_PAGE or SIF_Range or SIF_POS))<>0) then begin
    +      ScrollInfo.fMask := lfMask;
           ScrollInfo.nMin := AMin;
           ScrollInfo.nMax := AMax;
    +      ScrollInfo.nPos := APosition;
           ScrollInfo.nPage := APageSize;
    -      SetScrollInfo(Handle, SB_CTL, ScrollInfo, FPosition = APosition);
    +      { Last parameter of SetScrollInfo is meaningless }
    +      APosition := SetScrollInfo(Handle, SB_CTL, ScrollInfo, True);
    +      UpdatePositionChange;
         end;
    -  end;
    -  if FPosition <> APosition then
    -  begin
    -    FPosition := APosition;
    -    if HandleAllocated then
    -      if NotRightToLeft then
    -        SetScrollPos(Handle, SB_CTL, FPosition, True)
    -      else
    -        SetScrollPos(Handle, SB_CTL, FMax - FPosition, True);
    -    Change;
    -  end;
    -  if HandleAllocated then
         TWSScrollBarClass(WidgetSetClass).SetParams(Self);
    +  end
    +  else
    +    UpdatePositionChange;
     end;
     
     procedure TCustomScrollBar.SetParams(APosition, AMin, AMax: Integer);
    Index: lcl/interfaces/win32/win32winapi.inc
    ===================================================================
    --- lcl/interfaces/win32/win32winapi.inc	(revision 62137)
    +++ lcl/interfaces/win32/win32winapi.inc	(working copy)
    @@ -3375,7 +3375,9 @@
       ScrollInfo.cbSize:=sizeof(ScrollInfo);
       if (ScrollInfo.fMask and SIF_Range > 0) then
         ScrollInfo.nMax := Max(ScrollInfo.nMin, ScrollInfo.nMax - 1);
    -  Result := Windows.SetScrollInfo(Handle, SBStyle, @ScrollInfo, BRedraw);
    +  { ~bk passing BRedraw True/False might change the windows visual enabled state
    +        that would not be reflected in TScollBar.Enabled property }
    +  Result := Windows.SetScrollInfo(Handle, SBStyle, @ScrollInfo, WidgetSet.IsWindowEnabled(Handle));
     end;
     
     {------------------------------------------------------------------------------
    Index: lcl/interfaces/win32/win32wsstdctrls.pp
    ===================================================================
    --- lcl/interfaces/win32/win32wsstdctrls.pp	(revision 62137)
    +++ lcl/interfaces/win32/win32wsstdctrls.pp	(working copy)
    @@ -488,6 +488,12 @@
       ScrollInfo: TScrollInfo;
       AMax: Integer;
     begin
    +  { ~bk 2019.12.11 Completly disabled this code.
    +     https://docs.microsoft.com/en-us/windows/win32/controls/sbm-setscrollinfo
    +     says that "they should use the SetScrollInfo function".
    +     In our case, the SetScrollInfo function has already been completely handled
    +     in TCustomScrollBar.SetParams }
    +  {
       with AScrollBar do
       begin
         AMax := Max - 1;
    @@ -509,6 +515,7 @@
             SetWindowLong(Handle, GWL_STYLE, GetWindowLong(Handle, GWL_STYLE) or SBS_VERT);
         end;
       end;
    +  }
     end;
     
     { TWin32WSCustomGroupBox }
    
  • Horz_scroll.zip (6,169 bytes)

Relationships

related to 0034014 feedbackMichl ScrollBar Min/Max/Enabled properties wrong behaviour 

Activities

BrunoK

2019-12-12 17:14

reporter  

scrollbar_on_r62380.patch (4,340 bytes)
Index: lcl/include/scrollbar.inc
===================================================================
--- lcl/include/scrollbar.inc	(revision 62379)
+++ lcl/include/scrollbar.inc	(working copy)
@@ -91,40 +91,59 @@
 end;
 
 procedure TCustomScrollBar.SetParams(APosition, AMin, AMax, APageSize: Integer);
+  { Was position changed as a consequence of SetScrollInfo }
+  procedure UpdatePositionChange;
+  begin
+    if (APosition<>FPosition) then begin
+      FPosition := APosition;
+      Change;
+    end;
+  end;
 var
   ScrollInfo: TScrollInfo;
+  lfMask : UINT = 0;
 begin
   if AMax < AMin then
     raise EInvalidOperation.Create(rsScrollBarOutOfRange);
+
+  { Test if SetParams was called thru a .SetPosition, before eventual adjustment }
+  if (FPosition <> APosition) then
+    lfMask := lfMask or SIF_POS;
+  { Check position within range }
   if APosition < AMin then APosition := AMin;
   if APosition > AMax then APosition := AMax;
+
   if APageSize < 0 then APageSize := 0;
-  if (FMin <> AMin) or (FMax <> AMax) or (APageSize <> FPageSize) then
-  begin
+
+  { Test values that changed }
+  if (FMin <> AMin) or (FMax <> AMax) then begin
     FMin := AMin;
     FMax := AMax;
+    lfMask := lfMask or SIF_Range;
+  end;
+  if (lfMask and SIF_POS)<>0 then begin // TODO : NotRightToLeft, please Review
+    if not NotRightToLeft then          //        this code for correctness
+      APosition := FMax - APosition;
+  end;
+  if (FPageSize <> APageSize) then begin
+    lfMask := lfMask or SIF_Range;
     FPageSize := APageSize;
-    if HandleAllocated then
-    begin
-      ScrollInfo.fMask := SIF_PAGE or SIF_Range;
+  end;
+  if HandleAllocated then begin
+    if ((lfMask and (SIF_PAGE or SIF_Range or SIF_POS))<>0) then begin
+      ScrollInfo.fMask := lfMask;
       ScrollInfo.nMin := AMin;
       ScrollInfo.nMax := AMax;
+      ScrollInfo.nPos := APosition;
       ScrollInfo.nPage := APageSize;
-      SetScrollInfo(Handle, SB_CTL, ScrollInfo, FPosition = APosition);
+      { Last parameter of SetScrollInfo is meaningless }
+      APosition := SetScrollInfo(Handle, SB_CTL, ScrollInfo, True);
+      UpdatePositionChange;
     end;
-  end;
-  if FPosition <> APosition then
-  begin
-    FPosition := APosition;
-    if HandleAllocated then
-      if NotRightToLeft then
-        SetScrollPos(Handle, SB_CTL, FPosition, True)
-      else
-        SetScrollPos(Handle, SB_CTL, FMax - FPosition, True);
-    Change;
-  end;
-  if HandleAllocated then
     TWSScrollBarClass(WidgetSetClass).SetParams(Self);
+  end
+  else
+    UpdatePositionChange;
 end;
 
 procedure TCustomScrollBar.SetParams(APosition, AMin, AMax: Integer);
Index: lcl/interfaces/win32/win32winapi.inc
===================================================================
--- lcl/interfaces/win32/win32winapi.inc	(revision 62137)
+++ lcl/interfaces/win32/win32winapi.inc	(working copy)
@@ -3375,7 +3375,9 @@
   ScrollInfo.cbSize:=sizeof(ScrollInfo);
   if (ScrollInfo.fMask and SIF_Range > 0) then
     ScrollInfo.nMax := Max(ScrollInfo.nMin, ScrollInfo.nMax - 1);
-  Result := Windows.SetScrollInfo(Handle, SBStyle, @ScrollInfo, BRedraw);
+  { ~bk passing BRedraw True/False might change the windows visual enabled state
+        that would not be reflected in TScollBar.Enabled property }
+  Result := Windows.SetScrollInfo(Handle, SBStyle, @ScrollInfo, WidgetSet.IsWindowEnabled(Handle));
 end;
 
 {------------------------------------------------------------------------------
Index: lcl/interfaces/win32/win32wsstdctrls.pp
===================================================================
--- lcl/interfaces/win32/win32wsstdctrls.pp	(revision 62137)
+++ lcl/interfaces/win32/win32wsstdctrls.pp	(working copy)
@@ -488,6 +488,12 @@
   ScrollInfo: TScrollInfo;
   AMax: Integer;
 begin
+  { ~bk 2019.12.11 Completly disabled this code.
+     https://docs.microsoft.com/en-us/windows/win32/controls/sbm-setscrollinfo
+     says that "they should use the SetScrollInfo function".
+     In our case, the SetScrollInfo function has already been completely handled
+     in TCustomScrollBar.SetParams }
+  {
   with AScrollBar do
   begin
     AMax := Max - 1;
@@ -509,6 +515,7 @@
         SetWindowLong(Handle, GWL_STYLE, GetWindowLong(Handle, GWL_STYLE) or SBS_VERT);
     end;
   end;
+  }
 end;
 
 { TWin32WSCustomGroupBox }

Martin Friebe

2019-12-12 18:44

manager   ~0119801

Last edited: 2019-12-12 18:47

View 2 revisions

What are the issues/bugs that occur with the current scrollbars?
related issue is about scrollbar.pos? This is about min/max?



Optional: Where are the "hack-ish code" you found? (So that can be remedied too)

BrunoK

2019-12-12 21:37

reporter   ~0119806

Sorry for the term of bit hacked code, I should to have written "met with some difficulties" .

The dump of results of tracing the code in SetParams in the current trunk, is the following one :

0 SetParams ENTRY ScrollBar1 NO HANDLE Enabled : FALSE

0 SetParams ENTRY ScrollBar1 Enabled : FALSE WidgetSet.IsWindowEnabled : FALSE
0 Before SetScrollInfo ScrollBar1 Enabled : FALSE WidgetSet.IsWindowEnabled : FALSE
0 After SetScrollInfo ScrollBar1 Enabled : FALSE WidgetSet.IsWindowEnabled : FALSE
0 Before SetScrollPos ScrollBar1 Enabled : FALSE WidgetSet.IsWindowEnabled : FALSE
0 After SetScrollPos ScrollBar1 Enabled : FALSE WidgetSet.IsWindowEnabled : TRUE -> MISMATCH
0 Before (WidgetSetClass).SetParams(Self) ScrollBar1 Enabled : FALSE WidgetSet.IsWindowEnabled : TRUE -> MISMATCH
0 After (WidgetSetClass).SetParams(Self) ScrollBar1 Enabled : FALSE WidgetSet.IsWindowEnabled : TRUE -> MISMATCH

0 is the depth of recursion, no recursion there.

BrunoK

2019-12-13 11:42

reporter   ~0119814

Lets add a very simplified project in attachement, (a simplified version of the one in 0034014). In the scrollbar.inc you get the code that generated the BrunoK 2019-12-12 21:37 dump of results.
Just having the lcl/interfaces/win32/win32winapi.inc and lcl/interfaces/win32/win32wsstdctrls.pp patches is enough to suppress the bug, but the patch to SetParams(APosition, AMin, AMax, APageSize: Integer); using APosition := SetScrollInfo(Handle, SB_CTL, ScrollInfo, True); to completely set the ScrollBar is, in my opinion, more concise (as long as there aren't any bug in it).

BRedraw parameter means in fact that Windows should Enable/Disable the control state.

I have used a somewhat more primitive version since july/august 2018 to build my lazarus versions without having found a problem. This one wasn't linked to problems in win 10 release 1803 that were finally solved in revision 60224.

Horz_scroll.zip (6,169 bytes)

Issue History

Date Modified Username Field Change
2019-12-12 17:14 BrunoK New Issue
2019-12-12 17:14 BrunoK File Added: scrollbar_on_r62380.patch
2019-12-12 18:44 Martin Friebe Note Added: 0119801
2019-12-12 18:45 Martin Friebe Relationship added related to 0034014
2019-12-12 18:47 Martin Friebe Note Edited: 0119801 View Revisions
2019-12-12 21:37 BrunoK Note Added: 0119806
2019-12-13 11:42 BrunoK File Added: Horz_scroll.zip
2019-12-13 11:42 BrunoK Note Added: 0119814