View Issue Details

IDProjectCategoryView StatusLast Update
0038084LazarusLCLpublic2021-04-10 11:43
Reporterjamie philbrook Assigned Towp  
PrioritynormalSeverityminorReproducibilityalways
Status resolvedResolutionfixed 
Product Version2.1 (SVN) 
Summary0038084: TTreeView calls OnCollapsing and OnCollapsed during Control destruction and node clearing
DescriptionThe TreeView is calling these events during destruction or clearing regardless of the expanded state.

if a tree is not expanded the events get called anyways and even if they were expanded they should not be getting called.

Delphi does not do this, when destroying the control these events are not called.
Steps To ReproduceDrop a Treeview on the form. implement the OnCollapsed or OnCollapsing with some notification, for example a BEEP;

Free the control or just exit the app, you will get a beep and it does not matter if the tree is expanded or not..

During the events, you can test the NODE.Deleting and it is marked for deleting.

Should the events really get called at this time ?

I tested on Windows but I think it could also be happening on others too.

TagsNo tags attached.
Fixed in Revisionr64142, r64855
LazTarget2.2
WidgetsetWin32/Win64
Attached Files

Relationships

related to 0038735 assignedwp User code broken since rev 64855 

Activities

jamie philbrook

2020-11-15 15:07

reporter   ~0126958

P.S.

 Please add some nodes with Children on them too to make this happen.

Bart Broersma

2020-11-15 15:22

developer   ~0126960

@Jamie: if you are not sure that what you report actually is a bug, then please aske on the ML before opening a ticket.

Juha Manninen

2020-11-15 16:24

developer   ~0126963

It sounds like a bug, yes.
@jamie, you have created a test application to reproduce it. Can you please upload it here.

Juha Manninen

2020-11-15 17:57

developer   ~0126967

I think I fixed it. Please test with r64142.

jamie philbrook

2020-11-15 18:10

reporter   ~0126968

@bart:

 I thought I made it clear the first time?

 I said Delphi does not do this .

 Yes its a bug! period
@Juha I will check. thanks

jamie philbrook

2020-11-15 18:17

reporter   ~0126969

Yes, it is now working correctly.

I just ran a Clear test and a destroy test, those events are no longer called during that operation..

Good job..

I'll close it if you wish ?

jamie philbrook

2020-11-15 18:18

reporter   ~0126970

btw, this issue exist on older versions too, so its been there for a while and maybe this would be one for a merge ?

Juha Manninen

2020-11-15 18:51

developer   ~0126971

Great. Resolving.
You can add the revision to be merged if you want to the wiki page. I am not sure if anybody still merges them though. There will be no more 2.0.x dot releases. The new release Lazarus 2.2 will be forked at some point.

wp

2021-03-21 19:12

developer   ~0129808

Last edited: 2021-03-21 19:17

View 2 revisions

Reopening because the patch breaks standard behavior that OnExpanding event must be called when a node with HasChildren=true is expanded. Now this does not occur any more when BeginUpdate has been called when that node is expanded. This is a problem because in the OnExanding event often the child nodes of the expanding node are added. See forum https://forum.lazarus.freepascal.org/index.php/topic,53782.msg398517. Attaching the demo of user d7_to_laz which demonstrates the new issue.

wp

2021-03-21 19:13

developer   ~0129809

jamie philbrook

2021-03-21 19:51

reporter   ~0129810

So I just tested this in a laz 2.0.4 install which I happen to have on this PC and it works ok,
I have not made any changes to this install in that regard so at some time work was done on this TTreeView after the fact that caused this.

maybe a look back before I originally posted this..

wp

2021-03-21 22:25

developer   ~0129811

The new issue is caused by the tvsUpdating flag which prevents the OnExpand event from being fired when a node is expanded. I reverted the CanExpand and Expand methods to the state before Juha's changes, and the new issue is gone.

But I don't understand why the Begin/EndUpdate methods should have any effect also on the CanCollapse and Collapse methods which were also modified by Juha's code. I investigated why the tvsUpdating flag has been introduced and found that it had been there all the time since the first commit of TTreeView, and it had not been used for anything (except for setting and clearing)

Then I removed this flag and any code related to it. The new issue is gone, but jamie's issue is back. And this brings me to the initial question of this report: Is it harmful when during deletion a node the OnCollapse event fires? I do understand that OnCollapse/OnCollapsing should not fire during destruction because other controls (already destroyed) can be accessed here by user code; but this can be fixed by checking the csDestroying flag in ComponentState.

In the introduction there is a sentence "The TreeView is calling these events during destruction or clearing regardless of the expanded state." Well, maybe this is an issue... But I cannot reproduce it. Therefore I'd ask jamie to post a ready to use project (not just a description) to demonstrate the issue.

I am attaching a patch with the current state of my work. As I said, the expand/collapse events are firing as expected. But OnCollapsing and OnCollape are firing also when the tree dir cleared, but not when it is destroyed. Jamie, would this be acceptable?
treeview_38084.patch (2,978 bytes)   
Index: lcl/comctrls.pp
===================================================================
--- lcl/comctrls.pp	(revision 64848)
+++ lcl/comctrls.pp	(working copy)
@@ -3193,7 +3193,6 @@
     function GetCount: Integer;
     function GetOwner: TPersistent; override;
     procedure SetItem(Index: Integer; AValue: TTreeNode);
-    procedure SetUpdateState(Updating: Boolean);
   public
     constructor Create(AnOwner: TCustomTreeView);
     destructor Destroy; override;
@@ -3269,7 +3268,6 @@
     tvsIsEditing,
     tvsStateChanging,
     tvsManualNotify,
-    tvsUpdating,
     tvsPainting,
     tvoFocusedPainting,
     tvsDblClicked,
Index: lcl/include/treeview.inc
===================================================================
--- lcl/include/treeview.inc	(revision 64848)
+++ lcl/include/treeview.inc	(working copy)
@@ -2596,17 +2596,14 @@
 
 procedure TTreeNodes.BeginUpdate;
 begin
-  if FUpdateCount = 0 then SetUpdateState(True);
   Inc(FUpdateCount);
 end;
 
-procedure TTreeNodes.SetUpdateState(Updating: Boolean);
+procedure TTreeNodes.EndUpdate;
 begin
-  //SendMessage(Handle, WM_SETREDRAW, Ord(not Updating), 0);
-  if Updating then
-    Include(Owner.FStates,tvsUpdating)
-  else begin
-    Exclude(Owner.FStates,tvsUpdating);
+  Dec(FUpdateCount);
+  if FUpdateCount = 0 then
+  begin
     Include(Owner.FStates,tvsScrollbarChanged);
     Owner.UpdateScrollbars;
     Owner.Invalidate;
@@ -2613,12 +2610,6 @@
   end;
 end;
 
-procedure TTreeNodes.EndUpdate;
-begin
-  Dec(FUpdateCount);
-  if FUpdateCount = 0 then SetUpdateState(False);
-end;
-
 procedure TTreeNodes.FreeAllNodeData;
 var
   i: Integer;
@@ -4872,7 +4863,7 @@
   Node: TTreeNode;
   InsertMarkRect: TRect;
 begin
-  if [tvsUpdating,tvsPainting] * FStates <> [] then Exit;
+  if [tvsPainting] * FStates <> [] then Exit;
   Include(FStates, tvsPainting);
   try
     if Focused then
@@ -5549,7 +5540,7 @@
 procedure TCustomTreeView.Expand(Node: TTreeNode);
 begin
   UpdateScrollbars;
-  if Assigned(FOnExpanded) and not (tvsUpdating in FStates) then
+  if Assigned(FOnExpanded) then
     FOnExpanded(Self, Node);
 end;
 
@@ -5556,14 +5547,16 @@
 function TCustomTreeView.CanExpand(Node: TTreeNode): Boolean;
 begin
   Result := True;
-  if Assigned(FOnExpanding) and not (tvsUpdating in FStates) then
+  if Assigned(FOnExpanding)then
     FOnExpanding(Self, Node, Result);
 end;
 
 procedure TCustomTreeView.Collapse(Node: TTreeNode);
 begin
+  if csDestroying in ComponentState then
+    exit;
   UpdateScrollbars;
-  if Assigned(FOnCollapsed) and not (tvsUpdating in FStates) then
+  if Assigned(FOnCollapsed) then
     FOnCollapsed(Self, Node);
 end;
 
@@ -5570,7 +5563,9 @@
 function TCustomTreeView.CanCollapse(Node: TTreeNode): Boolean;
 begin
   Result := True;
-  if Assigned(FOnCollapsing) and not (tvsUpdating in FStates) then
+  if csDestroying in ComponentState then
+    exit;
+  if Assigned(FOnCollapsing) then
     FOnCollapsing(Self, Node, Result);
 end;
 
treeview_38084.patch (2,978 bytes)   

wp

2021-03-21 22:35

developer   ~0129812

I am attaching also my test program.

jamie philbrook

2021-03-21 23:28

reporter   ~0129814

The original problem is when it was getting destroyed, these events were getting called
as for clearing the list and watching the collapsing , that would be normal if the list is already expanded, but before it was doing this even when the list wasn't expanded.

if the collapsing events are not getting called when they are not expanded during a item delete or control destruction then I would say its ok.

The key here is, the events should not be getting called if the NODE isn't expanded already... otherwise if they are expanded then it would be normal since items are getting removed.

wp

2021-03-21 23:51

developer   ~0129815

Last edited: 2021-03-21 23:52

View 2 revisions

So the point is that, when the tree is cleared, OnCollapse events are fired for nodes which are not expanded?

My current version (when you appyl the provided patch to trunk) does not show this behaviour in the demo program of note 0038084:0129812:
* Start the demo, click "Populate - there is only the root node, still collapsed. Click "Clear", the root node goes away, not OnCollapse* event is logged in the memo.
* Click "Populate" again, expand the root node. Click "Clear" - there is one OnCollapse/OnCollapsing event as expected for the expanded root node, but none of the (collapsed) nodes below root fires an OnCollapse* event.

Please confirm, and I'll commit my version.

jamie philbrook

2021-03-21 23:54

reporter   ~0129816

Ok, I just installed that patch and its working perfectly fine?

I don't get it ? I remember the issue clearly and if these files are completed reverted then they should be actiing the same again but they are not...

 what I did is I put a simple Tree view on the form, started with a parent, then used a button to add childs to the parent node.. with that, I did not get any false expanding triggers and I didn't get any issues with collapsing events when destroying the control.

I don't know what to tell you, it's working ok here from what I see.

wp

2021-03-22 00:15

reporter   ~0129817

Sorry, I am confused: You last sentence says that my patch solves the issue. But why is there a question mark after the first sentence then?

Please confirm: Does the patched trunk still show the issue, or not?

jamie philbrook

2021-03-22 00:37

reporter   ~0129818

Because I am under the impression that you reverted it back as it was and I was expecting the same effects as originally reported.

Its appears to be working fine with your patch, I don't see any ill effects here with that.

Please apply it .. Thank you,.

wp

2021-03-22 09:45

reporter   ~0129822

Thanks for the confirmation. Applied. Please close if ok.

Issue History

Date Modified Username Field Change
2020-11-15 15:05 jamie philbrook New Issue
2020-11-15 15:07 jamie philbrook Note Added: 0126958
2020-11-15 15:22 Bart Broersma Note Added: 0126960
2020-11-15 16:24 Juha Manninen Note Added: 0126963
2020-11-15 17:54 Juha Manninen Assigned To => Juha Manninen
2020-11-15 17:54 Juha Manninen Status new => assigned
2020-11-15 17:57 Juha Manninen Note Added: 0126967
2020-11-15 18:10 jamie philbrook Note Added: 0126968
2020-11-15 18:17 jamie philbrook Note Added: 0126969
2020-11-15 18:18 jamie philbrook Note Added: 0126970
2020-11-15 18:51 Juha Manninen Status assigned => resolved
2020-11-15 18:51 Juha Manninen Resolution open => fixed
2020-11-15 18:51 Juha Manninen Fixed in Revision => r64142
2020-11-15 18:51 Juha Manninen LazTarget => -
2020-11-15 18:51 Juha Manninen Widgetset Win32/Win64 => Win32/Win64
2020-11-15 18:51 Juha Manninen Note Added: 0126971
2021-03-21 19:12 wp Assigned To Juha Manninen => wp
2021-03-21 19:12 wp Status resolved => assigned
2021-03-21 19:12 wp Resolution fixed => open
2021-03-21 19:12 wp Note Added: 0129808
2021-03-21 19:13 wp Note Added: 0129809
2021-03-21 19:13 wp File Added: test_customtreeview_expanding_event_3.zip
2021-03-21 19:17 wp Note Edited: 0129808 View Revisions
2021-03-21 19:51 jamie philbrook Note Added: 0129810
2021-03-21 22:25 wp Note Added: 0129811
2021-03-21 22:25 wp File Added: treeview_38084.patch
2021-03-21 22:35 wp Note Added: 0129812
2021-03-21 22:35 wp File Added: treeview_expanding_collapsing.zip
2021-03-21 23:28 jamie philbrook Note Added: 0129814
2021-03-21 23:51 wp Note Added: 0129815
2021-03-21 23:52 wp Note Edited: 0129815 View Revisions
2021-03-21 23:54 jamie philbrook Note Added: 0129816
2021-03-22 00:15 wp Note Added: 0129817
2021-03-22 00:37 jamie philbrook Note Added: 0129818
2021-03-22 09:45 wp Status assigned => resolved
2021-03-22 09:45 wp Resolution open => fixed
2021-03-22 09:45 wp Fixed in Revision r64142 => r64142, r64855
2021-03-22 09:45 wp LazTarget - => 2.2
2021-03-22 09:45 wp Widgetset Win32/Win64 => Win32/Win64
2021-03-22 09:45 wp Note Added: 0129822
2021-04-10 11:43 Juha Manninen Relationship added related to 0038735