View Issue Details

IDProjectCategoryView StatusLast Update
0036429LazarusLCLpublic2020-04-21 03:13
ReporterBrunoK Assigned ToMartin Friebe  
PrioritynormalSeverityminorReproducibilityhave not tried
Status closedResolutionfixed 
Platformintel 386/64OSwindows 
Product Version2.0.7 (SVN) 
Fixed in Version2.2 
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 Revision62613
LazTarget2.2
WidgetsetWin32/Win64
Attached Files

Relationships

related to 0034014 resolvedMartin Friebe 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 }
scrollbar_on_r62380.patch (4,340 bytes)   

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)

Martin Friebe

2020-02-07 19:53

manager   ~0120925

I applied a fix (minimum required) for the enabled issue in revision 62613.

I deliberately kept (but fixed) SetParams (Win32). It may be used in future (for example when changing horiz/vert could be done without recreating the window handle).
I admit it is questionable, if it needs to be kept. Since I only occasionally touch widgetset code, I am not calling that decision. If important to you, you can create a separate issue. This can then be picked up by another developer (if anyone feels up for it). They may then decide either way.
Yet that question may be moot anyway, see below

Martin Friebe

2020-02-07 19:54

manager   ~0120926

About the patch to scrollbox.inc.
I am general in favour of it.

I noted some issues
1) Doing
   FPosition := APosition
after
   if not NotRightToLeft then APosition := FMax - APosition;
is wrong thoug / but easily fixed

2) "change" must be called even if "not HandleAllocated"

-----
On top of this it would be good to deal with the duplicate actions due to calling both
SetScrollInfo and SetParams.

However, if any of them (and which one) can be skipped must be the decision of the widgetset.
My Idea would be to add
  TWidgetSet.SetScrollInfoAndParams (or UpdateInfoAndSetParams)
This could have a default implementation of calling both functions.
Each widgetset can then decide if this is needed. (Windows only needs SetParams, since that includes ScrollInfo)

In this case keeping win32.SetParams as it is now, would be ok.

BrunoK

2020-02-08 15:55

reporter   ~0120943

There are quite different behaviours depending on Widget sets for handling of their ScrollBar's.

1) For example Qt5 will have a position range of [Min..Max] whereas windows has [Min..Max[
2) Windows (I mean the Windows native scrollbar) is pretty limited, not being able to show the scroll position for a disabled control whereas, Gtk2 and QT5 show the position and range in non focusable / non clickable GUI control, much nicer...
3) different vision of what happens when range is non default.

> I noted some issues
> 1) Doing
> FPosition := APosition
> after
> if not NotRightToLeft then APosition := FMax - APosition;
> is wrong thoug / but easily fixed
There the idea is to detect if a position change happens consequent to SetScrollInfo call and prevent recursive call to change.

> 2) "change" must be called even if "not HandleAllocated"
It is, the last lines of the patch are to TCustomScrollBar.SetParams
are
  else
    UpdatePositionChange;
    
but effectively, the bit in 'if HandleAllocated then ' might required some thinking ... essentially if
(lfMask and (SIF_PAGE or SIF_Range or SIF_POS)) = 0 there is nothing to do and we should exit or move
UpdatePositionChange just after TWSScrollBarClass(WidgetSetClass).SetParams(Self);

> On top of this it would be good to deal with the duplicate actions due to calling both
> SetScrollInfo and SetParams.
Here we get into the problem of widget sets requiring adequate handling for themselves. In windows, SetScrollInfo adjusts all the variable parameters and also correct position taking in account the the min / max / range and returns the new position as a result to it being called, that's why I commented out all the body of/in TWin32WSScrollBar.SetParams.

> However, if any of them (and which one) can be skipped must be the decision of the widgetset.
> My Idea would be to add
> TWidgetSet.SetScrollInfoAndParams (or UpdateInfoAndSetParams)
> This could have a default implementation of calling both functions.>
> Each widgetset can then decide if this is needed. (Windows only needs >SetParams, since that includes ScrollInfo)
Yes that would be the best approach but would involve non windozer's maintainers to do some adjusting and testing. The definition of that new TWidgetSet procedure would have to be strictly defined so to be consistent across the platforms.
Many WidgetSets do return the new position from the SetScrollInfo function, but, for example, GTK2 doesn't.

Probably marking as 'Won't fix' is the easiest way of handling this, and I'll close. Applying the patch is easy for me.

Martin Friebe

2020-02-08 18:28

manager   ~0120946

1) FPosition := APosition
=> (Partly) my fault, I overlooked that APosition is re-assigned the result of SetScrollInfo.
Except:
  If Handle is NOT allocated, but NotRightToLeft is true. => Example setting pos=20 of max 100 => APosition will be 80 when assigned to FPosition (in the final else). That is wrong.

Also
  Using the result of SetScrollInfo, instead of the original APosition value, introduces a further source of error. If the Widgetset returns a wrong value.
  Currently the logic is "FPosition = last user specified value". Using the result of SetScrollInfo changes that logic (though this should always lead to the same behaviour...)

This means that if the widgetset changes (or reports the change of) position (even if FPosition = APosition) then FPosition will be updated.
And a change event will be triggered.

This would actually be more correct than what currently happens (assuming this can happen)...
However, if this can happen, it does change when the event is triggered. And that affects end-user apps (they can be very sensitive to that sort of things). So that then needs to be an incompatible change.

Using the result of SetScrollInfo should be opened as separate issue. (Not mixed with grouping the calls to the widgetset, which merely is an optimization, that is expected NOT to change behaviour)

>>> There the idea is to detect if a position change happens consequent to SetScrollInfo call and prevent recursive call to change.
Where does that currently happen?

I seem to miss a fundamental point....

--------------------
>>> { Test if SetParams was called thru a .SetPosition, before eventual adjustment }

You check (intentional?) "FPosition <> APosition" before adjusting FPosition for Min/Max.
So you have an FPosition change, even if there is no change after the adjusting.

This means you will get a call to SetScollInfo (because lfMask will be set), even if there should be no change.
  Position := MaxPos + 1
will always call SetScrollInfo.
IMHO it should not?

Martin Friebe

2020-02-08 19:41

manager   ~0120953

I added 2 sample commits https://github.com/User4martin/lazarus/commits/f-some-scrollbar-optim-issue-36429

1) Grouping the calls to SetScrollInfo.
I have (for now) left the "FPosition <> APosition" at the top. I still need to understand why

2) A possible way (proof of concept) to avoid the duplication between SetScrollInfo and SetParams.
Decided by the WS. Can be extended for each WS.
In SetScrollInfoBeforeParams each WS can perform any task that is not done in SetParams. After that SetParams will be called.
(There is no result, because it is currently not needed, and may require extra work to retrieve)

I am reluctant to add (2) as it is. I have not evaluated, if the entire interaction between LCL and widgetset can be fixed to make only one (of the two existing) calls at all.

Of course it could also be fixed by having SetScrollInfoBeforeParams (with diff name), make the calls to SetScrollInfo AND SetParams. And then only call that function.

BrunoK

2020-02-09 14:12

reporter   ~0120967

> You check (intentional?) "FPosition <> APosition" before adjusting FPosition for Min/Max.
> So you have an FPosition change, even if there is no change after the adjusting.
This is a mistake on my part ! SIF_POS should be evaluated after { Check position within range } block.

If it is of any use, you could pm me on lazarus forum or e-mail me your current patches relative to the current svn trunk, I'll test them.

Martin Friebe

2020-02-09 16:42

manager   ~0120970

Fixed the incorrect behaviour (enabling by accident) in r62613.

Martin Friebe

2020-04-21 03:13

manager   ~0122311

Committed another fix in revision 63040

This caused the following issue:
- Add a TScrollbar to a form, set Max = 2
- Add an scrollbar OnChange event handler to display the Scrollbar.Position, e.g:
    procedure TForm1.ScrollBar1Change(Sender: TObject);
    begin
      Caption := IntToStr(Scrollbar1.Position);
    end;

- Run. The scrollbar thumb is at the left. Correct.
- Click on the right arrow -> the caption displays "1", but the scrollbar thumb jumps to the right end of the scrollbar (it should go to the center position because there are 3 allowed values, 0, 1, 2).
- Click on the right arrow once more --> caption displays "2" as expected, the scrollbar does not move any more.
- In total: the scrollbar thumb reaches the right-most position at one position value too early.

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
2020-02-07 19:13 Martin Friebe Assigned To => Martin Friebe
2020-02-07 19:13 Martin Friebe Status new => assigned
2020-02-07 19:53 Martin Friebe Note Added: 0120925
2020-02-07 19:54 Martin Friebe Status assigned => feedback
2020-02-07 19:54 Martin Friebe LazTarget => -
2020-02-07 19:54 Martin Friebe Note Added: 0120926
2020-02-08 15:55 BrunoK Note Added: 0120943
2020-02-08 15:55 BrunoK Status feedback => assigned
2020-02-08 18:28 Martin Friebe Note Added: 0120946
2020-02-08 19:41 Martin Friebe Note Added: 0120953
2020-02-08 19:44 Martin Friebe Status assigned => feedback
2020-02-09 14:12 BrunoK Note Added: 0120967
2020-02-09 14:12 BrunoK Status feedback => assigned
2020-02-09 16:42 Martin Friebe Status assigned => resolved
2020-02-09 16:42 Martin Friebe Resolution open => fixed
2020-02-09 16:42 Martin Friebe Fixed in Version => 2.2
2020-02-09 16:42 Martin Friebe Fixed in Revision => 62613
2020-02-09 16:42 Martin Friebe LazTarget - => 2.2
2020-02-09 16:42 Martin Friebe Widgetset Win32/Win64 => Win32/Win64
2020-02-09 16:42 Martin Friebe Note Added: 0120970
2020-02-10 12:53 BrunoK Status resolved => closed
2020-04-21 03:13 Martin Friebe Note Added: 0122311