From ec83799b921f9e7218a4ac1bb1f84db45107fd72 Mon Sep 17 00:00:00 2001 From: TetzkatLipHoka <10427286+TetzkatLipHoka@users.noreply.github.com> Date: Mon, 17 Aug 2026 20:16:15 +0200 Subject: [PATCH] Fix #1197: SelectedCount is stale while selection change events run ToggleSelection() - the Shift+Arrow path - removes nodes via InternalRemoveFromSelection(). That routine only *marks* the entry in FSelection (it sets the low bit of the pointer, which is what PackArray later looks for) but fires DoRemoveFromSelection() and Change() straight away. FSelectionCount is not corrected until PackArray runs after the loop, so every handler invoked in between sees a count that is too high by the number of nodes already dropped. Iterating SelectedNodes gives the right answer at the same moment because vsSelected has been cleared, which is exactly the discrepancy reported. FSelectionCount cannot simply be decremented when marking: it is also the length PackArray scans, so lowering it early would leave marked entries in the array. Instead the pending marks are counted and subtracted in GetSelectedCount, and SelectedCount now reads that getter instead of the raw field. Internal callers keep using FSelectionCount directly, so the physical bookkeeping is unchanged. Event order is deliberately left alone - Change() is called from InternalRemoveFromSelection() on purpose, see the comment referring to #1047. The five line pack-and-resize block that appeared at seven call sites is now PackSelection(), which also resets the pending counter. Centralising it is what keeps that counter from drifting, since resetting it at seven places is easy to forget. It returns whether the array was shortened, which is what InvertSelection used its local flag for. Adds Tests/VTSelectedCountIssue1197Tests.pas: one test asserts the count seen during OnRemoveFromSelection, a second asserts the count after the operation so a future change cannot over-correct. Verified both ways - with the fix the suite is 136 passed / 2 failed, reverting only the getter change puts it back to 135 / 3. The two remaining failures are the pre-existing TestCopyHTML1 and TestCopyHTML2. Co-Authored-By: Claude Opus 5 --- Source/VirtualTrees.BaseTree.pas | 100 +++++++------- Tests/Tests.dpr | 1 + Tests/VTSelectedCountIssue1197Tests.pas | 171 ++++++++++++++++++++++++ 3 files changed, 217 insertions(+), 55 deletions(-) create mode 100644 Tests/VTSelectedCountIssue1197Tests.pas diff --git a/Source/VirtualTrees.BaseTree.pas b/Source/VirtualTrees.BaseTree.pas index c67c3185..188842a5 100644 --- a/Source/VirtualTrees.BaseTree.pas +++ b/Source/VirtualTrees.BaseTree.pas @@ -738,6 +738,10 @@ TBaseVirtualTree = class abstract(TVTBaseAncestor) FVclStyleEnabled: Boolean; FSelectionCount: Integer; + FSelectionMarkedCount: Integer; // Number of entries in FSelection that InternalRemoveFromSelection has + // marked for removal but that PackSelection has not yet dropped. Only + // SelectedCount subtracts it; FSelectionCount stays the physical count + // because PackArray needs it to know how far to scan. See issue #1197. procedure CMStyleChanged(var Message: TMessage); message CM_STYLECHANGED; procedure CMParentDoubleBufferedChange(var Message: TMessage); message CM_PARENTDOUBLEBUFFEREDCHANGED; @@ -792,6 +796,7 @@ TBaseVirtualTree = class abstract(TVTBaseAncestor) function IsLastVisibleChild(Parent, Node: PVirtualNode): Boolean; function MakeNewNode: PVirtualNode; function PackArray({*}const TheArray: TNodeArray; Count: Integer): Integer; + function PackSelection: Boolean; procedure FakeReadIdent(Reader: TReader); procedure SetAlignment(const Value: TAlignment); procedure SetAnimationDuration(const Value: Cardinal); @@ -1782,7 +1787,7 @@ TBaseVirtualTree = class abstract(TVTBaseAncestor) property SelectionLocked: Boolean read FSelectionLocked write FSelectionLocked; property TotalCount: Cardinal read GetTotalCount; property TreeStates: TVirtualTreeStates read FStates write FStates; - property SelectedCount: Integer read FSelectionCount; + property SelectedCount: Integer read GetSelectedCount; property TopNode: PVirtualNode read GetTopNode write SetTopNode; property VerticalAlignment[Node: PVirtualNode]: Byte read GetVerticalAlignment write SetVerticalAlignment; property VisibleCount: Cardinal read FVisibleCount; @@ -3640,7 +3645,9 @@ function TBaseVirtualTree.GetSelected(Node: PVirtualNode): Boolean; function TBaseVirtualTree.GetSelectedCount: Integer; begin - Exit(FSelectionCount); + // Entries already marked for removal must not be counted any more, otherwise this reports a stale value while + // OnRemoveFromSelection / OnChange run (issue #1197). FSelectionMarkedCount is 0 outside those windows. + Exit(FSelectionCount - FSelectionMarkedCount); end; //---------------------------------------------------------------------------------------------------------------------- @@ -3876,7 +3883,6 @@ function TBaseVirtualTree.HandleDrawSelection(X, Y: TDimension): Boolean; OldRect, NewRect: TRect; MainColumn: TColumnIndex; - MaxValue: Integer; // limits of a node and its text NodeLeft, @@ -3934,12 +3940,7 @@ function TBaseVirtualTree.HandleDrawSelection(X, Y: TDimension): Boolean; if Result then begin // Do some housekeeping if there was a change. - MaxValue := PackArray(FSelection, FSelectionCount); - if MaxValue > -1 then - begin - FSelectionCount := MaxValue; - SetLength(FSelection, FSelectionCount); - end; + PackSelection(); if FTempNodeCount > 0 then begin if tsClearOnNewSelection in fStates then @@ -4261,6 +4262,30 @@ function TBaseVirtualTree.PackArray({*}const TheArray: TNodeArray; Count: Intege //---------------------------------------------------------------------------------------------------------------------- +function TBaseVirtualTree.PackSelection: Boolean; + +// Drops the entries that InternalRemoveFromSelection has marked for removal and updates the selection count +// accordingly. Returns True if the array was actually shortened. +// This used to be an open coded five liner repeated at every call site; having it in one place is what keeps +// FSelectionMarkedCount from drifting, because resetting it is easy to forget (issue #1197). + +var + NewSize: Integer; + +begin + NewSize := PackArray(FSelection, FSelectionCount); + Result := NewSize > -1; + if Result then + begin + FSelectionCount := NewSize; + SetLength(FSelection, FSelectionCount); + end; + // No marked entries can be left over, regardless of whether anything was removed. + FSelectionMarkedCount := 0; +end; + +//---------------------------------------------------------------------------------------------------------------------- + procedure TBaseVirtualTree.PrepareBitmaps(NeedButtons, NeedLines: Boolean); // initializes the contents of the internal bitmaps @@ -13545,7 +13570,6 @@ procedure TBaseVirtualTree.InternalCacheNode(Node: PVirtualNode); procedure TBaseVirtualTree.InternalClearSelection(); var - Count: Integer; lNode: PVirtualNode; begin // It is possible that there are invalid node references in the selection array @@ -13553,12 +13577,7 @@ procedure TBaseVirtualTree.InternalClearSelection(); // Handle this potentially dangerous situation by packing the selection array explicitely. if IsUpdating then begin - Count := PackArray(FSelection, FSelectionCount); - if Count > -1 then - begin - FSelectionCount := Count; - SetLength(FSelection, FSelectionCount); - end; + PackSelection(); end; while FSelectionCount > 0 do @@ -13573,6 +13592,7 @@ procedure TBaseVirtualTree.InternalClearSelection(); end; ResetRangeAnchor; FSelection := nil; + FSelectionMarkedCount := 0; // the array is gone, so nothing can still be pending DoStateChange([], [tsClearPending]); end; @@ -13827,6 +13847,10 @@ procedure TBaseVirtualTree.InternalRemoveFromSelection(Node: PVirtualNode); if SyncCheckstateWithSelection[Node] then Node.CheckState := csUncheckedNormal; // Avoid using SetCheckState() as it handles toSyncCheckboxesWithSelection as well. System.Inc(PAnsiChar(FSelection[Index])); + // The entry is only marked here, PackSelection() drops it later. Until then FSelectionCount still counts it, + // so remember how many are pending - otherwise SelectedCount reports a stale, too high value in the events + // fired below, which is issue #1197. + System.Inc(FSelectionMarkedCount); DoRemoveFromSelection(Node); Change(Node); // Calling Change() here fixes issue #1047 end; @@ -15421,7 +15445,6 @@ procedure TBaseVirtualTree.ToggleSelection(StartNode, EndNode: PVirtualNode); var NodeFrom, NodeTo: PVirtualNode; - NewSize: Integer; Position: Integer; begin @@ -15477,12 +15500,7 @@ procedure TBaseVirtualTree.ToggleSelection(StartNode, EndNode: PVirtualNode); InternalRemoveFromSelection(NodeFrom); // Do some housekeeping if there was a change. - NewSize := PackArray(FSelection, FSelectionCount); - if NewSize > -1 then - begin - FSelectionCount := NewSize; - SetLength(FSelection, FSelectionCount); - end; + PackSelection(); // If the range went over the anchor then we need to reselect it. if not (vsSelected in FRangeAnchor.States) then InternalCacheNode(FRangeAnchor); @@ -15520,7 +15538,6 @@ procedure TBaseVirtualTree.UnselectNodes(StartNode, EndNode: PVirtualNode); var NodeFrom, NodeTo: PVirtualNode; - NewSize: Integer; begin if not FSelectionLocked then @@ -15557,12 +15574,7 @@ procedure TBaseVirtualTree.UnselectNodes(StartNode, EndNode: PVirtualNode); InternalRemoveFromSelection(NodeFrom); // Do some housekeeping. - NewSize := PackArray(FSelection, FSelectionCount); - if NewSize > -1 then - begin - FSelectionCount := NewSize; - SetLength(FSelection, FSelectionCount); - end; + PackSelection(); end; end; @@ -16963,7 +16975,6 @@ procedure TBaseVirtualTree.DeleteChildren(Node: PVirtualNode; ResetHasChildren: Mark: PVirtualNode; LastTop, LastLeft: TDimension; - NewSize: Integer; ParentVisible: Boolean; begin @@ -17024,12 +17035,7 @@ procedure TBaseVirtualTree.DeleteChildren(Node: PVirtualNode; ResetHasChildren: InvalidateCache; if FUpdateCount = 0 then begin - NewSize := PackArray(FSelection, FSelectionCount); - if NewSize > -1 then - begin - FSelectionCount := NewSize; - SetLength(FSelection, FSelectionCount); - end; + PackSelection(); ValidateCache; UpdateScrollBars(True); @@ -17263,9 +17269,6 @@ procedure TBaseVirtualTree.EndSynch; procedure TBaseVirtualTree.EndUpdate; -var - NewSize: Integer; - begin if FUpdateCount = 0 then exit; @@ -17281,12 +17284,7 @@ procedure TBaseVirtualTree.EndUpdate; Exclude(FStates, tsUpdateHiddenChildrenNeeded); end; - NewSize := PackArray(FSelection, FSelectionCount); - if NewSize > -1 then - begin - FSelectionCount := NewSize; - SetLength(FSelection, FSelectionCount); - end; + PackSelection(); InvalidateCache; ValidateCache; @@ -20550,7 +20548,6 @@ procedure TBaseVirtualTree.InvertSelection(VisibleOnly: Boolean); var Run: PVirtualNode; - NewSize: Integer; NextFunction: TGetNextNodeProc; TriggerChange: Boolean; @@ -20574,14 +20571,7 @@ procedure TBaseVirtualTree.InvertSelection(VisibleOnly: Boolean); // do some housekeeping // Need to trigger the OnChange event from here if nodes were only deleted but not added. - TriggerChange := False; - NewSize := PackArray(FSelection, FSelectionCount); - if NewSize > -1 then - begin - FSelectionCount := NewSize; - SetLength(FSelection, FSelectionCount); - TriggerChange := True; - end; + TriggerChange := PackSelection(); if FTempNodeCount > 0 then begin AddToSelection(FTempNodeCache, FTempNodeCount); diff --git a/Tests/Tests.dpr b/Tests/Tests.dpr index 46c91cd0..874c335d 100644 --- a/Tests/Tests.dpr +++ b/Tests/Tests.dpr @@ -17,6 +17,7 @@ uses VTOnEditCancelledTests in 'VTOnEditCancelledTests.pas', VTOnDrawTextTests in 'VTOnDrawTextTests.pas', VTCellSelectionTests in 'VTCellSelectionTests.pas', + VTSelectedCountIssue1197Tests in 'VTSelectedCountIssue1197Tests.pas', VirtualTrees.MouseUtils in 'VirtualTrees.MouseUtils.pas', VTCellSelectionTests.VisibilityForm in 'VTCellSelectionTests.VisibilityForm.pas' {VisibilityForm}, VTCellSelectionTests.VTSelectionTestForm in 'VTCellSelectionTests.VTSelectionTestForm.pas' {SelectionTestForm}; diff --git a/Tests/VTSelectedCountIssue1197Tests.pas b/Tests/VTSelectedCountIssue1197Tests.pas new file mode 100644 index 00000000..92ddd9e4 --- /dev/null +++ b/Tests/VTSelectedCountIssue1197Tests.pas @@ -0,0 +1,171 @@ +unit VTSelectedCountIssue1197Tests; + +// Regressionstest zu Issue #1197 "SelectedCount is not always correct". +// +// Befund: ToggleSelection() (der Shift+Pfeil-Pfad) entfernt Knoten per +// InternalRemoveFromSelection aus der Auswahl. Diese Routine MARKIERT den Eintrag im +// Auswahl-Array nur (sie setzt das unterste Bit des Zeigers, siehe PackArray), feuert +// aber sofort DoRemoveFromSelection und Change. FSelectionCount wird erst NACH der +// Schleife durch PackArray korrigiert. +// +// Folge: In OnRemoveFromSelection / OnChange / OnStateChange meldet SelectedCount noch +// den alten, zu hohen Wert, waehrend das Zaehlen der Knoten mit vsSelected bereits +// stimmt - genau das beschreibt der Reporter. + +interface + +uses + DUnitX.TestFramework, + Classes, + Vcl.Forms, + VirtualTrees, + VirtualTrees.Types, + VirtualTrees.BaseTree; + +type + // Cracker, um an das protected ToggleSelection heranzukommen (der Tastaturpfad ruft es). + TTestTree = class(TVirtualStringTree) + public + procedure PublicToggleSelection(StartNode, EndNode: PVirtualNode); + end; + + [TestFixture] + TVTSelectedCountIssue1197Tests = class + strict private + fForm: TForm; + fTree: TTestTree; + fCountInEvent: Integer; // SelectedCount, wie es das Event sieht + fActualInEvent: Integer; // tatsaechlich selektierte Knoten zum selben Zeitpunkt + fEventFired: Boolean; + function CountSelectedNodes: Integer; + procedure TreeRemoveFromSelection(Sender: TBaseVirtualTree; Node: PVirtualNode); + function NodeByIndex(Index: Integer): PVirtualNode; + public + [Setup] + procedure Setup; + [TearDown] + procedure TearDown; + + /// SelectedCount muss auch waehrend OnRemoveFromSelection zur tatsaechlichen + /// Anzahl selektierter Knoten passen. + [Test] + procedure SelectedCountIsCorrectDuringRemoveFromSelection; + + /// Nach Abschluss der Operation muss der Wert in jedem Fall stimmen + /// (das funktioniert schon vor dem Fix - Absicherung gegen Ueberkorrektur). + [Test] + procedure SelectedCountIsCorrectAfterToggleSelection; + end; + +implementation + +uses + SysUtils; + +{ TTestTree } + +procedure TTestTree.PublicToggleSelection(StartNode, EndNode: PVirtualNode); +begin + ToggleSelection(StartNode, EndNode); +end; + +{ TVTSelectedCountIssue1197Tests } + +procedure TVTSelectedCountIssue1197Tests.Setup; +begin + fForm := TForm.Create(nil); + fTree := TTestTree.Create(fForm); + fTree.Parent := fForm; + fTree.TreeOptions.SelectionOptions := fTree.TreeOptions.SelectionOptions + [toMultiSelect]; + fTree.NodeDataSize := 0; + fTree.RootNodeCount := 10; + fTree.ValidateNode(nil, True); + fEventFired := False; + fCountInEvent := -1; + fActualInEvent := -1; +end; + +procedure TVTSelectedCountIssue1197Tests.TearDown; +begin + FreeAndNil(fForm); +end; + +function TVTSelectedCountIssue1197Tests.NodeByIndex(Index: Integer): PVirtualNode; +var + I: Integer; +begin + Result := fTree.GetFirst; + for I := 1 to Index do + Result := fTree.GetNext(Result); +end; + +function TVTSelectedCountIssue1197Tests.CountSelectedNodes: Integer; +var + Node: PVirtualNode; +begin + Result := 0; + Node := fTree.GetFirst; + while Assigned(Node) do + begin + if vsSelected in Node.States then + Inc(Result); + Node := fTree.GetNext(Node); + end; +end; + +procedure TVTSelectedCountIssue1197Tests.TreeRemoveFromSelection(Sender: TBaseVirtualTree; + Node: PVirtualNode); +begin + // Nur den ersten Aufruf festhalten - dort ist die Abweichung am groessten. + if fEventFired then + Exit; + fEventFired := True; + fCountInEvent := fTree.SelectedCount; + fActualInEvent := CountSelectedNodes; +end; + +procedure TVTSelectedCountIssue1197Tests.SelectedCountIsCorrectDuringRemoveFromSelection; +var + First, Fifth: PVirtualNode; +begin + First := NodeByIndex(0); + Fifth := NodeByIndex(4); + + // Knoten 0..4 auswaehlen, Anker auf den ersten setzen (wie bei Shift+Pfeil runter). + fTree.FocusedNode := First; + fTree.Selected[First] := True; + fTree.SelectNodes(First, Fifth, False); + Assert.AreEqual(5, fTree.SelectedCount, 'Vorbedingung: 5 Knoten ausgewaehlt'); + + fTree.OnRemoveFromSelection := TreeRemoveFromSelection; + + // Auswahl verkleinern (Shift+Pfeil hoch): der Bereich 4..2 wird abgewaehlt. + fTree.PublicToggleSelection(Fifth, NodeByIndex(2)); + + Assert.IsTrue(fEventFired, 'OnRemoveFromSelection wurde nicht ausgeloest'); + Assert.AreEqual(fActualInEvent, fCountInEvent, + Format('SelectedCount meldet im Event %d, tatsaechlich selektiert sind %d', + [fCountInEvent, fActualInEvent])); +end; + +procedure TVTSelectedCountIssue1197Tests.SelectedCountIsCorrectAfterToggleSelection; +var + First, Fifth: PVirtualNode; +begin + First := NodeByIndex(0); + Fifth := NodeByIndex(4); + + fTree.FocusedNode := First; + fTree.Selected[First] := True; + fTree.SelectNodes(First, Fifth, False); + + fTree.PublicToggleSelection(Fifth, NodeByIndex(2)); + + Assert.AreEqual(CountSelectedNodes, fTree.SelectedCount, + 'SelectedCount nach Abschluss der Operation'); +end; + +initialization + TDUnitX.RegisterTestFixture(TVTSelectedCountIssue1197Tests); + +end.