TBaseVirtualTree.GetChildCount CRASH

[JJD2K]

Latest changes cause crash in:
https://github.com/TurboPack/MustangpeakVirtualshellTools/blob/master/Delphi/VirtualShellToolsDD.dpk

Old code works perfectly in latest sources:
function TBaseVirtualTree.GetChildCount(Node: PVirtualNode): Cardinal;

begin
if (Node = nil) or (Node = FRoot) then
Result := FRoot.ChildCount
else
Result := Node.ChildCount;
end;

[JJD2K]

I do not know what the new code does but somehow it creates an endless loop causing Stack overflow - most likely sending events that should not be sent.

?????????????
if (Node = nil) or (Node = FRoot) then
Exit(FRoot.ChildCount);
if not GetChildrenInitialized(Node) then
InitChildren(Node);
Exit(Node.ChildCount);

[JJD2K]

Does reverting to old code have any consequences?

[pyscripter]

The fix to #871 has undesired consequences.

In your code you can use Node.ChildCount instead of Tree.ChildCount[Node] to avoid the unwanted initialization of the child nodes.

See pyscripter/MustangpeakVirtualShellTools@3725172 for the fix.

[joachimmarder]

I think the code change was OK and the code in the derived class should be changed. Like TBaseVirtualTree.ToggleNode() it should work with vsInitialized and TVirtualNode.ChildCount or the code should be moved to InitChildren().

[JJD2K]

I do not know if it is OK, but

if ChildCount[Node] = 0 then

if Node.ChildCount = 0 then

should not produce different behavior.

If ChildCount[Node] should not be accessed - it should be made private. Such unpredictable beahaviour is not good. You will end up in a position where this component lives its own life and you can not track what is happening.

[joachimmarder]

If ChildCount[Node] should not be accessed - it should be made private.

Fully agreed, see #832.

You will end up in a position where this component lives its own life

Well, that's what it did when I started in this project. But it still has a lot of techncial debts that often cannot be fixed without breaking chnages like this one.

[JJD2K]

Breaking changes should be applied only if it is absolutely necessary and should be applied in a way that it is obvious for the existing code what should be changed - not to lead to such recursions.
In this case the above member had to be made private and in the code where it is locates had to write:
Please use Node.ChildCount instead

I am saying this with all my best feelings, but I see such breaking changes here quite often - actually every time I update.

[joachimmarder]

You suggested a breaking chnage yourself:

... it should be made private.

I often see the problem here that the bad breaking chnages are those suggested by the other users only.

[JJD2K]

It is better to have an error in compile time and be able to fix it rather than run time.
So for the breaking change above it would be better to make the property private - then the ones using it would know what should be changed.
I hope you understand what I mean.

And I did not propose a breaking change - I proposed better implementation of your breaking change ;)

[joachimmarder]

I received a lot of complaints when doing breaking changes in the past. Please keep in mind that >99.9% of all uses of the ChildCount[] property are still valid after the change. Why break 100% of them? I don't think you make a lot of fans that way.