Skip to content

Fix #1197: SelectedCount is stale while selection change events run #1381

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
TetzkatLipHoka wants to merge 1 commit into
base: master
Choose a base branch
from
+217 −55
Open

Fix #1197: SelectedCount is stale while selection change events run #1381

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
100 changes: 45 additions & 55 deletions Source/VirtualTrees.BaseTree.pas
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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;

//----------------------------------------------------------------------------------------------------------------------
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -13545,20 +13570,14 @@ 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
// if the tree update is locked and changes in the structure were made.
// 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
Expand All @@ -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;

Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -15421,7 +15445,6 @@ procedure TBaseVirtualTree.ToggleSelection(StartNode, EndNode: PVirtualNode);
var
NodeFrom,
NodeTo: PVirtualNode;
NewSize: Integer;
Position: Integer;

begin
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -15520,7 +15538,6 @@ procedure TBaseVirtualTree.UnselectNodes(StartNode, EndNode: PVirtualNode);
var
NodeFrom,
NodeTo: PVirtualNode;
NewSize: Integer;

begin
if not FSelectionLocked then
Expand Down Expand Up @@ -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;

Expand Down Expand Up @@ -16963,7 +16975,6 @@ procedure TBaseVirtualTree.DeleteChildren(Node: PVirtualNode; ResetHasChildren:
Mark: PVirtualNode;
LastTop,
LastLeft: TDimension;
NewSize: Integer;
ParentVisible: Boolean;

begin
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -17263,9 +17269,6 @@ procedure TBaseVirtualTree.EndSynch;

procedure TBaseVirtualTree.EndUpdate;

var
NewSize: Integer;

begin
if FUpdateCount = 0 then
exit;
Expand All @@ -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;
Expand Down Expand Up @@ -20550,7 +20548,6 @@ procedure TBaseVirtualTree.InvertSelection(VisibleOnly: Boolean);

var
Run: PVirtualNode;
NewSize: Integer;
NextFunction: TGetNextNodeProc;
TriggerChange: Boolean;

Expand All @@ -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);
Expand Down
1 change: 1 addition & 0 deletions Tests/Tests.dpr
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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};
Expand Down
Loading