Virtual-TreeView icon indicating copy to clipboard operation
Virtual-TreeView copied to clipboard

Make members of TVirtualNode private?

Open joachimmarder opened this issue 7 years ago • 6 comments

Frequently users of Virtual TreeView have problems because they use the members of TVirtualNode directly instead of the proper methods in TBaseVirtualTree. See e.g. #831

We should consider making them private, we could replace them with nice getter functions or properties, which are implmeneted similar to those in the TBaseVirtualTree. This has already been done for the Data field.

joachimmarder avatar Oct 13 '18 10:10 joachimmarder

+1

pyscripter avatar Oct 14 '18 00:10 pyscripter

Isn't this a duplicate of #823 ?

obones avatar Nov 02 '18 12:11 obones

Yes, its a duplicate. Forgot about that.

joachimmarder avatar Nov 02 '18 12:11 joachimmarder

If realy considered i will change it not to be private only as protected. You can not imagine what users can do with VT, limiting this to private will be not beneficial.

livius2 avatar Nov 06 '18 09:11 livius2

TVirtualNode is a record, so protected won't make any difference.

You can not imagine what users can do with VT

I can! I get the emails if that stuff doesn't work as expected. Or if they simply did not recognize that TVirtualNode.Parent is an internal piece of data that should better not be touched without using the appropriate setter.

joachimmarder avatar Nov 06 '18 19:11 joachimmarder

Or if they simply did not recognize that TVirtualNode.Parent is an internal piece of data that should better not be touched without using the appropriate setter.

I found myself using TVirtualNode.Parent pretty frequently (as a read-only property though). I also use Index and States which currently are irreplaceable by a tree method. And tree.NodeParent[Node] is inconvenient in some cases when you only have a node variable provided that there's no TVirtualNode.Owner property. So I vote for hiding important members but exposing getters for them and maybe replace tree.NodeParent[node] with node.GetParent or at least node.GetOwner.NodeParent[node]

Fr0sT-Brutal avatar Jul 16 '19 14:07 Fr0sT-Brutal

Now, with the units split up, we can't use friend access of the control classes on private members of TVirtualNode anymore. That means we will also need public setters, we just could not use them in the properties and leave them read-only. Any further thoughts on this from the watchers of this issue? Is there still enough benefit to justify this change?

joachimmarder avatar Mar 12 '23 09:03 joachimmarder

Well, to me, as a general rule, every field should be private and only accessed via properties. I have not checked if all such properties would need a "write" accessor though but if that's not the case, I would only add write wherever needed, leaving the other read only.

obones avatar Mar 13 '23 07:03 obones

I now did this for TVirtualNode.Parent.

joachimmarder avatar Apr 01 '23 12:04 joachimmarder

I will stop my work here. The members that typically cause support are now private with public readonly properties and a public setter function, which is now necessary after all the units have been split up.

joachimmarder avatar Sep 21 '23 14:09 joachimmarder