View Issue Details

IDProjectCategoryView StatusLast Update
0034014LazarusLCLpublic2020-02-12 10:09
ReporterjdelauneyAssigned ToMartin Friebe 
PrioritynormalSeverityminorReproducibilityalways
Status resolvedResolutionfixed 
PlatformPCOSWindowsOS Version10
Product Version1.8.5 (SVN)Product Build58023M 
Target VersionFixed in Version2.2 
Summary0034014: ScrollBar Min/Max/Enabled properties wrong behaviour
DescriptionWhen Scrollbar's min and / or max property is changed by code, the enabled property is automatically set to true. It will not do that.
By forcing enabled to false just after by code do nothing. Enabled stay on True.
Steps To ReproduceSee the attached sample
Additional InformationNo tested on other OS and other Widgetset
TagsNo tags attached.
Fixed in Revision
LazTarget2.2
WidgetsetWin32/Win64
Attached Files
  • scrollbarbug.zip (1,609,669 bytes)
  • ScrollBar_Bug0034014.7z (2,459 bytes)
  • ScrollBar180729.patch (5,587 bytes)
    Index: lcl/include/scrollbar.inc
    ===================================================================
    --- lcl/include/scrollbar.inc	(revision 58640)
    +++ lcl/include/scrollbar.inc	(working copy)
    @@ -1,4 +1,4 @@
    -{%MainUnit ../stdctrls.pp}
    +{%MainUnit ../stdctrls.pp} {At revision: 58600}
     
     {
      TCustomScrollBar
    @@ -93,40 +93,80 @@
     procedure TCustomScrollBar.SetParams(APosition, AMin, AMax, APageSize: Integer);
     var
       ScrollInfo: TScrollInfo;
    +  lEnabled : boolean;
    +  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) then begin
         FMin := AMin;
    +    lfMask := lfMask or SIF_Range;
    +  end;
    +  if (FMax <> AMax) then begin
         FMax := AMax;
    +    lfMask := lfMask or SIF_Range;
    +  end;
    +  if (lfMask and SIF_POS)<>0 then begin // Only if position change requested
    +    if not NotRightToLeft then          // Review this code for correctness
    +      APosition := FMax - APosition;
    +    FPosition := 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
    +    lEnabled := IsWindowEnabled(Handle); // Dont use IsEnabled, because Form could
    +                                         // be disabled due to form Disabled.
    +    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);
    +      APosition := SetScrollInfo(Handle, SB_CTL, ScrollInfo, lEnabled);
    +      { Was position changed as a consequence of SetScrollInfo }
    +      if (APosition<>FPosition) then begin
    +        if ((lfMask and SIF_POS) = 0) then // SIF_POS not requested, Ping-Pong
    +          Position := APosition  // actualisation of Position to force .Change
    +        else begin               // Ajust FPosition to new position
    +          if not NotRightToLeft then       // Review this code for correctness
    +            APosition := FMax - APosition;
    +          FPosition := APosition;
    +        end;
    +      end;
         end;
       end;
    -  if FPosition <> APosition then
    -  begin
    -    FPosition := APosition;
    -    if HandleAllocated then
    +
    +{~bk not needed in windows, qt5}
    +{$IFNDEF Win32 and not defined(Qt5)}
    +  if (FPosition <> APosition) then begin
    +    if HandleAllocated then begin
    +      ScrollInfo.fMask := SIF_POS;
           if NotRightToLeft then
    -        SetScrollPos(Handle, SB_CTL, FPosition, True)
    +        ScrollInfo.nPos := APosition
           else
    -        SetScrollPos(Handle, SB_CTL, FMax - FPosition, True);
    -    Change;
    +        ScrollInfo.nPos := FMax - FPosition;
    +      FPosition := SetScrollInfo(Handle, SB_CTL, ScrollInfo, lEnabled);
    +    end
    +    else
    +      FPosition := APosition;
       end;
    -
    -
       if HandleAllocated then
         TWSScrollBarClass(WidgetSetClass).SetParams(Self);
    +{$ENDIF}
     end;
     
     procedure TCustomScrollBar.SetParams(APosition, AMin, AMax: Integer);
    @@ -145,24 +185,31 @@
         PreferredWidth:=GetSystemMetrics(SM_CYVSCROLL);
     end;
     
    +{ FMin, FMax, FPosition and FPage updated by SetParams }
     procedure TCustomScrollBar.SetPosition(Value: Integer);
     begin
    -  SetParams(Value, FMin, FMax, FPageSize);
    +  if Value<>FPosition then begin
    +    SetParams(Value, FMin, FMax, FPageSize);
    +    Change;  // Do it only when position changed
    +  end;
     end;
     
     procedure TCustomScrollBar.SetPageSize(Value: Integer);
     begin
    -  SetParams(FPosition, FMin, FMax, Value);
    +  if Value<>FPageSize then
    +    SetParams(FPosition, FMin, FMax, Value);
     end;
     
     procedure TCustomScrollBar.SetMin(Value: Integer);
     begin
    -  SetParams(FPosition, Value, FMax, FPageSize);
    +  if Value<>FMin then
    +    SetParams(FPosition, Value, FMax, FPageSize);
     end;
     
     procedure TCustomScrollBar.SetMax(Value: Integer);
     begin
    -  SetParams(FPosition, FMin, Value, FPageSize);
    +  if Value<>FMax then
    +    SetParams(FPosition, FMin, Value, FPageSize);
     end;
     
     procedure TCustomScrollBar.Change;
    @@ -255,6 +302,13 @@
       DefaultHandler(Message);
     end;
     
    +{ When enabling, need to resync client application }
    +procedure TCustomScrollBar.CMEnabledChanged(var Message: TLMessage);
    +begin
    +  inherited;
    +  Change;
    +end;
    +
     class procedure TCustomScrollBar.WSRegisterClass;
     begin
       inherited WSRegisterClass;
    Index: lcl/stdctrls.pp
    ===================================================================
    --- lcl/stdctrls.pp	(revision 58640)
    +++ lcl/stdctrls.pp	(working copy)
    @@ -87,6 +87,7 @@
         procedure CNVScroll(var Message: TLMVScroll); message LM_VSCROLL;
         procedure CNCtlColorScrollBar(var Message: TLMessage); message CN_CTLCOLORSCROLLBAR;
         procedure WMEraseBkgnd(var Message: TLMEraseBkgnd); message LM_ERASEBKGND;
    +    procedure CMEnabledChanged(var Message: TLMessage); message CM_ENABLEDCHANGED;
       protected
         class procedure WSRegisterClass; override;
         class function GetControlClassDefaultSize: TSize; override;
    
    ScrollBar180729.patch (5,587 bytes)
  • scrollbar.inc.patch (429 bytes)
    Index: lcl/include/scrollbar.inc
    ===================================================================
    --- lcl/include/scrollbar.inc	(revision 59194)
    +++ lcl/include/scrollbar.inc	(working copy)
    @@ -101,6 +101,7 @@
       if APageSize < 0 then APageSize := 0;
       if (FMin <> AMin) or (FMax <> AMax) or (APageSize <> FPageSize) then
       begin
    +    Enabled := True;
         FMin := AMin;
         FMax := AMax;
         FPageSize := APageSize;
    
    scrollbar.inc.patch (429 bytes)

Relationships

related to 0036429 closedMartin Friebe Behavior of ScrollBar Enabled property vs hwnd.IsWindowEnabled during scroll position change. 

Activities

jdelauney

2018-07-20 20:50

reporter  

scrollbarbug.zip (1,609,669 bytes)

BrunoK

2018-07-29 15:00

reporter   ~0109738

ScrollBar_Bug0034014.7z revised example for testing. Original loaded from Lazarus forum.

ScrollBar180729.patch tested for a week in win32 and briefly with Qt5.

BrunoK

2018-07-29 15:01

reporter  

ScrollBar_Bug0034014.7z (2,459 bytes)

BrunoK

2018-07-29 15:01

reporter  

ScrollBar180729.patch (5,587 bytes)
Index: lcl/include/scrollbar.inc
===================================================================
--- lcl/include/scrollbar.inc	(revision 58640)
+++ lcl/include/scrollbar.inc	(working copy)
@@ -1,4 +1,4 @@
-{%MainUnit ../stdctrls.pp}
+{%MainUnit ../stdctrls.pp} {At revision: 58600}
 
 {
  TCustomScrollBar
@@ -93,40 +93,80 @@
 procedure TCustomScrollBar.SetParams(APosition, AMin, AMax, APageSize: Integer);
 var
   ScrollInfo: TScrollInfo;
+  lEnabled : boolean;
+  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) then begin
     FMin := AMin;
+    lfMask := lfMask or SIF_Range;
+  end;
+  if (FMax <> AMax) then begin
     FMax := AMax;
+    lfMask := lfMask or SIF_Range;
+  end;
+  if (lfMask and SIF_POS)<>0 then begin // Only if position change requested
+    if not NotRightToLeft then          // Review this code for correctness
+      APosition := FMax - APosition;
+    FPosition := 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
+    lEnabled := IsWindowEnabled(Handle); // Dont use IsEnabled, because Form could
+                                         // be disabled due to form Disabled.
+    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);
+      APosition := SetScrollInfo(Handle, SB_CTL, ScrollInfo, lEnabled);
+      { Was position changed as a consequence of SetScrollInfo }
+      if (APosition<>FPosition) then begin
+        if ((lfMask and SIF_POS) = 0) then // SIF_POS not requested, Ping-Pong
+          Position := APosition  // actualisation of Position to force .Change
+        else begin               // Ajust FPosition to new position
+          if not NotRightToLeft then       // Review this code for correctness
+            APosition := FMax - APosition;
+          FPosition := APosition;
+        end;
+      end;
     end;
   end;
-  if FPosition <> APosition then
-  begin
-    FPosition := APosition;
-    if HandleAllocated then
+
+{~bk not needed in windows, qt5}
+{$IFNDEF Win32 and not defined(Qt5)}
+  if (FPosition <> APosition) then begin
+    if HandleAllocated then begin
+      ScrollInfo.fMask := SIF_POS;
       if NotRightToLeft then
-        SetScrollPos(Handle, SB_CTL, FPosition, True)
+        ScrollInfo.nPos := APosition
       else
-        SetScrollPos(Handle, SB_CTL, FMax - FPosition, True);
-    Change;
+        ScrollInfo.nPos := FMax - FPosition;
+      FPosition := SetScrollInfo(Handle, SB_CTL, ScrollInfo, lEnabled);
+    end
+    else
+      FPosition := APosition;
   end;
-
-
   if HandleAllocated then
     TWSScrollBarClass(WidgetSetClass).SetParams(Self);
+{$ENDIF}
 end;
 
 procedure TCustomScrollBar.SetParams(APosition, AMin, AMax: Integer);
@@ -145,24 +185,31 @@
     PreferredWidth:=GetSystemMetrics(SM_CYVSCROLL);
 end;
 
+{ FMin, FMax, FPosition and FPage updated by SetParams }
 procedure TCustomScrollBar.SetPosition(Value: Integer);
 begin
-  SetParams(Value, FMin, FMax, FPageSize);
+  if Value<>FPosition then begin
+    SetParams(Value, FMin, FMax, FPageSize);
+    Change;  // Do it only when position changed
+  end;
 end;
 
 procedure TCustomScrollBar.SetPageSize(Value: Integer);
 begin
-  SetParams(FPosition, FMin, FMax, Value);
+  if Value<>FPageSize then
+    SetParams(FPosition, FMin, FMax, Value);
 end;
 
 procedure TCustomScrollBar.SetMin(Value: Integer);
 begin
-  SetParams(FPosition, Value, FMax, FPageSize);
+  if Value<>FMin then
+    SetParams(FPosition, Value, FMax, FPageSize);
 end;
 
 procedure TCustomScrollBar.SetMax(Value: Integer);
 begin
-  SetParams(FPosition, FMin, Value, FPageSize);
+  if Value<>FMax then
+    SetParams(FPosition, FMin, Value, FPageSize);
 end;
 
 procedure TCustomScrollBar.Change;
@@ -255,6 +302,13 @@
   DefaultHandler(Message);
 end;
 
+{ When enabling, need to resync client application }
+procedure TCustomScrollBar.CMEnabledChanged(var Message: TLMessage);
+begin
+  inherited;
+  Change;
+end;
+
 class procedure TCustomScrollBar.WSRegisterClass;
 begin
   inherited WSRegisterClass;
Index: lcl/stdctrls.pp
===================================================================
--- lcl/stdctrls.pp	(revision 58640)
+++ lcl/stdctrls.pp	(working copy)
@@ -87,6 +87,7 @@
     procedure CNVScroll(var Message: TLMVScroll); message LM_VSCROLL;
     procedure CNCtlColorScrollBar(var Message: TLMessage); message CN_CTLCOLORSCROLLBAR;
     procedure WMEraseBkgnd(var Message: TLMEraseBkgnd); message LM_ERASEBKGND;
+    procedure CMEnabledChanged(var Message: TLMessage); message CM_ENABLEDCHANGED;
   protected
     class procedure WSRegisterClass; override;
     class function GetControlClassDefaultSize: TSize; override;
ScrollBar180729.patch (5,587 bytes)

BrunoK

2018-07-29 15:02

reporter   ~0109739

Additionnly Qt5 sets range [min, max] and win32 sets range [min, max[

Juha Manninen

2018-07-31 20:51

developer   ~0109794

Last edited: 2018-07-31 20:52

View 3 revisions

The patch is not good because it adds widgetset dependent code and IFDEFs into LCL code. Which widgetsets did you test?
I guess you have access to at least GTK2. How about Mac?
I believe the code can be refactored and the parts moved to widgetset classes.

 -{%MainUnit ../stdctrls.pp}
 +{%MainUnit ../stdctrls.pp} {At revision: 58600}
Did you add the comment yourself or was it added by some tool?

BrunoK

2018-08-01 14:26

reporter   ~0109812

> The patch is not good because it adds widgetset dependent code and IFDEFs into LCL code. Which widgetsets did you test?
I use it in my lazarus Win32 (Windows 10) environment, now for 10 days.
Tested briefly with Qt5 and that was hellish to install.

> I believe the code can be refactored and the parts moved to widgetset classes.
Actually if you drop the block in where I exclude code that seems totally useless, SetSCrollInfo is already redirected to WidgetSet.SetSCrollInfo(Handle, SBStyle, ScrollInfo, Redraw)

> +{%MainUnit ../stdctrls.pp} {At revision: 58600}
That's an indication I added of what was the trunk version when I checked out to build the patch with TortoiseSvn.

stdctrls : The patch in stdctrls prevents excessive and useless calls to TCustomScrollBar.SetParams and also provide logical and usable notification of changes.

A note :
I have difficulty keeping in sync with trunk, because my revisions regarding my functional and logical approach to EnableWindow processing on Win32 have been butchered with dubious patches.

Juha Manninen

2018-08-03 10:43

developer   ~0109852

Last edited: 2018-08-03 10:57

View 3 revisions

> Actually if you drop the block in where I exclude code that seems totally useless,
> SetSCrollInfo is already redirected to WidgetSet.SetSCrollInfo(Handle, SBStyle, ScrollInfo, Redraw)

What does it mean? Can the block be removed?

> That's an indication I added of what was the trunk version when I checked out to build the patch with TortoiseSvn.

Such comments are not needed. New commits always go on top of older ones. You should use as recent revision as possible to avoid merge conflicts.

> I have difficulty keeping in sync with trunk, because my revisions regarding my functional and
> logical approach to EnableWindow processing on Win32 have been butchered with dubious patches.

Which revisions and dubious patches? Is that somehow related to this ScrollBar issue?

BrunoK

2018-08-03 13:36

reporter   ~0109853

>> What does it mean? Can the block be removed?
Yes, I think so.

>>Such comments are not needed. New commits always go on top of older ones. You should use as recent revision as possible to avoid merge conflicts.
I make an effort to supply patches relative to current trunk, even if my units are 'Unversion and add to the ignore list'. Many units do not have an indication of their last revision in the first line.

>> Which revisions and dubious patches? Is that somehow related to this ScrollBar issue?
It is related because it concerns HWND.'Enabled' which is not TWinControl.Enabled, a problem of logic.
>> dubious patches?
Revision : 58448, 58497 Changes done to try to circumvent a logical error in handling of HWND.Enabled consequent to changes in Windows 10 that sends WM_ENABLE notification even if there is no change.
TWinControl.inc should be modified with the greatest care.
58448 Removes EnableWindow in InitializeWnd for all WidgetSets but the problem concerns ONLY Windows, removing this call is pretty likely to impact other WidgetSets.
58497 Replaces the default behaviour, like in Delphi, that TWinControls HWNDs are created by default in the enabled state.

Juha Manninen

2018-08-04 23:13

developer   ~0109884

The ScrollBar_Bug0034014.7z demo misuses "Other unit files (-Fu)" and "Include files (-Fi)" for widgetset (Win32) specific paths. Don't do that please.

> I make an effort to supply patches relative to current trunk, even if my units are 'Unversion and add to the ignore list'.

I don't understand. Only files not under revision control should be in the ignore list.

> Many units do not have an indication of their last revision in the first line.

Right, obviously not. Their revision history can be seen using the revision control tools.

r58448 and r58497 are by Michl. I don't understand the issue enough to see how it affects ScrollBar, thus assigning to Michl.

Michl

2018-09-30 21:31

developer   ~0111126

@jdelauney: Can you explain, why you want set Min and Max properties and don't want to show it?

Delphi shows the same behaviour as the LCL TScrollBar now.

If there is no real good reason, I'll force to fix it with "No change required".

Michl

2018-09-30 22:59

developer   ~0111129

Last edited: 2018-09-30 23:03

View 3 revisions

Ah, I missed the point, that the scrollbar is enabled but ScrollBar.Enabled is not set. This can be fixed by setting Enabled to True in TCustomScrollBar.SetParams. Then it seems to be real Delphi compatible.

See added scrollbar.inc.patch.

Michl

2018-09-30 23:01

developer  

scrollbar.inc.patch (429 bytes)
Index: lcl/include/scrollbar.inc
===================================================================
--- lcl/include/scrollbar.inc	(revision 59194)
+++ lcl/include/scrollbar.inc	(working copy)
@@ -101,6 +101,7 @@
   if APageSize < 0 then APageSize := 0;
   if (FMin <> AMin) or (FMax <> AMax) or (APageSize <> FPageSize) then
   begin
+    Enabled := True;
     FMin := AMin;
     FMax := AMax;
     FPageSize := APageSize;
scrollbar.inc.patch (429 bytes)

Martin Friebe

2020-02-12 10:09

manager   ~0121042

resolved with related issue

Issue History

Date Modified Username Field Change
2018-07-20 20:50 jdelauney New Issue
2018-07-20 20:50 jdelauney File Added: scrollbarbug.zip
2018-07-29 15:00 BrunoK Note Added: 0109738
2018-07-29 15:01 BrunoK File Added: ScrollBar_Bug0034014.7z
2018-07-29 15:01 BrunoK File Added: ScrollBar180729.patch
2018-07-29 15:02 BrunoK Note Added: 0109739
2018-07-31 20:51 Juha Manninen Note Added: 0109794
2018-07-31 20:52 Juha Manninen Note Edited: 0109794 View Revisions
2018-07-31 20:52 Juha Manninen Note Edited: 0109794 View Revisions
2018-08-01 14:26 BrunoK Note Added: 0109812
2018-08-03 10:43 Juha Manninen Note Added: 0109852
2018-08-03 10:46 Juha Manninen Note Edited: 0109852 View Revisions
2018-08-03 10:57 Juha Manninen Note Edited: 0109852 View Revisions
2018-08-03 13:36 BrunoK Note Added: 0109853
2018-08-04 23:13 Juha Manninen Note Added: 0109884
2018-08-04 23:13 Juha Manninen Assigned To => Michl
2018-08-04 23:13 Juha Manninen Status new => assigned
2018-09-30 21:31 Michl LazTarget => -
2018-09-30 21:31 Michl Note Added: 0111126
2018-09-30 21:31 Michl Status assigned => feedback
2018-09-30 22:59 Michl Note Added: 0111129
2018-09-30 23:01 Michl File Added: scrollbar.inc.patch
2018-09-30 23:02 Michl Note Edited: 0111129 View Revisions
2018-09-30 23:03 Michl Note Edited: 0111129 View Revisions
2019-12-12 18:45 Martin Friebe Relationship added related to 0036429
2020-02-12 10:09 Martin Friebe Assigned To Michl => Martin Friebe
2020-02-12 10:09 Martin Friebe Status feedback => resolved
2020-02-12 10:09 Martin Friebe Resolution open => fixed
2020-02-12 10:09 Martin Friebe Fixed in Version => 2.2
2020-02-12 10:09 Martin Friebe LazTarget - => 2.2
2020-02-12 10:09 Martin Friebe Widgetset Win32/Win64 => Win32/Win64
2020-02-12 10:09 Martin Friebe Note Added: 0121042