Multiline broken (since update from 5.3.0 to 6.4.1)

[j214752]

Hi,

we have an App that uses Multiline support in TreeView. Since we updated from 5.3.0 to 6.4.1 our TreeView has only Single-Line Nodeheight instead of the MultiLine-Height. As if every Node had only 1 Text-Line.

We debuged a little and found out that the old TreeView 5.3.0 does not fire the OnMeasureItem while in BeginUpdate; EndUpdate; The 6.4.1 on the other hand does.

we do the following when building our Tree:

VirtualStringTree1.BeginUpdate;
try
  VirtualStringTree1.Clear;

  for i := 0 to Entries do
  begin
    Node := VirtualStringTree1.AddChild(nil); // <-- V6.4.1 measures NodeHeights here
    VirtualStringTree1.MultiLine[Node] := True;
    Data := VirtualStringTree1.GetNodeData(Node);
    Data.S := GenerateString;
  end;
finally
  VirtualStringTree1.EndUpdate; // <-- V5.3.0 measures NodeHeights here
end;

The OnMeasureItem is called from SetChildCount. In the V5.3.0 you will find the following comment:
// The actual node height will later be computed once it is clear
// whether this node has a variable node height or not.

I think there is some problem introduced in some Version after 5.3.0. Specially while in BeginUpdate; EndUpdate; It is more efficient when only calculating Stuff when we are no more in an Update-Mode.

I Attach a Simple Demo.
V5.3.0:

V6.4.1:

Demo:
VirtualTreeViewMultiLine.zip

[sanjayssk]

Joachim,

I investigated this and found that it was changed in 5.5 in issue #557, mentioned as #447 in CHANGES. file. It's quite a length discussion in there with original issue for which this was done not found at that time. I you can go through that and recall why that change was made, it will help. I think it was better in earlier code not to measure height in setchildcount and postpone it till the Paint as the earlier code was doing.

[sanjayssk]

Afterthought: One solution is to reset the vsHeightMeasured state of each node on EndUpdate. This should work for old applications relying on a measure of node height at first paint time. At the same time, any new applications will not break. However, we also need to test with a large number of nodes to see if BeginUpdate/EndUpdate is slowed down by that code introduced in #557.

[joachimmarder]

I you can go through that and recall why that change was made, it will help

Sorry, but I don't recall the details. I need to got through #557 and pull request #559 when I find the time.

[sanjayssk]

I did some experiments.
To make this test program work with the latest version, I made the following change:
Moved the code after AddChild, that initializes the node, to OnInitNode event. Once this is done, the multi line starts working.

But there is a big problem. The design change that was made in VTV makes it too slow for the same code.
To test the speed, I did this:

1. Changed the test program to add 10,000 nodes.
2. Compiled the test program as given with the old version 5.3 VTV. Then ran it. It took 31 msecs to add 10000 nodes.
3. Compiled the test program with my changes above for OnInitNode to make it work with the latest VTV source. Ran the test again. It takes almost 2 secs to finish. It's much slower now. Imagine how slow it will be for 100,000 nodes or more. This is going to affect existing applications. The reason is clearly a call to measure item at adding time. The earlier code was postponing it to later.

I think we need to revisit that old issue and see why the change was made and then see if the filtering problem can be fixed in some other way.

[joachimmarder]

I think the line

 Data.S := GenerateString;

Should be part of the OnInitNode event handler. Whenever you change the node's string after the node has been initialzed, you need to clear the flag vsHeightMeasured from TVirtualNode.States.

[joachimmarder]

Compiled the test program with my changes above for OnInitNode
to make it work with the latest VTV source. Ran the test again.
It takes almost 2 secs to finish. It's much slower now.

The old version however did not calculate the scrollbars correctly, because it calculated the actual node height on a toVariableNodeHeight scenario only for the visible nodes. This was what #557 targeted.

[sanjayssk]

I understand. But I think any height calculation should be postponed to after EndUpdate in the traditional way if possible. I'm coming in after a long gap on this. I will reinspect the code to see if that is possible.

…

On Tue, Feb 28, 2017 at 2:52 PM, Joachim Marder ***@***.***> wrote: Compiled the test program with my changes above for OnInitNode to make it work with the latest VTV source. Ran the test again. It takes almost 2 secs to finish. It's much slower now. The old version however did not calculate the scrollbars correctly, because it calculated the actual node height on a toVariableNodeHeight scenario only for the visible nodes. This was what #557 <#557> targeted. — You are receiving this because you were assigned. Reply to this email directly, view it on GitHub <#675 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ATX-xg_nwB5vMtmesbLnRy7BM0Imo7Anks5rg-dugaJpZM4K9hrU> .

[sanjayssk]

But it should continue to support the old code that does it in the standard TreeView way where the nodes are added and data is assigned immediately. The analogy should still work and should be as fast as possible. Huge capacity is the main feature of VTV but it should not be at the cost of reduced speed that the modifications have caused. Earlier version is extremely fast with the same loop.

…

On Mon, Jan 9, 2017 at 12:42 AM, Joachim Marder ***@***.***> wrote: I think the line Data.S := GenerateString; Should be part of the OnInitNode event handler. Whenever you change the node's stringafter the node has been initialzed, you need to clear the flagvsHeightMeasuredfromTVirtualNode.States`. — You are receiving this because you were assigned. Reply to this email directly, view it on GitHub <#675 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ATX-xv_M9QI7PaBZVzbCli8iczOs_79Xks5rQTUAgaJpZM4K9hrU> .

[joachimmarder]

But I think any height calculation should be postponed
to after EndUpdate in the traditional way if possible.

As long as the height is calculated for all nodes correctly, and so the scrollbars, I don't see any problem with that.

But it should continue to support the old code that does it in the standard TreeView way
where the nodes are added and data is assigned immediately.

Doing the height calculation in EndUpdate() would indeed improve this, as one is no longer forced to clear the vsHeightMeasured flag in this scenario.

Huge capacity is the main feature of VTV but it should not be at the cost of reduced speed that the
modifications have caused. Earlier version is extremely fast with the same loop.

But at the price of wrong scrollbar behavior. I don't think we should trade wrong behavior for speed. The current version is still as fast as the old version in case you have constant node heights. If someone wants maximum speed, he has to follow the virtual paradigm. And he has the choice to perform complex calculations in OnMeasueHeight event handler or return a constant value at no cost.

[sanjayssk]

@joachimmarder
As it stands, there are two closing comments from me on this issue:

1. The application that was reported as broken needs to be changed so that MultiLine for the node is set in OnInitNode event. Only then it will work properly. There is no solution for improving speed though that has been affected as observed above.

2. The breaking change for such multiline applications (using VTV 5.3 or earlier) has been due to the fact that the node height is now determined much earlier when an AddChild is done (SetChildCount) in the sample code given at the top. Earlier versions (5.3 and earlier) postponed the committing of node height till the actual paint after the EndUpdate occurred. There have been numerous version changes since 5.3 and the relevant code has changed a lot in various places. That makes it impractical to try to fix it by moving all that code to after EndUpdate. If we do, we might break more recent applications that don't even use multi line nodes.

Should we close this issue with the label "Breaking change"?

[joachimmarder]

Should we close this issue

Yes.

with the label "Breaking change"

Since nothing was changed for this issue in V6.7 I would use a label like WONTFIX or INVALID for this issue.