4

I'm using Delphi XE2. I build a custom TComboBox so that I can easily add key/string pairs and handle the cleanup in the component's destructor.

All if not (csDesigning in ComponentState) code is omitted for brevity.

interface

type
  TKeyRec = class(TObject)
    Key: string;
    Value: string;
  end;

  TMyComboBox = class(TComboBox)
  public
    destructor Destroy; override;
    procedure AddItemPair(const Key, Value: string);
  end;

implementation

destructor TMyComboBox.Destroy;
var i: Integer;
begin
  for i := 0 to Self.Items.Count - 1 do
    Self.Items.Objects[i].Free;
  Self.Clear;
  inherited;
end;

procedure TMyComboBox.AddItemPair(const Key, Value: string);
var rec: TKeyRec;
begin
  rec := TKeyRec.Create;
  rec.Key := Key;
  rec.Value := Value;
  Self.Items.AddObject(Value, rec);
end;

When the application closes, the destructor is called, but the Items.Count property is inaccessible because the TComboBox must have a parent control to access this property. By the time the destructor is called, it no longer has a parent control.

I saw this problem once before and had to store the objects in a separate TList and free them separately. But that only worked because the order that I added them to the TList was always the same as the strings added to the combo box. When the user selected a string, I could use the combo box index to find the correlating object in the TList. If the combo box is sorted, then the indexes won't match, so I can't always use that solution.

Has anyone else seen this? How did you workaround the issue? It would be really nice to be able to free the objects in the component's destructor!

James L.
  • 9,384
  • 5
  • 38
  • 77
  • 3
    Yes. Same problem with [this one](http://stackoverflow.com/questions/15043297/where-to-free-dynamically-allocated-tframes-components-objects). ComboBox keeps it items with the native control, which is long gone when it's its parent/owner destroying itself. – Sertac Akyuz Aug 24 '13 at 00:51
  • 1
    Have you tried using BeforeDestruction (http://docwiki.embarcadero.com/Libraries/XE2/en/System.TObject.BeforeDestruction) instead? – Gerry Coll Aug 24 '13 at 04:19
  • 1
    @GerryColl - Good thought about using BeforeDestruction. However, the same problem persists... – James L. Aug 24 '13 at 05:23
  • Shouldn't you at least call the inherited destructor after cleaning up your stuff? I don't think this will solve your problem but afaik it's a must to do that. – alzaimar Aug 24 '13 at 05:34
  • @alzaimar - Yes, I am doing that. I just forgot to add it here. Thanks! – James L. Aug 24 '13 at 05:54
  • @SertacAkyuz - Thanks for the link. That certainly will work. – James L. Aug 24 '13 at 06:36

3 Answers3

2

You can override function GetItemsClass:

function GetItemsClass: TCustomComboBoxStringsClass; override;

It is called by Combo to create Items (by default it is TComboBoxStrings probably). Then you can create your own TComboBoxStrings descendant, for example TComboBoxStringObjects, where you can free object linked with item (when item deleted).

Anton K
  • 4,658
  • 2
  • 47
  • 60
Andrei Galatyn
  • 3,322
  • 2
  • 24
  • 38
  • Thanks Andrei. I think that might work since I could add a destructor to the TComboBoxStringObjects class. However, in trying to solve this I have read quite a bit and now tend to agree with David Heffernan and NGLN's comment/answer in the link from Sertac that suggests that storing objects in a GUI control is discouraged. I just finished writing the component that stores the objects in a `TList`. Everything is now properly freed when the component is destroyed. I'm preparing to post that as an answer alongside yours. Thanks again! – James L. Aug 24 '13 at 07:54
  • @James: James, i still believe that overriding of GetItemsClass is better solution. In your code you ARE storing objects within UI control anyway. Some disadvantages of your solution: you have to provide some additional methods in addition to standard, you will have problems if one day you use standard method (Items.AddObject for example) instead of additional one. I think it is better (and at same time easier) solution is to change behaviour of Items as i suggested. In this case you will have standard interface + some additional functionality (ability to free linked objects). – Andrei Galatyn Aug 24 '13 at 10:52
  • Andrei, I agree with your comments. I had already finished a solution that worked when you posted your answer and it was late (e.g., 2am). I'll give it a try today and let you know how it works. – James L. Aug 24 '13 at 16:29
  • Overriding the `GetItemsClass` still has the same problem. The destructor of the custom `TComboBoxStringObjects` class is still called too late and a 'no parent window' error is raised when accessing the `Count` property. I think the only other way to make it work is to use the raw Windows API calls to detect and free the objects, which is actually in the native code but is not used due to `{$IF DEFINED(CLR)}` statements. – James L. Aug 24 '13 at 21:18
  • @James. Yes, it seems that in time of destruction Win object is not accessible by messages anymore or at least not fully functional. You still can do it this way - if you override AddObject method also and will save copy of objects in your own list. But i just check the code, there is more serious problem. Combo uses class TComboBoxStrings by default, but this class defined in implementation section of StdCtrls instead of interface section and thus is not available for extension. Because of this it is better to keep it the way you already do or keep objects separately. – Andrei Galatyn Aug 24 '13 at 21:58
  • I agree. Thanks for your time Andrei. – James L. Aug 24 '13 at 22:32
1

After reading the link from Sertac (David Heffernan's comment and NGLN's answer), I believe a solution that stores the objects in a managed list and not in a GUI control is the best. To that end, I have create a combo box that descends from TCustomComboBox. This lets me promote all the properties except for Sorted to published. This keeps the internal FList in sync with the strings in the combo boxes Items property. I just make sure they are sorted the way I want before adding them...

The following shows what I did. I only included the essential code (less range checking) for brevity, but included some conditional logic that allows the combo box to be used without objects as well.

FList is properly destroyed in the destructor, freeing all objects without any run-time exceptions and the object list is managed within the component itself instead of having to manage it elsewhere -- making it very portable. It works when the control is added to a form at design-time, or when it is created at run-time. I hope this is useful to someone else!

interface

type
  TMyComboBox = class(TCustomComboBox)
  private
    FList: TList;
    FUsesObjects: Boolean;
    function GetKey: string;
  public
    constructor Create(AOwner: TComponent); override;
    destructor Destroy; override;
    procedure AddItemPair(const Key, Value: string);
    procedure ClearAllItems;
    procedure DeleteItem(const Index: Integer);
    property Key: string read GetKey;
  published
    // all published properties (except Sorted) from TComboBox
  end;

implementation

type
  TKeyRec = class(TObject)
    Key: string;
    Value: string;
  end;

function TMyComboBox.GetKey: string;
begin
  if not FUsesObjects then
    raise Exception.Create('Objects are not used.');

  Result := TKeyRec(FList.Items[ItemIndex]).Key;
end;

constructor TMyComboBox.Create(AOwner: TComponent);
begin
  inherited;

  if not (csDesigning in ComponentState) then
  begin
    FUsesObjects := False;
    FList := TList.Create;
  end;
end;

destructor TMyComboBox.Destroy;
begin
  if not (csDesigning in ComponentState) then
  begin
    ClearAllItems;
    FreeAndNil(FList);
  end;

  inherited;
end;

procedure TMyComboBox.AddItemPair(const Key, Value: string);
var rec: TKeyRec;
begin
  FUsesObjects := True;
  rec := TKeyRec.Create;
  rec.Key := Key;
  rec.Value := Value;
  FList.Add(rec);
  Items.Add(Value);
end;

procedure TMyComboBox.ClearAllItems;
var i: Integer;
begin
  if not (csDesigning in ComponentState) then
  begin
    if FUsesObjects then
    begin
      for i := 0 to FList.Count - 1 do
        TKeyRec(FList.Items[i]).Free;
      FList.Clear;
    end;
    if not (csDestroying in ComponentState) then
      Clear; // can't clear if the component is being destroyed or there is an exception, 'no parent window'
  end;
end;

procedure TMyComboBox.DeleteItem(const Index: Integer);
begin
  if FUsesObjects then
  begin
    TKeyRec(FList.Items[Index]).Free;
    FList.Delete(Index);
  end;
  Items.Delete(Index);
end;

end.
Community
  • 1
  • 1
James L.
  • 9,384
  • 5
  • 38
  • 77
  • 1
    This is no different from putting them in Objects. You've still got the GUI widget owning the objects. – David Heffernan Aug 24 '13 at 08:41
  • @DavidHeffernan - When I wrote the answer, I was thinking of the `TStrings` list that is used to paint the drop-down list. The objects aren't managed by it. I agree they are still owned by the GUI widget. I also agree that my design isn't any better than having the `TStrings` manage them, except that now they can be freed when the widget is destroyed. Whether I do it this way or as Andrei suggests, do you think it is a bad design to manage the objects in the widget? – James L. Aug 24 '13 at 16:40
  • I would not like to say. I'm not in posession of all the design criteria and constraints. – David Heffernan Aug 24 '13 at 16:45
0

There's a way that avoid the need of rewrite the component to use another list to save the objects. The solution is to use the WM_DESTROY message along with the ComponentState property. When the component is about to be destroyed, its state change to csDestroying, so the next time it receives a WM_DESTROY message it will be not part of the window recreation process. We use this method in our component library sucessfully.

TMyCombo = class(TCombobox)
...
  procedure   WMDestroy(var message: TMessage); message WM_DESTROY;
...

procedure TMyCombo.WMDestroy(var message: TMessage);
var
  i: integer;
begin
  if (csDestroying in ComponentState) then
    for i:=0 to Items.Count - 1 do
      Items.Objects[i].Free;
  inherited;
end;
NGLN
  • 43,011
  • 8
  • 105
  • 200
  • Yes, see also [this answer](http://stackoverflow.com/a/15052195/757830) where [Sertac Akyuz](http://stackoverflow.com/questions/18413263/tcombobox-control-has-no-parent-window-in-destructor/25813670#comment27049797_18413263) pointed to. – NGLN Oct 13 '14 at 20:40