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

[FMX port]Discussions #1134

Open
livius2 opened this issue Oct 17, 2022 · 23 comments
Open

[FMX port]Discussions #1134

livius2 opened this issue Oct 17, 2022 · 23 comments
Labels
Open for Discussion There are several possibilites to address the issue and anyone is invited for comments.

Comments

@livius2
Copy link
Contributor

livius2 commented Oct 17, 2022

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 joachimmarder added the Open for Discussion There are several possibilites to address the issue and anyone is invited for comments. label Oct 18, 2022
@joachimmarder
Copy link
Contributor

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
Copy link
Contributor Author

livius2 commented Oct 18, 2022

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
Copy link
Contributor

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
Copy link
Contributor Author

livius2 commented Oct 22, 2022

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
Copy link
Contributor

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
Copy link
Contributor Author

livius2 commented Oct 26, 2022

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
Copy link
Contributor

joachimmarder commented Oct 27, 2022

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
Copy link
Contributor Author

livius2 commented Oct 27, 2022

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
Copy link
Contributor

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
Copy link
Contributor Author

livius2 commented Sep 24, 2023

@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
Copy link
Contributor

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
Copy link
Contributor Author

livius2 commented Oct 29, 2023

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
Copy link
Contributor Author

livius2 commented Oct 29, 2023

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
Copy link
Contributor

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
Copy link
Contributor Author

livius2 commented Oct 31, 2023

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
Copy link
Contributor

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
Copy link
Contributor Author

livius2 commented Nov 5, 2023

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

@joachimmarder
Copy link
Contributor

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
Copy link
Contributor Author

livius2 commented Nov 14, 2023

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

joachimmarder added a commit that referenced this issue Nov 14, 2023
@joachimmarder
Copy link
Contributor

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

@joachimmarder
Copy link
Contributor

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

@livius2
Copy link
Contributor Author

livius2 commented Nov 18, 2023

From my side no.

@livius2
Copy link
Contributor Author

livius2 commented Jun 30, 2024

Just for information, I uploaded current sources that compile under FMX to my fork.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Open for Discussion There are several possibilites to address the issue and anyone is invited for comments.
Projects
None yet
Development

No branches or pull requests

2 participants