View Issue Details
| ID | Project | Category | View Status | Date Submitted | Last Update |
|---|---|---|---|---|---|
| 0038084 | Lazarus | LCL | public | 2020-11-15 14:05 | 2021-04-10 09:43 |
| Reporter | jamie philbrook | Assigned To | wp | ||
| Priority | normal | Severity | minor | Reproducibility | always |
| Status | resolved | Resolution | fixed | ||
| Product Version | 2.1 (SVN) | ||||
| Summary | 0038084: TTreeView calls OnCollapsing and OnCollapsed during Control destruction and node clearing | ||||
| Description | The 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 Reproduce | Drop 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. | ||||
| Tags | No tags attached. | ||||
| Fixed in Revision | r64142, r64855 | ||||
| LazTarget | 2.2 | ||||
| Widgetset | Win32/Win64 | ||||
| Attached Files |
| ||||
|
|
P.S. Please add some nodes with Children on them too to make this happen. |
|
|
@Jamie: if you are not sure that what you report actually is a bug, then please aske on the ML before opening a ticket. |
|
|
It sounds like a bug, yes. @jamie, you have created a test application to reproduce it. Can you please upload it here. |
|
|
I think I fixed it. Please test with r64142. |
|
|
@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 |
|
|
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 ? |
|
|
btw, this issue exist on older versions too, so its been there for a while and maybe this would be one for a merge ? |
|
|
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. |
|
|
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. |
|
|
|
|
|
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.. |
|
|
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;
|
|
|
I am attaching also my test program. |
|
|
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. |
|
|
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. |
|
|
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. |
|
|
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? |
|
|
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,. |
|
|
Thanks for the confirmation. Applied. Please close if ok. |
| Date Modified | Username | Field | Change |
|---|---|---|---|
| 2020-11-15 14:05 | jamie philbrook | New Issue | |
| 2020-11-15 14:07 | jamie philbrook | Note Added: 0126958 | |
| 2020-11-15 14:22 | Bart Broersma | Note Added: 0126960 | |
| 2020-11-15 15:24 | Juha Manninen | Note Added: 0126963 | |
| 2020-11-15 16:54 | Juha Manninen | Assigned To | => Juha Manninen |
| 2020-11-15 16:54 | Juha Manninen | Status | new => assigned |
| 2020-11-15 16:57 | Juha Manninen | Note Added: 0126967 | |
| 2020-11-15 17:10 | jamie philbrook | Note Added: 0126968 | |
| 2020-11-15 17:17 | jamie philbrook | Note Added: 0126969 | |
| 2020-11-15 17:18 | jamie philbrook | Note Added: 0126970 | |
| 2020-11-15 17:51 | Juha Manninen | Status | assigned => resolved |
| 2020-11-15 17:51 | Juha Manninen | Resolution | open => fixed |
| 2020-11-15 17:51 | Juha Manninen | Fixed in Revision | => r64142 |
| 2020-11-15 17:51 | Juha Manninen | LazTarget | => - |
| 2020-11-15 17:51 | Juha Manninen | Widgetset | Win32/Win64 => Win32/Win64 |
| 2020-11-15 17:51 | Juha Manninen | Note Added: 0126971 | |
| 2021-03-21 18:12 | wp | Assigned To | Juha Manninen => wp |
| 2021-03-21 18:12 | wp | Status | resolved => assigned |
| 2021-03-21 18:12 | wp | Resolution | fixed => open |
| 2021-03-21 18:12 | wp | Note Added: 0129808 | |
| 2021-03-21 18:13 | wp | Note Added: 0129809 | |
| 2021-03-21 18:13 | wp | File Added: test_customtreeview_expanding_event_3.zip | |
| 2021-03-21 18:17 | wp | Note Edited: 0129808 | View Revisions |
| 2021-03-21 18:51 | jamie philbrook | Note Added: 0129810 | |
| 2021-03-21 21:25 | wp | Note Added: 0129811 | |
| 2021-03-21 21:25 | wp | File Added: treeview_38084.patch | |
| 2021-03-21 21:35 | wp | Note Added: 0129812 | |
| 2021-03-21 21:35 | wp | File Added: treeview_expanding_collapsing.zip | |
| 2021-03-21 22:28 | jamie philbrook | Note Added: 0129814 | |
| 2021-03-21 22:51 | wp | Note Added: 0129815 | |
| 2021-03-21 22:52 | wp | Note Edited: 0129815 | View Revisions |
| 2021-03-21 22:54 | jamie philbrook | Note Added: 0129816 | |
| 2021-03-21 23:15 | wp | Note Added: 0129817 | |
| 2021-03-21 23:37 | jamie philbrook | Note Added: 0129818 | |
| 2021-03-22 08:45 | wp | Status | assigned => resolved |
| 2021-03-22 08:45 | wp | Resolution | open => fixed |
| 2021-03-22 08:45 | wp | Fixed in Revision | r64142 => r64142, r64855 |
| 2021-03-22 08:45 | wp | LazTarget | - => 2.2 |
| 2021-03-22 08:45 | wp | Widgetset | Win32/Win64 => Win32/Win64 |
| 2021-03-22 08:45 | wp | Note Added: 0129822 | |
| 2021-04-10 09:43 | Juha Manninen | Relationship added | related to 0038735 |
