custom TTreeView

[WalkingAway 1]

Hello

Just have created simple TTreeView based component.

There are memory leaks. Reason, that New(Pointer) and then Dispose(Pointer) - second one not implemented yet.

 

To handle this, I got to Destroy.

But interesting that in Destroy method Items.Count is empty, somehow everithing is freed already...

Why?

Do I have to keep and track own ToDelete list for that?

 

pas file in the attachement

 

Thanks.

myComponent.txt

[Remy Lebeau 1264]

You are storing custom allocated data in the TTreeNode.Data property.  To free that memory, you can override the TreeView's virtual Delete() method, which is called for every TTreeNode object that gets destroyed at runtime, eg:

type
  TmyDBTreeView = class(TTreeView)
  protected
    procedure Delete(Node: TTreeNode); override;
  end;

procedure TmyDBTreeView.Delete(Node: TTreeNode);
begin
  inherited Delete(Node); // fires the OnDeletion event handler
  Dispose(PNodeRec(Node.Data));
  Node.Data := nil;
end;

However, a better way to handle this is to not use the TTreeNode.Data property at all.  Leave that untouched so users of your component can use it for their own application data.  Instead, you should create a custom descendant of TTreeNode to hold your component's extra data, and then you can override the TreeView's virtual CreateNode() method to return new instances of your custom Node class, eg:

type
  TmyDBTreeNode = class(TTreeNode)
  public
    IdNode: integer;
    IdParent : integer;
  end;

  TmyDBTreeView = class(TTreeView)
  protected
    function CreateNode: TTreeNode; override;
  end;
  
function TmyDBTreeView.CreateNode: TTreeNode;
begin
  Result := TmyDBTreeNode.Create(Items);
end;

Then, any place that you were accessing PNodeRec(Node.Data)^ before, you can instead access TmyDBTreeNode(Node), for example:

// if DataSource.DataSet.FieldByName(IDParentNode).AsInteger = PNodeRec(Node.Data)^.IdNode then
if DataSource.DataSet.FieldByName(IDParentNode).AsInteger = TmyDBTreeNode(Node).IdNode then

function TmyDBTreeView.GetNodeIdParent(Node: TTreeNode): integer;
begin
  //Result := PNodeRec(Node)^.IdParent;
  Result := TmyDBTreeNode(Node).IdParent;
end;

function TmyDBTreeView.GetNodeId(Node: TTreeNode): integer;
begin
  //Result := PNodeRec(Node)^.IdNode;
  Result := TmyDBTreeNode(Node).IdNode;
end;

And when adding new nodes to the TreeView, you don't need to use Add...Object() methods anymore, you can just use the normal Add...() methods instead, eg:

//New(NRec);
//NRec^.IdNode := DataSource.DataSet.FieldByName(IDNode).AsInteger;
//NRec^.IdParent := DataSource.DataSet.FieldByName(IDParentNode).AsInteger;
//Node := Items.AddObject(nil, DataSource.DataSet.FieldByName(DataField).AsString, NRec);
Node := Items.Add(nil, DataSource.DataSet.FieldByName(DataField).AsString);
TmyDBTreeNode(Node).IdNode := DataSource.DataSet.FieldByName(IDNode).AsInteger;
TmyDBTreeNode(Node).IdParent := DataSource.DataSet.FieldByName(IDParentNode).AsInteger;

// New(DNode);
// DNode^.IdNode := DataSource.DataSet.FieldByName(FIDNode).AsInteger;
// DNode^.IdParent := DataSource.DataSet.FieldByName(FIDParentNode).AsInteger;
// NewNode := Items.AddChildObject(Node, Field.AsString, DNode);
NewNode := Items.AddChild(Node, Field.AsString);
TmyDBTreeNode(NewNode).IdNode := DataSource.DataSet.FieldByName(FIDNode).AsInteger;
TmyDBTreeNode(NewNode).IdParent := DataSource.DataSet.FieldByName(FIDParentNode).AsInteger;

---

 

That being said, I notice a number of other issues with your code, that are unrelated to node management.

 

1. Why do you have a Sleep() loop inside of your TmyDBTreeView destructor?  Not only is a loop redundant when you could simply calculate the wanted duration in a single call, but calling Sleep() during destruction really doesn't belong there to begin with.

 

2. Any use of Items.(Begin|End)Update() should be protected with a try..finally block.

 

3. This code is wrong:

procedure TmyDBTreeView.CMGetDataLink(var Message: TMessage);
begin
  Message.Result := Integer(FDataLink);
end;

If you ever try to use your component in 64bit, this will fail as it will truncate the returned pointer.  The Message.Result is a NativeInt, not an Integer, so cast accordingly.

 

4. Inside of TmyDBTreeView.SetDataSource(), since you are calling FreeNotification() on a new DataSource, you should call RemoveFreeNotification() on any DataSource that was previously assigned.  Although, I think this is completely redundant since you are not actually storing a reference to the DataSource, you are just assigning it to your DataLink, so you could just remove use of FreeNotification() altogether and let the DataLink handle that for you.

 

Also, SetDataSource() is assigning the new DataSource to your DataLink only if csLoading is enabled, which means it is being assigned only during DFM streaming. But if a user of your component decides to change the DataSource at runtime after the DFM is loaded, you are not updating your DataLink.

 

5. in TmyDBTreeView.DeleteNodeFromTreeOnly(), you are looping incorrectly.  Since you are looping forwards, if a node is deleted then your loop counter will get out of sync and end up skipping the next node.  To account for that, you need to either:

 

a. loop backwards instead of forwards, eg:

procedure TmyDBTreeView.DeleteNodeFromTreeOnly(id_node: integer);
var
  i: integer;
begin
  for i := Items.Count - 1 downto 0 do begin
    //if PNodeRec(Items[i])^.IdNode = id_node then Items[i].Delete;
    if TmyDBTreeNode(Items[i]).IdNode = id_node then Items[i].Delete;
  end;
end;

b. change the loop so your 'i' variable is incremented only when a node is NOT deleted, eg:

procedure TmyDBTreeView.DeleteNodeFromTreeOnly(id_node: integer);
var
  i, cnt: integer;
begin
  i := 0;
  cnt := Items.Count;
  while i < cnt do begin
    //if PNodeRec(Items[i])^.IdNode = id_node then Items[i].Delete then
    if TmyDBTreeNode(Items[i]).IdNode = id_node then Items[i].Delete then
      Dec(cnt)
    else
      Inc(i);
  end;
end;

 

Edited July 12, 2023 by Remy Lebeau

[WalkingAway 1]

Thank you so much for your answers, I will check everything.

It opens my mind.

Of course, probably better idea to use something available, f.e. VirtualTreeView. But I tried just for educational purpose.

Thank you again. Some of mistakes were not visible for me at all...