Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Derive class that simplifies handling of data for cells in grid #799

Open
urupprecht opened this issue May 24, 2018 · 16 comments
Open

Derive class that simplifies handling of data for cells in grid #799

urupprecht opened this issue May 24, 2018 · 16 comments
Labels
Pull Requests Invited There are no current plans to address the issue, but we would be happy if someone supplies a PR.

Comments

@urupprecht
Copy link

I'm working currently on a kind of Objectinspector, like the one Delphi has. To do so, i've create a lot of "Editors" for the VST, including some, that calls Dialogs, to edit multiple items using a Dialog (for example: The font property of a Label). For this i need to exchange more data between the vst and the editor, so i've done a little enhancement, to exchange pointers instead of a string.

The changes i made, are marked with Beginpatch/Endpatch within the source attached.
patched_VirtualTrees.zip

Would be greate, if this make it to regulare VST 😄

@joachimmarder
Copy link
Contributor

Thanks for the patch.

It however looks a bit strange to me to have a new property Values[] (you forgot the BeginPatch here) that does not much more than firing events consumed in your application. So both the property and the events are used only in the application, which means you could call the event handler in your application circumventing the new Values[] property. I think this property also contradicts the principle of separation of concerns (OK, Delphi was never good here and VTV isn't too).

Please send patches as in the patch or diff file format, our Wiki describes how to create them. This makes it easier to apply the patch to any version, and you cannot forget a line like in this case.

In case we accept this patch, we should consider to add it on the level of TBaseVirtualTree, I don't see anything that is specific to the TVirtualStringTree here.

@joachimmarder joachimmarder added the Open for Discussion There are several possibilites to address the issue and anyone is invited for comments. label May 24, 2018
@pyscripter
Copy link
Contributor

Why don't you just iterate over the selected nodes in your multi-value edit dialog? You can already get an array of selected nodes. Is there really a need to further complicate an already overly complex component?

@urupprecht
Copy link
Author

@joachimmarder
Your right. I'll do the diff-patch as soon as i ' ve a little time :)

@pyscripter
The dialog itself, doesn't know anything about Nodes or VST. So for example, for Font-Property i use the standard TFontDialog from delphi. Also, the editor (in this case a simple Button), don't know anything about specific data. I've designed the Editors for use independed from my specific Application.

The changes to VST doesn't make the component much more complicated. They only gives the user the option to handle data the same way as he does with string values.

@joachimmarder
Copy link
Contributor

The changes to VST doesn't make the component much more complicated.

While this is true it heads the components one step further in the wrong direction by coupling model and view tighter than necessary, which especially will lead newbies to a bad architecture.

@urupprecht
Copy link
Author

No, as the path (events, functions) are already in the component. If not, it would be impossible, to edit a value within the VST. I've only added the same events/functions, that works with pointers, instead of strings.

I don't see, where i'm coupling model and view tighter, as it already is.

@UweRupprecht
Copy link

Ok..found some time :)

I've forked the project, and do the changes within the fork. This way, i can do a pull request for the changes. Hope this is ok ?

@UweRupprecht
Copy link

As said before, i do not see, where i'm coupling model and view tighter. The View already do know nothing about the userdata within the node. The only change is, that the Editor response to changes on the data (and the editor should know, what kind of data has to be edited) using pointer instead of a string.

@joachimmarder
Copy link
Contributor

I'm sorry, but I still do not see the point in piping the data of a control that is designed based on the virtual paradigm through the control. Because the idea of the virtual paradigm is that the controls knows nothing about the data, the controls is supposed to do callbacks to the data. Any other comments on this matter from other users?

@joachimmarder
Copy link
Contributor

Maybe I missed something. I see a new OnGetData event, but it is not used anywhere in the control. Who is firing this event and who is consuming it? If both is the application or form, what is the point in piping the data through the control?

The only change is, that the Editor response to changes on the data (and the editor should know, what kind of data has to be edited)

Yes, of course, the editor is data model specific, it needs to know the data model, but it does not need to know the VirtualStringTree control. Especially it should not use the control to get access to the data model, this should be injected in constructors or through properties.

@UweRupprecht
Copy link

As the onGetText event, the onGetData event is fired within the GetData Method, which in turn is used in the value property.

To be clear, we are talking about inplace editors :) (like the standard one, that is already implemented)

Maybe a example clears some points :)

Think about the Objectinspector of Delphi. In our example, at the Font-Property.
It's displayed as one Main-Property (TFont) and its subproperties (size, name...).

Handling the subproperties can be done, using the normal Text, as most of the real data can be converted to a string.

But the main property (in this case an Object-Instance) is not that easy. So, the editor (or to be exact, the editorlink), has to know this instance to feed the editorcontrol/editordialog with the proper data. In turn, the application must be informed, when it's data needs to be updated.

Its pretty much the same way, normal Strings are handle.

@joachimmarder
Copy link
Contributor

As the onGetText event, the onGetData event is fired within the GetData Method,

But neither OnGetData nor GetData is called by the control, and it is not needed by the control. In contrast to this, the TVirtualStringTree actually uses OnGetText to display the text.

So, the editor (or to be exact, the editorlink), has to know this instance to feed the editorcontrol/editordialog with the proper data.

It needs to know the instance of the data model, but it does not need to know the control. It should receive a reference to the data model through constructor injection or through a property.

Its pretty much the same way, normal Strings are handle.

No. GetText() is needed so that TVirtualStringTree can display the text. There is indeed a SetText(), but for this my above arguments apply too: It leads to bad design when piping changes to the data model through the control. But SetData() was already there when I took over the project.

@UweRupprecht
Copy link

It needs to know the instance of the data model, but it does not need to know the control.

The editorlink needs to know both, the VST, as the VST calls the methods of the editorlink. The editorlink also need to know the editcontrol, as it has to position, display the control, and also feed the editcontrol with the proper data, that needs to be edited. It also needs to transport the edited data (whatever this might be) back to the application.

In normal circumstances, the editlink uses the Text-property of the VST, to do this and as long as you have simple data, this is fine. When it became to more complex data, it's impossible to use the Text-Property.

@joachimmarder
Copy link
Contributor

But the editor does not need to use the control to read from and write to the data model. The editor may interact with the control for UI purposes.

@obones
Copy link
Contributor

obones commented Nov 2, 2018

Just to add a few information, we have our own component derived from TCustomVirtualStringTree which sets everything needed for Grid mode and introduces the following events:

OnGetCellDataType
OnGetCellValue
OnGetEditValue
OnGetDisplayValue

Where the data type is an enum like this:

cdtString, cdtInteger, cdtFloat, cdtPercent, cdtItems, cdtCheckBox, cdtFileName, cdtMultipleItems, cdtDate

and the XXValue events are using a Variant to get the appropriate value from the user code.
This allows us to display our own custom editors depending on the cell data type, complete with a potential button to allow advanced edition.
Obviously, in the cdtString mode, it behaves as the default class.

I'm not sure this should be put in the base class, but it's not that difficult to implement in a derived one.

@joachimmarder
Copy link
Contributor

We could add such a derived class (e.g. a TVirtualTreeGrid control) to this package in its own unit. I would be happy to accept such a pull request.

The advantage is that the base does not get any more complex but the code is available to all users.

@joachimmarder joachimmarder added the Pull Requests Invited There are no current plans to address the issue, but we would be happy if someone supplies a PR. label Feb 19, 2019
@joachimmarder joachimmarder removed the Open for Discussion There are several possibilites to address the issue and anyone is invited for comments. label Apr 8, 2019
@joachimmarder joachimmarder changed the title Handling additional Data Derive class that simplifies handling of data for cells in grid Apr 8, 2019
@Fr0sT-Brutal
Copy link
Contributor

Just to add a few information, we have our own component derived from TCustomVirtualStringTree which sets everything needed for Grid mode and introduces the following events:

I've did something similar for grid and editors. The fact that everyone has to make its own wheel makes me sad. +1 for adding this component to VTV package (at least as Contribution)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Pull Requests Invited There are no current plans to address the issue, but we would be happy if someone supplies a PR.
Projects
None yet
Development

No branches or pull requests

6 participants