[FMX port]Discussions

[livius2]

Hi

As i do not have good place for discussion about some next steps, so i have created topic here.

The question for next patch. Is it possible to add parameter for already virtual methods? Or this is prohibited?
I am thinking about adding parameter to DrawDottedHLine and DrawDottedVLine. I need to add dottedBrush: TBrush. I do not see a way without new parameter.

[joachimmarder]

For me that would be OK. Ideally, the method would be protected (not public) and a default value for the new parameter is added.
Another option would be overloading. Which method do you consider to extend?

[livius2]

DrawDottedHLine and DrawDottedVLine are protected and i need to add dottedBrush: TBrush.
I can add it as default nil and when nil use internally FDottedBrush then on VCL. But this will not work ok as on FMX i need always set its value, Once FDottedBrush in other places FDottedBrushGrid.
As they are protected, then i suppose without default will be ok.

[joachimmarder]

i need to add dottedBrush: TBrush.
I can add it as default nil and when nil use internally FDottedBrush then on VCL. But this will not work ok as on FMX i need always set its value,

Sorry, I'm not familiar with FMX, but can you please explain why you can't hold it in a member variable and need to create it every time?

[livius2]

I shortened the answer ;-)
On VCL pattern is a bit mask, color is applied leter. On FMX it work with color included. So two brushes needed, but they are persistent, i do not create them every time.

[joachimmarder]

Wouldn't it be more consistent, to have also two members for the brushes in VCL as well? That should reduce conditional code for this stuff.

[livius2]

Currently there are two members for VCL too, but as on VCL it is not needed it simple have getter from second.
https://github.com/JAM-Software/Virtual-TreeView/pull/1135/files#diff-7043477841360f7ac0a14b63b865317b48576c9d94e978c21205a3aea0dda736

property DottedBrushTreeLines: TBrush read FDottedBrushTreeLines write FDottedBrushTreeLines;
property DottedBrushGridLines: TBrush read GetDottedBrushGridLines;

But still i need parameters in this methods to pass DottedBrushTreeLines or DottedBrushGridLines.

[joachimmarder]

color is applied leter.

I was unable to find the appropriate code. Can you tell me the method name where this is done?
I'm going to accept your PR in a moment.

[livius2]

Look at DrawDottedVLine, there is Canvas.Brush.Color change
But to Winapi.Windows.FillRect DC of Canvas is passed and separately dottedBrush.Handle.
Without changing the brush color itself, there is a draw of dottedBrush which contain the pattern.
On FMX i do not see equivalent method, so the brtush must contain the color.
We cannot use simple scenario like TStrokeDash.Dot as we have also custom styles OnGetLineStyle.
This still need some fix on FMX, but i will do this after final port, it is not critical at this point.

My biggest concern here was not to write final code, but to eliminate most of the ifdefs from it. Still 200 left :/
But progress is big, i have removed currently ~ 400.

[joachimmarder]

I was able to move a lot of stuff from class TVTAncestorVcl to TVTBaseAncestorVcl. I still wonder if we need both of these platform dependent levels in the class hierarchy, or if we can get rid of one. It seems at least very unusual.

How is your progress in the FMX port, @livius2?

[livius2]

@joachimmarder I do not looked at your current changes, but look at discussion why it was introduced as two classes not one:
#1125 (comment)

How is your progress in the FMX port

To be honest, I stopped due to the workload at work. But it's almost winter, so I'll have more time and I'll come back to the topic. I like to finish it to the beginning of the year. But it also depends on whether we will reach a compromise at certain points when it comes to "ifdef". Because I don't think it's possible to get rid of them 100%, unfortunately. Despite the division into additional classes. But maybe we'll come up with a clever idea.

[joachimmarder]

I do not looked at your current changes

Have you had a chance to check my chnages?

but look at discussion why it was introduced as two classes not one

I'm still not sure that two platform-specific classes are neeeded and my refactorings were a test to see if we can maybe eliminate one.

But it also depends on whether we will reach a compromise at certain points when it comes to "ifdef"

I find it important to centralize them, like we did so far. And I'm sure many can be avoided by using some smart ideas. But without a concrete example, this is difficult to discuss.

[livius2]

I need to incorporate all your changes since the last time I ran this code. And in the meantime, I see a lot has happened. When I'm done, I'll try to compile it for FMX and see if anything bothers me. I'll let you know.

[livius2]

Finally done "merge" and fixes. Pull request created. Now i am ready to move forward.
Some of your changes fixes some problems. And extraction of code to e.g. VirtualTrees.Types was also good choice.
And must say, that having new class hierarchy simplify thing by multiple magnitude level.

[joachimmarder]

And must say, that having new class hierarchy simplify thing by multiple magnitude level.

Well, the quesion remains if TVTAncestorVcl is truly necessary. I was able to move a lot from here to TVTBaseAncestorVcl.

[livius2]

Well, the quesion remains if TVTAncestorVcl is truly necessary. I was able to move a lot from here to TVTBaseAncestorVcl.

Until we finish full FMX port i cannot decide. And removing it, have also consequences.
Now we have "same" hierarchy on both platforms. From user experience POV, it will be more natural i think.
And also if in some point it will be required to introduce TVTAncestorVcl, changing again hierarchy can create another i do not konw if "braking" change, but can be chuge change for someone.

[joachimmarder]

Now we have "same" hierarchy on both platforms.

Yes, but that does not tell anything about if both platform specific classes are truly needed.

From user experience POV, it will be more natural i think.

I don't find it natural to have platform specific classes on two different levels of the class hierarchy.

And also if in some point it will be required to introduce TVTAncestorVcl,

For the moment, we could simply leave it there, but without any methods. And at the end, remove if not needed.

[livius2]

In the VirtualTrees.pas there is InitializeGlobalStructures but nowhere called? Is something broken?

[joachimmarder]

In the VirtualTrees.pas there is InitializeGlobalStructures but nowhere called? Is something broken?

I see a call in TBaseVirtualTree.Create(). Can you please check again, @livius2 ?

[livius2]

But this is different (different code) InitializeGlobalStructures located in VirtualTrees.BaseTree.pas but InitializeGlobalStructures from VirtualTrees.pas is not used.

[joachimmarder]

Sorry, you are right, @livius2, I missed that the method was split up.

[joachimmarder]

I would like to make a release of V8 this weekend. Are there any concerns from your side doing this?

[livius2]

From my side no.