View Issue Details

IDProjectCategoryView StatusLast Update
0032863LazarusLCLpublic2017-12-21 00:25
ReporterRik van KekemAssigned ToMichl 
PrioritynormalSeverityminorReproducibilityalways
Status closedResolutionfixed 
Product Version1.8Product Build 
Target Version1.8.2Fixed in Version1.9 (SVN) 
Summary0032863: TNotebook always fires OnBeforeShow for pages when it's destroyed
DescriptionThe TNotebook always fires the OnBeforeShow for it's pages when it's destroyed. This behavior was different in Laz 1.6.4.

Also see discussion at
http://forum.lazarus.freepascal.org/index.php/topic,39348.msg269804
Steps To ReproduceAttached example shows the firing of OnBeforePage for Page2 and Page1 when the program is closed. It's logical because Page 3 is destroyed first and Page2 becomes visible.

But this does not need to happen if TNoteBook itself is destroyed.
Additional InformationIn /lcl/include/page.inc there is a destructor TPage.Destroy which introduced this behavior.

Maybe the check for "(Parent <> nil) and (Parent is TNotebook)" could also include that the parent is not in csDestroying state. It it is, it could just remove itself from the FPageList without triggering the OnBeforeShow for the next page (like it was in previous versions).
TagsNo tags attached.
Fixed in Revision56811
LazTarget-
Widgetset
Attached Files
  • TNotebook.zip (2,739 bytes)
  • page.inc.patch (870 bytes)
    Index: lcl/include/page.inc
    ===================================================================
    --- lcl/include/page.inc	(revision 56808)
    +++ lcl/include/page.inc	(working copy)
    @@ -33,7 +33,8 @@
       end;
     
       //if old and new parent is a TNotebook then remove the page from the old one
    -  if (OldParent <> nil) and (OldParent is TNotebook) then
    +  if (OldParent <> nil) and (OldParent is TNotebook) and
    +    not (csDestroying in OldParent.ComponentState) then
       begin
         // remove from old pagelist
         ParentNotebook := TNotebook(OldParent);
    @@ -63,7 +64,8 @@
       DebugLn('[TPage.Destroy]');
       {$endif}
     
    -  if (Parent <> nil) and (Parent is TNotebook) then
    +  if (Parent <> nil) and (Parent is TNotebook) and
    +    not (csDestroying in Parent.ComponentState) then
       begin
         {$ifdef DEBUG_NEW_NOTEBOOK}
         DebugLn('[TPage.Destroy] FPages.Remove(Self)');
    
    page.inc.patch (870 bytes)

Relationships

related to 0030710 closedJuha Manninen TNoteBook.Pages property editor is not functional 

Activities

Rik van Kekem

2017-12-20 17:20

reporter  

TNotebook.zip (2,739 bytes)

Juha Manninen

2017-12-20 19:27

developer   ~0104892

Could you provide a patch please.

Rik van Kekem

2017-12-20 22:30

reporter  

page.inc.patch (870 bytes)
Index: lcl/include/page.inc
===================================================================
--- lcl/include/page.inc	(revision 56808)
+++ lcl/include/page.inc	(working copy)
@@ -33,7 +33,8 @@
   end;
 
   //if old and new parent is a TNotebook then remove the page from the old one
-  if (OldParent <> nil) and (OldParent is TNotebook) then
+  if (OldParent <> nil) and (OldParent is TNotebook) and
+    not (csDestroying in OldParent.ComponentState) then
   begin
     // remove from old pagelist
     ParentNotebook := TNotebook(OldParent);
@@ -63,7 +64,8 @@
   DebugLn('[TPage.Destroy]');
   {$endif}
 
-  if (Parent <> nil) and (Parent is TNotebook) then
+  if (Parent <> nil) and (Parent is TNotebook) and
+    not (csDestroying in Parent.ComponentState) then
   begin
     {$ifdef DEBUG_NEW_NOTEBOOK}
     DebugLn('[TPage.Destroy] FPages.Remove(Self)');
page.inc.patch (870 bytes)

Rik van Kekem

2017-12-20 22:30

reporter   ~0104896

Last edited: 2017-12-20 22:31

View 2 revisions

I was finally able to create a patch. First I didn't have the latest trunk and second I had a struggle with Microsoft Defender (slow compile) :(

Besides the FPages.Delete() in TPage.Destroy, the SetParent(nil) (which is done in the ancestor) also triggers a FPages.Delete(i) in TPage.SetParent().

Both need to be skipped if the parent is in destroying state.
Furthermore, there is no need to do a FPage.Delete() for the page, if the parent is in destroy state because the parent will clean up the FPage-StringList itself.

Patch attached.

Michl

2017-12-20 23:03

developer   ~0104899

Yes, I redesigned TNoteBook. This I've obviously missed.

Thank you for the patch! I applied and merge requested it for 1.8.2.

Please test and close if ok.

Rik van Kekem

2017-12-21 00:25

reporter   ~0104901

Tested and confirmed fixed in revision 56811.

Issue History

Date Modified Username Field Change
2017-12-20 17:18 Rik van Kekem New Issue
2017-12-20 17:20 Rik van Kekem File Added: TNotebook.zip
2017-12-20 19:27 Juha Manninen Note Added: 0104892
2017-12-20 22:18 Michl Assigned To => Michl
2017-12-20 22:18 Michl Status new => assigned
2017-12-20 22:30 Rik van Kekem File Added: page.inc.patch
2017-12-20 22:30 Rik van Kekem Note Added: 0104896
2017-12-20 22:31 Rik van Kekem Note Edited: 0104896 View Revisions
2017-12-20 23:03 Michl Relationship added related to 0030710
2017-12-20 23:03 Michl Fixed in Revision => 56811
2017-12-20 23:03 Michl LazTarget => -
2017-12-20 23:03 Michl Note Added: 0104899
2017-12-20 23:03 Michl Status assigned => resolved
2017-12-20 23:03 Michl Fixed in Version => 1.9 (SVN)
2017-12-20 23:03 Michl Resolution open => fixed
2017-12-20 23:03 Michl Target Version => 1.8.2
2017-12-21 00:25 Rik van Kekem Note Added: 0104901
2017-12-21 00:25 Rik van Kekem Status resolved => closed