Add event that is fired when a check state is needed

[joachimmarder]

Compared to other information like image index and node text the check state is not virtual, which means you need to actively manage and assign it.

In certain situations it can be costly to query the check state. Doing that in OnInitNode can be bad in sorted trees and lists, because every node is initialized if the control is sorted.

For both use cases it would help to have an event that is fired whenever a check state is needed.

[joachimmarder]

Idea: Use exisiting method GetCheckState() whereever a check state is needed, instead of accessing Node.CheckStat directly. Let GetCheckState() fire a new event OnGetCheckState of type: TVTGetCheckState = procedure(Sender: TBaseVirtualTree; Node: PVirtualNode) of object;

[sanjayssk]

There is some difference here. The virtual stuff like image index and text has no Set method because there is no internal storage for these and they are managed entirely by application. But a check state does have an internal Node.CheckState that is used by code everywhere. That means the application has to manage that CheckState anyway by calling Sets in addition to its own management of CheckState. Doesn't that make it non-virtual again?

[joachimmarder]

Doesn't that make it non-virtual again?

As I wrote, the checkstate is currently non-virtual and it will not be virtual after this issue is implemented. But it will be simpler for the developer to initialize only those checkstates that are truly needed.

That means the application has to manage that CheckState anyway by calling Sets in addition to its own management of CheckState.

The idea wasn't to eliminate the CheckState property, but to optimize the moment the application needs to set it.

[sanjayssk]

Current code sets Result as Node.CheckState.
So we will be calling OnGetCheckState with the Result already set to internal check state.
Then the application will modify it if necessary.

Looks simple enough but we should walk through a Scenario.

1. Suppose the internal CheckState of a node is csUncheckedNormal.

2. This is sent to application that disregards it and supplies its own Checked State. The problem is, what will happen to the internal state? So GetCheckState will need to modify internal check state based on what is returned from the event. This seems a little awkward where a Getter is setting the value. This might work as long as the application uses it responsibly only during initialization of the tree. Otherwise if used later, it will complicate the application logic where it will need to sync its own check state with the internal state that is set by the User when working with the tree.

[joachimmarder]

So GetCheckState will need to modify internal check state based on what is returned from the event

I'm sorry, but I don't understand what you mean. What do you consider as the internal check state?

There is only a single truth, and that is Node.CheckState, which will be returned by a getter. We already have this concept of a getter for other members of TVirtualNode too, which should be used instead of accessing the members directly.

[sanjayssk]

I mean Node.CheckState only. And what I understood so far is that you make it possible for the application to return another value for this by an OnGetCheckState event. The application can return a different value and that seems to be the purpose of the event. But what happens to Node.CheckState if the application returns a different value. That I'm not able to understand. The tree internal logic is entirely dependent on Node.CheckState and it does not use GetCheckState for its own stuff.

[sanjayssk]

I mean, GetCheckState is the method for the property CheckState of the tree itself. Whereas internal logic at many places directly uses Node.CheckState or CheckState directly depending on which class it is used in. So I don't see how the application can get by by providing its own value in the event unless it also changes the actual value. That's what is confusing to me.

[joachimmarder]

And what I understood so far is that you make it possible for the application to return another value for this by an OnGetCheckState event.

No, the idea was to fire an event, not to allow returning another value that may differ from Node.CheckState. Please see the event prototype in my 2nd comment.

The application can return a different value and that seems to be the purpose of the event.

No, the purpose of this is event is just to notify the application when this value Node.CheckState is actually needed.

[sanjayssk]

I see. Can you suggest or supply a sample that demonstrates how this event will be used just to compare what happens with and without it? Otherwise, I have no problem simply implementing it but I'm curious. Thanks.

[sanjayssk]

I'm sorry, I got confused by the name of the event OnGetCheckState because the name suggests a value return whereas I failed to notice that there is no VAR parameter passed. I think the name is confusing if its purpose is just to notify, may be something like OnNotifyGetCheckState.

[joachimmarder]

Can you suggest or supply a sample that demonstrates how this event will be used

I cannot supply code here as it is too complex, but I can extent my first comment. We plan to use it in SpaceObServer. Here the check-sates can be persisted in a database. If now a folder with thousands of objects is listed and a sort column is active in the header, the sorting will trigger the OnInitNode for all objects. So querying the check states from the databse in the OnInitNode event handler slows down the filling of the list. Actually only 40-50 of the thousands of objects will be visible after filling and sorting the list, and it would be sufficient to query the check state for those. We may even use work-items to query the check state asynchronoulsy for those objects that are actually visible.

[joachimmarder]

I got confused by the name of the event OnGetCheckState because the name suggests a value return

How about OnNeedCheckState or OnCheckStateNeeded?

[sanjayssk]

OK. I added the event. But I named it OnBeforeGetCheckState based on the description you gave.

Please change the name if necessary and improve the comments that I added in the source file.

[joachimmarder]

In order to make the event have the intended effect, it is important that all accesses of PVirtalNode.CheckState are done through GetCheckState(), please see my comment 2.

[sanjayssk]

The problem is that the Setter of CheckState[Node] itself is going through some functions deep down that use Node.CheckState. Those functions should not use the same property's Getter to avoid a Race condition. On the other hand, these functions might be called from places other than the Setter where they should be calling the Getter. So this is going to be complicated because that logic may also be used from places other than through Setter. This needs a careful analysis of the call hierarchy. I wrongly thought that the purpose of this is just for the application to know when the first call to get a CheckState is made so that it can perform some general initialization. But the real requirement seems to be for the application to know "each time" when a CheckState is asked for.

Perhaps, a better idea would be to make a Minimum Viable Demo that needs to do what you have in mind and then see how we can solve it in various ways.

[joachimmarder]

I wrongly thought that the purpose of this is just for the application to know when the first call to get a CheckState is made so that it can perform some general initialization

Would you say that this goal is already achieved and that for all code paths that potentially do the first access of the CheckState property fire the new event?

But the real requirement seems to be for the application to know "each time" when a CheckState is asked for.

Well, that requirement could be discussed if it makes a difference. I assumed that it wouldn't make a big difference to fire the event for all first accesses, or for all accesses.

[joachimmarder]

The problem is that the Setter of CheckState[Node] itself is going through some functions deep down that use Node.CheckState. Those functions should not use the same property's Getter to avoid a Race condition.

Agreed. We indeed need to be careful here in ChangeCheckState().

[sanjayssk]

Would you say that this goal is already achieved and that for all code paths that potentially do the first access of the CheckState property fire the new event?<<

Not really. For this, some experiments and debugging is needed to see when it gets fired for a Vtree when those features are enabled. Let me get back to you on this after some experiments.

[joachimmarder]

For this, some experiments and debugging is needed to see when it gets fired for a Vtree when those features are enabled

To avoid that work, my idea was to simply redirect all accesses of PVirtualNode.CheckState through GetCheckState(), except for those that are done when changing the CheckState property (-> ChangeCheckState() )

[sanjayssk]

I had a quick look and that calls the Setter SetCheckState internally. This needs some testing to see how often the application is being called in the lower logic.