Recent

Author Topic: Correct way to Excuse a TOpenDialog  (Read 538 times)

ahnz

  • Jr. Member
  • **
  • Posts: 57
Correct way to Excuse a TOpenDialog
« on: May 06, 2019, 11:21:47 pm »
Hi,

This is what I'm using

Quote
OS: OSX High Sierra 10.13.6
Lazarus: 2.1.0, r61162

Widgetset: Cocoa
Target: 64bit OSX

Compiler:
FPC 3.0.4 [2017/11/26] for x86_64

Debug:
LLDB  (with fpdebug)(Beta)
Path: /usr/bin/lldb
Version: lldb-1000.11.38.2

I'm trying to debug possible memory leaks in my project. Heres the bits of my project in question

Code: Pascal  [Select]
  1. type
  2.  
  3.   TImportImagesForm = class(TForm)
  4. ...
  5.   private
  6. ...
  7.     numberofFiles: integer;
  8.     SelectImportImagesDialog: TOpenDialog;
  9.     procedure SetImportButtonState;
  10.   public
  11.  
  12.   end;                                
  13.  
  14.  
  15. procedure TImportImagesForm.FormCreate(Sender: TObject);
  16. begin
  17.   SelectImportImagesDialog := TOpenDialog.Create(Self);
  18.   with SelectImportImagesDialog do
  19.   begin
  20.     Filter := 'Graphic (*.png;*.jpeg;*.jpg;*.jpe;*.gif)|*.png;*.jpeg;*.jpg;*.jpe;*.gif';
  21.     Options := [ofAllowMultiSelect, ofPathMustExist, ofFileMustExist, ofViewDetail, ofAutoPreview];
  22.     Title := 'Select Stamp Images';
  23.   end;
  24. end;
  25.  
  26. procedure TImportImagesForm.FormClose(Sender: TObject; var CloseAction: TCloseAction);
  27. begin
  28.   FreeAndNil(SelectImportImagesDialog);
  29. end;
  30.  
  31.  
  32. procedure TImportImagesForm.SelectImagesButtonClick(Sender: TObject);
  33. var
  34.   i: integer;
  35.   fileName: string;
  36. begin
  37.   if SelectImportImagesDialog.Execute then
  38.   begin
  39.     numberofFiles := SelectImportImagesDialog.Files.Count;
  40.     SetImportButtonState;
  41.   end;
  42. end;
  43.  


The resolved heap trace file shows (several of) ...

Quote
Call trace for block $0000000112B48140 size 64
  $00000001002DB353 file: cocoawsdialogs.pas : 687;
  $00000001002D9B3A file: cocoawsdialogs.pas : 231;
  $00000001002D948E file: cocoawsdialogs.pas : 314;
  $000000010004D05D file: commondialog.inc : 152;
  $000000010004E4FD file: filedialog.inc : 409;
  $000000010004CC11 file: commondialog.inc : 41;
  $000000010034F134 file: importimages.pas : 126;
  $0000000100212CF8 file: control.inc : 2912;

importimages.pas : 126 is this ...

Code: Pascal  [Select]
  1. if SelectImportImagesDialog.Execute then

Is this actually a memory leak, am I looking at this all wrong?

Thanks in advance for any help
OS: OSX High Sierra 10.13.6
Lazarus: 2.1.0, r61162

Widgetset: Cocoa
Target: 64bit OSX

Compiler:
FPC 3.0.4 [2017/11/26] for x86_64

Debug:
LLDB  (with fpdebug)(Beta)
Path: /usr/bin/lldb
Version: lldb-1000.11.38.2

furious programming

  • Sr. Member
  • ****
  • Posts: 328
  • I click a little.
Re: Correct way to Excuse a TOpenDialog
« Reply #1 on: May 06, 2019, 11:58:19 pm »
Why you create this dialog manually? Just put this dialog onto the form in the designer.
Lazarus 2.0.2 with FPC 3.0.4, Windows XP (all 32-bit)

VTwin

  • Hero Member
  • *****
  • Posts: 665
  • Former Turbo Pascal 3 user
Re: Correct way to Excuse a TOpenDialog
« Reply #2 on: May 07, 2019, 12:13:47 am »
Just a choice, but I create most dialogs as needed to ease memory load.

If you use TOpenDialog.Create(Self) then Free will be called on FormClose. FormClose.FreeAndNil is not necessary. Use TOpenDialog.Create(nil) if you will free it on your own.

Not sure if I said that clearly, but when you specify an owner, it calls Free when owner closes. I often use Application.
« Last Edit: May 07, 2019, 12:28:23 am by VTwin »
“Talk is cheap. Show me the code.” -Linus Torvalds

macOS 10.11.6: Lazarus 2.1.0 svn 61357M (64 bit Cocoa trunk)
Ubuntu 18.04.2: Lazarus 2.0.0 (64 bit on VBox)
Windows 7 Pro SP1: Lazarus 2.0.0 (64 bit on VBox)

furious programming

  • Sr. Member
  • ****
  • Posts: 328
  • I click a little.
Re: Correct way to Excuse a TOpenDialog
« Reply #3 on: May 07, 2019, 12:50:29 am »
Just a choice, but I create most dialogs as needed to ease memory load.

I understand, but in this case the dialog should be created, opened and released in the same routine, for example:

Code: Pascal  [Select]
  1. procedure TForm1.Button1Click(Sender: TObject);
  2. var
  3.   OpenDialog: TOpenDialog;
  4. begin
  5.   OpenDialog := TOpenDialog.Create(Self);
  6.   try
  7.     // setting dialog options
  8.  
  9.     if OpenDialog.Execute() then
  10.     begin
  11.       // operations with dialog data
  12.     end;
  13.   finally
  14.     OpenDialog.Free();
  15.   end;
  16. end;

However, the OP's code has such a construction:

Code: Pascal  [Select]
  1. procedure TForm1.FormCreate(Sender: TObject);
  2. begin
  3.   OpenDialog := TOpenDialog.Create(Self);
  4. end;
  5.  
  6. procedure TForm1.FormDestroy(Sender: TObject);
  7. begin
  8.   OpenDialog.Free();
  9. end;
  10.  
  11. procedure TForm1.Button1Click(Sender: TObject);
  12. begin
  13.   if OpenDialog.Execute() then
  14.   begin
  15.     // operations with dialog data
  16.   end;
  17. end;

which does not make sense, because this dialog still exists throughout the lifetime of the form and always has the same settings (title, filters and options). The same will be done by putting the component on the form — and writing the code is not needed. Just KISS.
« Last Edit: May 07, 2019, 12:53:32 am by furious programming »
Lazarus 2.0.2 with FPC 3.0.4, Windows XP (all 32-bit)

lucamar

  • Hero Member
  • *****
  • Posts: 1556
Re: Correct way to Excuse a TOpenDialog
« Reply #4 on: May 07, 2019, 12:55:16 am »
Is this actually a memory leak, am I looking at this all wrong?

There should be no memory leak, although the call to FreeAndNil is superfluous, as VTwin said.

I woud only do a little change (a matter of personal preference): I wouldn't create the dialog unitl I needed. That is, I would move the dialog-creation code from FormCreate() fo SelectImagesButtonClick, in add a guard against multi-creation:

Code: Pascal  [Select]
  1. procedure TImportImagesForm.SelectImagesButtonClick(Sender: TObject);
  2. var
  3.   i: integer;
  4.   fileName: string;
  5. begin
  6.   if not Assigned(SelectImportImagesDialog) the begin
  7.     SelectImportImagesDialog := TOpenDalog.Create(Self);
  8.     {... rest of dialog initialization ...}
  9.   end;
  10.   if SelectImportImagesDialog.Execute then
  11.   begin
  12.     numberofFiles := SelectImportImagesDialog.Files.Count;
  13.     SetImportButtonState;
  14.   end;
  15. end;


I understand, but in this case the dialog should be created, opened and released in the same routine, for example:

That's what I do usually (normally using a function to create and setup the dialog, if needed).

But sometimes you need the dialog to keep its current properties as they are (size/position, InitialDir, FileName, etc). In those cases it's best to use a global dialog object (e.g. as a field of the form) created on demand and not freed until program close-down.
« Last Edit: May 07, 2019, 01:04:03 am by lucamar »
Turbo Pascal 3 CP/M - Amstrad PCW 8256 (512 KB !!!) :P
Lazarus 1.8.4 & 2.0.2 w/FPC 3.0.4 on:
(K|L)Ubuntu 12..16, Windows XP SP3, various DOSes.

furious programming

  • Sr. Member
  • ****
  • Posts: 328
  • I click a little.
Re: Correct way to Excuse a TOpenDialog
« Reply #5 on: May 07, 2019, 12:59:09 am »
Still a lot of people does not know that the forgotten pointers/references does not need to be niled. Therefore, the FreeAndNil procedure is still used without any apparent need.
Lazarus 2.0.2 with FPC 3.0.4, Windows XP (all 32-bit)

VTwin

  • Hero Member
  • *****
  • Posts: 665
  • Former Turbo Pascal 3 user
Re: Correct way to Excuse a TOpenDialog
« Reply #6 on: May 07, 2019, 01:02:55 am »
@lucamar

That is pretty much what I do. Set it to nil, then open it as required. I generally set Application as the owner, and let the application free it when closed.

For a dialog I typically have a function such as RunMyDialog that takes care of the details.
« Last Edit: May 07, 2019, 01:17:06 am by VTwin »
“Talk is cheap. Show me the code.” -Linus Torvalds

macOS 10.11.6: Lazarus 2.1.0 svn 61357M (64 bit Cocoa trunk)
Ubuntu 18.04.2: Lazarus 2.0.0 (64 bit on VBox)
Windows 7 Pro SP1: Lazarus 2.0.0 (64 bit on VBox)

lucamar

  • Hero Member
  • *****
  • Posts: 1556
Re: Correct way to Excuse a TOpenDialog
« Reply #7 on: May 07, 2019, 01:13:18 am »
@lucamar

That is pretty much what I do. Set it to nil, then open it as required. I generally set Application as the owner, and let the application free it when closed.

Most of the time a dialog is invoked only from in a single form so there is no need to make the Application its owner: making the form the owner is enough. That has the advantage that when you really need the application to be the owner, seeing later on that you made it so makes you ask yourself why. Kind of a debugging of the mind, as it were :D

Anyway, when one (or more! they come in pairs... :)) dialog should really be global to the whole application it's more programmer-friendly to declare it in its own unit, along with the rest of really global components/controls. Then the application is the only "owner" at hand, so there can be no mistakes.
Turbo Pascal 3 CP/M - Amstrad PCW 8256 (512 KB !!!) :P
Lazarus 1.8.4 & 2.0.2 w/FPC 3.0.4 on:
(K|L)Ubuntu 12..16, Windows XP SP3, various DOSes.

VTwin

  • Hero Member
  • *****
  • Posts: 665
  • Former Turbo Pascal 3 user
Re: Correct way to Excuse a TOpenDialog
« Reply #8 on: May 07, 2019, 01:38:26 am »
I get your drift, but I'd have to examine my code further. My dialogs are not necessarily associated with a single Form, and have their own unit, so I'm not sure that would save memory. My compromise is to allocate them as needed and free them when the Application closes. Further optimization may not be useful, but I'm open to suggestions.

I guess that is your "global" suggestion?
« Last Edit: May 07, 2019, 01:57:55 am by VTwin »
“Talk is cheap. Show me the code.” -Linus Torvalds

macOS 10.11.6: Lazarus 2.1.0 svn 61357M (64 bit Cocoa trunk)
Ubuntu 18.04.2: Lazarus 2.0.0 (64 bit on VBox)
Windows 7 Pro SP1: Lazarus 2.0.0 (64 bit on VBox)

lucamar

  • Hero Member
  • *****
  • Posts: 1556
Re: Correct way to Excuse a TOpenDialog
« Reply #9 on: May 07, 2019, 02:11:33 am »
I get your drift, but I'd have to examine my code further. My dialogs are not necessarily associated with a single Form, and have their own unit, so I'm not sure that would save memory. My compromise is to allocate them as needed and free them when the Application closes. Further optimization may not be useful, but I'm open to suggestions.

I guess that is your "global" suggestion?

Yes, that's what I meant: if the dialog is really global (as in application-global) and can be used from more than one form, that's how I do it too: in their own unit, created when needed and make the Application the owner. As it really is, logically speaking :)

A further twist is to just expose, from that unit's interface, procs/functions to return the dialogs' result and keep all the dialog logic in the implementation. For example, expose a function GetFilename: String; whose implementation takes care of creating a/the dialog, executing it and returning the selected filename.
Turbo Pascal 3 CP/M - Amstrad PCW 8256 (512 KB !!!) :P
Lazarus 1.8.4 & 2.0.2 w/FPC 3.0.4 on:
(K|L)Ubuntu 12..16, Windows XP SP3, various DOSes.

ahnz

  • Jr. Member
  • **
  • Posts: 57
Re: Correct way to Excuse a TOpenDialog
« Reply #10 on: May 07, 2019, 02:47:51 am »
Thanks for all the feedback guys! - Very much appreciated.

The reason I'm creating this on the fly rather than adding the dialog to the form in designer is, thats where I started and I had the same "problem" so I changed to creating it on the fly to see if it made any difference, which it hasn't.

Does any one have any idea why the resolved heap-trace is referring to the line

Code: Pascal  [Select]
  1. if SelectImportImagesDialog.Execute then

Cheers
OS: OSX High Sierra 10.13.6
Lazarus: 2.1.0, r61162

Widgetset: Cocoa
Target: 64bit OSX

Compiler:
FPC 3.0.4 [2017/11/26] for x86_64

Debug:
LLDB  (with fpdebug)(Beta)
Path: /usr/bin/lldb
Version: lldb-1000.11.38.2

ASerge

  • Hero Member
  • *****
  • Posts: 1257
Re: Correct way to Excuse a TOpenDialog
« Reply #11 on: May 07, 2019, 04:20:23 am »
Still a lot of people does not know that the forgotten pointers/references does not need to be niled. Therefore, the FreeAndNil procedure is still used without any apparent need.
I think it came from here: https://eurekalog.blogspot.com/2009/04/why-should-you-always-use-freeandnil_28.html

lucamar

  • Hero Member
  • *****
  • Posts: 1556
Re: Correct way to Excuse a TOpenDialog
« Reply #12 on: May 07, 2019, 04:31:46 am »
Does any one have any idea why the resolved heap-trace is referring to the line
Code: Pascal  [Select]
  1. if SelectImportImagesDialog.Execute then

I'm not very sure about this but, Isn't the trace just telling you where the block is being used? If so, it would look awfully like a call stack: Execute diving deeper into the various "dialogs" units.
« Last Edit: May 07, 2019, 04:34:36 am by lucamar »
Turbo Pascal 3 CP/M - Amstrad PCW 8256 (512 KB !!!) :P
Lazarus 1.8.4 & 2.0.2 w/FPC 3.0.4 on:
(K|L)Ubuntu 12..16, Windows XP SP3, various DOSes.

VTwin

  • Hero Member
  • *****
  • Posts: 665
  • Former Turbo Pascal 3 user
Re: Correct way to Excuse a TOpenDialog
« Reply #13 on: May 07, 2019, 01:46:11 pm »
A further twist is to just expose, from that unit's interface, procs/functions to return the dialogs' result and keep all the dialog logic in the implementation. For example, expose a function GetFilename: String; whose implementation takes care of creating a/the dialog, executing it and returning the selected filename.

Yes, that is how I do it, a "RunMyFoobarDlg" function that may return a boolean or some data. I move the "MyFoobarDlg" pointer that Lazarus creates to the implementation to hide it, and set it to nil. The function creates the dialog if nil, and runs it.

Some are very complex and get and set a global record kept in a preferences dictionary for retrieval by other forms. In some cases the dialog will send global messages that other forms can respond to, for example a "Preview" button may update some graphics on another form.
“Talk is cheap. Show me the code.” -Linus Torvalds

macOS 10.11.6: Lazarus 2.1.0 svn 61357M (64 bit Cocoa trunk)
Ubuntu 18.04.2: Lazarus 2.0.0 (64 bit on VBox)
Windows 7 Pro SP1: Lazarus 2.0.0 (64 bit on VBox)

ahnz

  • Jr. Member
  • **
  • Posts: 57
Re: Correct way to Excuse a TOpenDialog
« Reply #14 on: May 08, 2019, 11:22:04 pm »
I have since done the following simple test...

Code: Pascal  [Select]
  1. program project1;
  2.  
  3. {$mode objfpc}{$H+}
  4.  
  5. uses
  6.   {$IFDEF UNIX}{$IFDEF UseCThreads}
  7.   cthreads,
  8.   {$ENDIF}{$ENDIF}
  9.   Interfaces, // this includes the LCL widgetset
  10.   Forms, Unit1, Unit2
  11.   { you can add units after this };
  12.  
  13. {$R *.res}
  14.  
  15. begin
  16.   SetHeapTraceOutput('/Users/andy/Desktop/memleak.trc');
  17.   RequireDerivedFormResource:=True;
  18.   Application.Scaled:=True;
  19.   Application.Initialize;
  20.   Application.CreateForm(TForm1, Form1);
  21.   Application.CreateForm(TForm2, Form2);
  22.   Application.Run;
  23. end.
  24.      
  25.  
  26. unit Unit1;
  27.  
  28. {$mode objfpc}{$H+}
  29.  
  30. interface
  31.  
  32. uses
  33.   Classes, SysUtils, Forms, Controls, Graphics, Dialogs, StdCtrls, OSUtils, Unit2;
  34.  
  35. type
  36.  
  37.   { TForm1 }
  38.  
  39.   TForm1 = class(TForm)
  40.     Button1: TButton;
  41.     procedure Button1Click(Sender: TObject);
  42.   private
  43.  
  44.   public
  45.  
  46.   end;
  47.  
  48. var
  49.   Form1: TForm1;
  50.  
  51. implementation
  52.  
  53. {$R *.lfm}
  54.  
  55. { TForm1 }
  56.  
  57. procedure TForm1.Button1Click(Sender: TObject);
  58. begin
  59.   if Form2.ShowModal = mrOk then
  60.   Close;
  61. end;
  62.  
  63. end.
  64.  
  65.  
  66. unit Unit2;
  67.  
  68. {$mode objfpc}{$H+}
  69.  
  70. interface
  71.  
  72. uses
  73.   Classes, SysUtils, Forms, Controls, Graphics, Dialogs, StdCtrls;
  74.  
  75. type
  76.  
  77.   { TForm2 }
  78.  
  79.   TForm2 = class(TForm)
  80.     Button1: TButton;
  81.     Button2: TButton;
  82.     procedure Button1Click(Sender: TObject);
  83.     procedure Button2Click(Sender: TObject);
  84.   private
  85.  
  86.   public
  87.  
  88.   end;
  89.  
  90. var
  91.   Form2: TForm2;
  92.  
  93. implementation
  94.  
  95. {$R *.lfm}
  96.  
  97. { TForm2 }
  98.  
  99. procedure TForm2.Button1Click(Sender: TObject);
  100. begin
  101.   ModalResult := mrOk;
  102. end;
  103.  
  104. procedure TForm2.Button2Click(Sender: TObject);
  105. begin
  106.   ModalResult := mrCancel;
  107. end;
  108.  
  109. end.

... and heaptrc shows (suggests) a memory leak at "if Form2.ShowModal = mrOk then" ... so I'm reckoning problem lies in the widgetset 

Cheers
OS: OSX High Sierra 10.13.6
Lazarus: 2.1.0, r61162

Widgetset: Cocoa
Target: 64bit OSX

Compiler:
FPC 3.0.4 [2017/11/26] for x86_64

Debug:
LLDB  (with fpdebug)(Beta)
Path: /usr/bin/lldb
Version: lldb-1000.11.38.2