Initial Port to Firemonkey

[livius2]

Hi,

this is realy big task but if you do not merge this i will continue it as separate repository.
And i suppose this will never go up because repos will continue go separate ways.

Description...
I use Inc and Dec overloads and make every call on INTEGER to have System.Inc, System.Dec.
This avoid many IFdefs and is cleaner way then declaring IncSingle and DecSingle.

The main purpose here was to do not break VCL - it should work unchanged.
This is initial port as is it have:
1. It compiles under FMX - this was main task of this pull request.
2. It draw tree nodes with apropiate levels
3. It draw buttons plus/minus.

No more functionality is working.
To make it fully live it must have:

1. Click support
2. Header support - now it work only with main column.
3. Dragging
4. and any mouse, editing and drawing.

To test FMX prot of VT - you must add in the e.g. Delphi project->Options->Conditional defines
VT_FMX

regards,
Karol Bieniaszewski

below FMX unit (Empty form) i create VT dynammicaly to test its functionality.

unit Unit1;

interface
//{$DEFINE VT_FMX}
{$IFNDEF VT_FMX}
  {$DEFINE VT_VCL}
{$ENDIF}

uses
  System.SysUtils, System.Types, System.UITypes, System.Classes, System.Variants,
  FMX.Types, FMX.Controls, FMX.Forms, FMX.Graphics, FMX.Dialogs, FMX.TextLayout, VirtualTrees;

type
  PNode = ^TNode;
  TNode = record
    S: String[255];
  end;
  TForm1 = class(TForm)
    procedure FormCreate(Sender: TObject);
  private
    { Private declarations }
  public
    { Public declarations }
    VT: TVirtualStringTree;

    procedure VTGetNodeDataSizeEvent(Sender: TBaseVirtualTree; var NodeDataSize: Integer);
    procedure VSTGetTextEvent(Sender: TBaseVirtualTree; Node: PVirtualNode; Column: TColumnIndex; TextType: TVSTTextType; var CellText: string);
  end;

var
  Form1: TForm1;

implementation

{$R *.fmx}

procedure TForm1.FormCreate(Sender: TObject);
Var a: {$IFDEF VT_FMX}Single{$ELSE}Integer{$ENDIF};
  Node, NodeParent: PVirtualNode;
  NodeDane: PNode;
  i, j: Integer;
begin

  a:= 10;
  Dec(a, {$IFDEF VT_FMX}1.8{$ELSE}2{$ENDIF});
  Caption:= FloatToStr(a);

  VT:= TVirtualStringTree.Create(Self);
  VT.Parent:= Self;
  VT.Align:= TAlignLayout.Client;
  VT.Fill.Color:= TAlphaColorRec.Yellow;

  VT.OnGetNodeDataSize:= VTGetNodeDataSizeEvent;
  VT.OnGetText:= VSTGetTextEvent;
  VT.BeginUpdate;
  try
    for i:= 1 to 4 do
      begin
        Node:= VT.AddChild(nil);
        NodeDane:= VT.GetNodeData(Node);
        NodeDane.S:= 'i=' + i.ToString;
        NodeParent:= Node;
        for j:= 1 to 3 do
          begin
            Node:= VT.AddChild(NodeParent);
            NodeDane:= VT.GetNodeData(Node);
            NodeDane.S:= 'i=' + i.ToString + ', j=' + j.ToString;
          end;
        if i mod 2 = 0 then
          VT.Expanded[NodeParent]:= false else
          VT.Expanded[NodeParent]:= true;
      end;
    //VT.FullExpand();
  finally
    VT.EndUpdate;
  end;
end;

procedure TForm1.VSTGetTextEvent(Sender: TBaseVirtualTree; Node: PVirtualNode; Column: TColumnIndex; TextType: TVSTTextType;
  var CellText: string);
Var NodeDane: PNode;
begin
  if Node=nil then
    exit; //!!!
  NodeDane:= VT.GetNodeData(Node);
  CellText:= NodeDane.S;
end;

procedure TForm1.VTGetNodeDataSizeEvent(Sender: TBaseVirtualTree; var NodeDataSize: Integer);
begin
  NodeDataSize:= SizeOf(TNode);
end;

end.

[joachimmarder]

Please understand that I cannot accept such a pull request just because it compiles. But I find it interesting to check what needs to be changed in order to get Virtual TreeView to work on FMX.

Why is there as need for two conditional defines VT_FMX and VT_VCL? It should be fine to use $ifndef instead.

And I was wondering it is possible to change more code in a way that it compiles on both platforms instead of using $ifdefs, as they have many disadvantages. E.g. change parameter types from HDC to TCanvas.

[livius2]

Please understand that I cannot accept such a pull request just because it compiles.

I know but it do not only compiles, it does not break the current VCL code.
Without "compile" it is not possible to move on and support more and more features under FMX.
Now work can be progressive but without initial port it will never happen.

Why is there as need for two conditional defines VT_FMX and VT_VCL? It should be fine to use $ifndef instead.

It is trivial choice. It is visualy distinctive when "$ifndef FMX" and "$ifdef FMX" is not so simple.
And VT_VCL is automatically defined only someone must define manualy VT_FMX when working under Firemonkey.
And this can be simple changed if really needed by find replace.

And I was wondering it is possible to change more code in a way that it compiles on both platforms instead of using $ifdefs, as they have many disadvantages. E.g. change parameter types from HDC to TCanvas.

My purpose was to not break current VCL code. If we change HDC to TCanvas this can broke some applications which operate on HDC directly without Canvas. I remember that i have used it in some applications where i print something using VT but printing device have not supported Canvas.
Of course this can be fixed by create dummy Canvas and assign Hande:= DC; but this require change in user code.
And as above it can be simple changed by find replace in future development if needed.

[joachimmarder]

Without "compile" it is not possible to move on and support more and more features under FMX.

But why do these changes need to be in the master branch?

When I took over the project several years ago it was a hell of $ifdefs, and code changes easily broke a code path that I did not compile on my local IDE. It took a long time to clean all this up and I don't intend to get back there.

I am neither convinced that it makes sense to have all this in a single source code, nor that this will finally work, because Virtual TreeView was built closely tied to VCL and Win32 API. Especially the painting and TBitmap related stuff could get difficult. FMX was not designed with this use case in mind.

My purpose was to not break current VCL code.

But my aim is to have clean code from the beginning.

If we change HDC to TCanvas this can broke some applications which operate on HDC directly without Canvas.

But a VCL canvas always has a HDC on that you can operate. But you can use the abstraction that TCanvas offers whenever you do not need to work on HDC, and use HDC when truly needed.

We should also define an alias for Integer/Single instead having an $ifdef at each parameter.

[livius2]

But why do these changes need to be in the master branch?

I do not suppose that dividing this by new branch do something useful. I am opposite, this will bring code maintenance to difficult level.

I am neither convinced that it makes sense to have all this in a single source code, nor that this will finally work, because Virtual TreeView was built closely tied to VCL and Win32 API. Especially the painting and TBitmap related stuff could get difficult. FMX was not designed with this use case in mind.

Painting is not the problem at all. I make painting of the tree itself as you can see if you run attached code. Tree is painting ok, nodes and its levels, buttons +/-.
I have more painting in my local code - but it is from my first try, year ago. It is not so clean as this attached commit. I work progressively on this, but this require to have code merged.
I can work on some features but one person will do this too slow i suppose.
You see that Firemonkey is many years on the market but no one successfully make VT port to FMX.
This can happen now. And this is required for me presonally. I will do the job even if you do not merge this. But then this will be in my private repo and no one will benefit from this.

built closely tied to VCL and Win32 API

Not so much as we can suppose. The hardest task will befull mouse support.
Painting i can migrate without problem. It require time but it is not so hard.

But my aim is to have clean code from the beginning.

Code is quite clean. Look at Delphi source code - there is much more ifdefs and it is working ok.
We can progressively cleen up some ifdefs, e.g. for variables inside procedures not expressed outside.
But if we change parameter types, especially for events - we will get hard migration for the users projects. You know, Delphi will tell you "incompatibile parameter types remove event?".

But a VCL canvas always has a HDC on that you can operate. But you can use the abstraction that >TCanvas offers whenever you do not need to work on HDC, and use HDC when truly needed.

look above about compatibility. It can be easly done but require that users must change their already compilable projects. If this is really acceptable we can go this way, but for me it is not required.

We should also define an alias for Integer/Single instead having an $ifdef at each parameter.

as previously about compatibility. But this can be done for variables inside procedures not expressed outside. Remember only about some warnings introduced. As you know sometimes some parameter is Integer other Cardinal. And calculation if changed, can generate warnings which should be "fixed".

And as summary. I understand your worry.
But I think that the profit is much bigger than the risk taken.
And if we not do this, no one do this for us :)

[pyscripter]

Why not add this to a public branch, so that anyone interested can benefit and contribute and wait till it matures and becomes working before merging it to the main branch. I would vote against merging into the main branch stuff that do not fully work.

[livius2]

The longer this code will be in a separate branch, the more difficult it will be to merge, because it will be more and more forked. However, merging this now does not affect the functionality of VCL. Anyone who wants to test FMX will have to add the VT_FMX directive manually, for this reason, it will not be officially supported yet..

[joachimmarder]

I do not suppose that dividing this by new branch do something useful. I am opposite, this will bring code maintenance to difficult level.

Sorry, but I think the opposite is true. It is a very uncommon approach.

You see that Firemonkey is many years on the market but no one successfully make VT port to FMX.

I don't think the possibility of using Git branches prevented this.

Look at Delphi source code - there is much more ifdefs and it is working ok.

I wasn't saying that it cannot work, it is just a difficult and time consuming to maintain. And I am doing this in my spare time, while Embarcadero gets money for the product.

Delphi will tell you "incompatibile parameter types remove event?".

Yes, for events this is a different story. But for many function I don't see a problem. It may not be popular among all users of VT, but I prefer a clean break if I get rewarded by by clean source code afterwards.

As you know sometimes some parameter is Integer other Cardinal.

Yes, this is very annoying. While Cardinal / UInt is often formally correct, you typically end up in casting them to prevent compiler warnings, because VCL or other functions use Integer. I don't see any benefit in using Cardinal here and there. In the past 20 years so much ancient and obscure code was collected in Virtual TreeView that a cleanup would be important, even if it hurts in the form of breaking changes.

The longer this code will be in a separate branch, the more difficult it will be to merge, because it will be more and more forked.

No, you just need to regularly rebase the code to the latest code in master, in which I will make changes that make $ifdefs obsolete (like HDC => TCanvas). When your code is stable we can merge it.

[pyscripter]

@livius2 If the FMX support is in a separate branch while it is being developed, you can always sync with the main branch from time to time.

Look what happened to SynEdit. It used to be a flagship Delphi component. It tried to maintain compatibility with Kylix and all Delphi versions since version 3 or something. There are more IFDEFs than code lines. Kylix compatibility is still in, even though I do not think there is a single Kylix user in the word. And then people started putting half-baked functionality in. The current maintainer @CWBudde decided to include GDI+ support. Nice idea, but he lost interest and never completed the work. The non-working GDI+ support is in the master branch. The code has become incredibly complex and virtually unmaintainable.

The story with VT was quite similar. Too many options, overly complex and hard to maintain. But @joachimmarder has done a great job by having a fresh start, focusing on compatibility with newer versions only, streamlining the code and resisting requests to add new options, new events or whatever. Great job. Don't spoil it.

[livius2]

I am not totally opposite to have new branch.
It have some benefits.
But, what make this new branch better approach?
If now we have code working on VCL and changes to FMX should not affect it.
No need to rebase/sync whatever.
New functionality ported only need to post new commit to the master without merging between branches. I can follow master or new branch, you can decide..

---

About Integer / Cardinal. I can change any place (excluding events) to have

type
  TDimension = Integer; //VCL
  TDimension = Single; //FMX

Do you have better name for it? And Integer is enought? or it should be Int64?
I can make changes also for TCanvas/HDC if you need (first in conflicted already places).
This choice can really minimize the need for ifdefs

[joachimmarder]

I like TDimension. It should be Integer for VCL, as this is type typically used here, even if negative values should not occur. There are a lot of types and aliases defined for FMX as well as some substitute function that do not exists on FMX, we should move them this in a new unit VirtualTrees.FMX to have them seperated and to keep changes in main unit small. I can merge this unit in a early stage as it won't interfere with the rest.

You will need patience with me, I will always seek for the cleanest solution. Git makes it relatively easy to keep this separated for a long time without going separate ways.

I opened issue #841 for the most important changes. Let me know what I missed.

[livius2]

I will always seek for the cleanest solution.

I too. I only do not know what are the preferences here.
For example i see now that creating new unit is not something which is avoided but prefered.

We should determine who does what to not to duplicate the work.
I can wait for your rework or
i can replace "all" occurrences Integer/Single ifdefs and make a new commit.
If also accepted i can change HDC to TCanvas to avoid ifdefs.

[joachimmarder]

i see now that creating new unit is not something which is avoided but prefered.

Yes, I would say it is a commonly agreed best practice to not have such monster units like VirtualTrees.pas.

We should determine who does what to not to duplicate the work.

Yes. The points in #841 will be done in master branch. I will do them when I find time, I will also accept pull requests that focus exactly on one of these aspects, e.g. replacing Integer / Cardinal by TDimension. Whoever starts a job should post a comment in #841.

[joachimmarder]

If also accepted i can change HDC to TCanvas to avoid ifdefs.

Yes, but it hard to tell in advance if this will go smoothly or not. In any case please do this is an own commit / pull request.

[livius2]

Ok, i have made changes.

• changed all to TDimension - except events
• changed HDC to TCanvas - sometimes this require dummy canvas but no overhead as i create it only once (this must be used with care - i see no problem now but if some procedure call other procedure and there is also dummy canvas needed then maybe better is to create it always instead of reussing same object, or simply restore handle before exit from procedure - i go to this way as this provide minimal overheads)
• moved code to new VirtualTrees.FMX.pas
• fixed some mistakes in previous commit

[joachimmarder]

changed all to TDimension - except events

Sorry, but these changes have been done in your branch. If I accept the PR now, I will get all the changes including all incomplete FMX stuff. As I wrote in an earlier comment the changes I suggested in #841 should be done in the master branch and should be one commit or PR per change.

[pyscripter]

I see no benefit in merging any changes now. Why not delay merging until the FMX stuff becomes stable. You risk breaking existing code especially if you change the signature of events.

Remember also that there are third party components descending from VT.

[joachimmarder]

Why not delay merging until the FMX stuff becomes stable.

I wanted to prevent a monster-merge. The non-breaking aspects could be done in advance and would so reduce the diff between both branches. But, yes, we don't need to hurry. If showstoppers come up, the changes would be of no benefit.

[pyscripter]

You will not know whether showstoppers will arise or @livius2 loses interest or development stops for any reason, unless you wait for a stable FMX version to emerge. By that time the master branch will be unrecognizable for no reason. Your priority should be to preserve the integrity of the master branch on which many users depend.

[pyscripter]

There is also a Lazarus port. I saw no requests to merge it with this project. How would you deal with such requests?

[pyscripter]

Don't take me wrong. I can see the benefits of having VT available on MacOS or even Android.

[livius2]

Sorry, but these changes have been done in your branch. If I accept the PR now, I will get all the changes including all incomplete FMX stuff. As I wrote in an earlier comment the changes I suggested in #841 should be done in the master branch and should be one commit or PR per change.

You can cherry-pick particular commits if you need it now on the master branch.
And separete action put this whole pull request into separate branch.

@pyscripter
I supose decision should be taken what have higher prioryty.
I am very interested in porting VT to Firemonkey also if functionality will be strongly limited at first glance.

If this new branch for FMX port will be finally commited into master i can also help with Lazarus.
I have used it in the past occasionaly, because i use Delphi and now we have Delphi Community edition.
The time resources are strongly limited then decision must be taken.

I suppose that some simple commits for Lazarus compatibility is the best way.
Creating another big pull request for Lazarus will be hard to merge.

[livius2]

Do you think that merging is possible in near feature?
Because merge new changes from master is too costly and error prone.

[joachimmarder]

Because merge new changes from master is too costly and error prone.

Why exactly?

Do you think that merging is possible in near feature?

Currently the PR does not merge due to conflicts.

It was hard work in the past years to get Virtual TreeView to a stable state and I have concerns that the FMX support that is work in progress may cause new problems, and that I need to deal with the merge problems you are currently dealing with.

I would prefer to have this in a separate fork and to get both forks closer together smoothly, e.g. by using TDimension where necessary in the official branch, by merging VirtualTrees.FMX etc.

[livius2]

Why exactly?

When some change is in the master i must compare it to my already changed/different sources.
Then merging in some situations are not trivial.
It is different task then incorporate change into master and test it against vcl fmx if we will have this merged already.

But i understand that merging big change to source which work good is a risk task.
I like to see TDimension in the master. This is some step forward.

[joachimmarder]

When some change is in the master i must compare it to my already changed/different sources.
Then merging in some situations are not trivial.

Well, I fear this applies to both directions.

like to see TDimension in the master.

Me too. Is there any chance that you send a pull request for this?

[livius2]

Well, I fear this applies to both directions.

If you merge my changes, it is already merged with your code and you see only changes to this code.
This is simple side by side comparision.
But if i need to merge your changes i have 3 files: orginal version which i hve changed in my code, your new version with change and my file modified sources. If change is simple i compare side by side but with other i must work on 3 files.

like to see TDimension in the master.

Me too. Is there any chance that you send a pull request for this?

I have TDimension changed VT but i do not know how to create pull request with this file.
When i try with GithubDesktop it do not work as desired. It always compare it with my all sources also if i create new branch. I am not fluent with git.
On Github page i cannot do this online because the size of file is bigger then allowed.
When i clone main repository and try to create pull it show nothing on the page.

Any hint how to do this?

[joachimmarder]

I'm sorry, I am also not a Git expert. Maybe we can use cherry-picking to achieve this.

[livius2]

I see no way to do this from my fork, also from new branch with only this change :/
It always show me whole comparision tree in pull request.
I also can not create second fork with one account. Second account violate github law.

I'm stuck.

I see the way :)
You should create new branch "TDimension" and add privilege in this branch to me. I can then commit to it directly and create pull request from there
https://help.github.com/en/articles/enabling-branch-restrictions

[joachimmarder]

I just looked at some of your changes. The are breaking chnages, for example: TVirtualNode.TotalHeight currently is a Word (16Bit) while in your code it is a TDimension (32Bit integer). Many variables changed from unsigned to signed which causes several warnings in external code. And I think there are way too many $ifdefs in the code, they make code difficult to read and you can easily break code in the non-active parts. And can't published FMX properties have a default value?
I am still not convinced that having all this in a single source would be a wise decision. I think s separate forks makes more sense. They may still get more similar over time.

[livius2]

I just looked at some of your changes. The are breaking chnages, for example: TVirtualNode.TotalHeight currently is a Word (16Bit) while in your code it is a TDimension (32Bit integer).

it is known consequence of the change, but this make code much more understandable.
We eliminate Word, SmallInt, Cardinal and make one single same type.
And current 64k(Word) can be limiting if someone need to print on high resolution printer device like a plotter (i have worked on some project which was "near" this limit).

Many variables changed from unsigned to signed which causes several warnings in external code.

Same as above, but we can make some checks to prevent negative values. And remove then warnings.

And I think there are way too many $ifdefs in the code, they make code difficult to read and you can easily break code in the non-active parts.

Do you think about final merge or TDimension one? TDimenison have not ifdefs.

And can't published FMX properties have a default value?

Float (Single) can not have default also in VCL.

I am still not convinced that having all this in a single source would be a wise decision.
I think s separate forks makes more sense. They may still get more similar over time.

this is your choice, but do you see more problems with TDimension introducing?
I can make then another patch with inc, dec, move
next with canvas and more..

[joachimmarder]

it is known consequence of the change, but this make code much more understandable.

Sure, but it is still a breking chnage as it changes the memory layout of the central data structure of Virtual TreeView. I guess someone would need to write new version of the reader and writer. If we would start on a blank piece of papier, there would be no question which type to use, but there is a lot of legacy code around. And users hate if we break their code.

Same as above, but we can make some checks to prevent negative values.

Negative valuea are not the problem, seems tha previous developers have seen this quite pragmatic:
Inc(Integer(Run.TotalCount), Difference);

And remove then warnings

We cannot fix warnings in 3rd party code that is written against Virtual TreeView code. Anyway, I opened issue #892 for this specific topic.

Do you think about final merge or TDimension one? TDimenison have not ifdefs.

No, I am talking about the huge number of other $ifdefs that we won't be able to remove in a single code base.

do you see more problems with TDimension introducing?

No yet, but the two mentioned above are serious enough.

[livius2]

We can include compatibility config
{$IFDEF compatibility}Word, Cardinal whatever{$ELSE}TDimension{$ENDIF}
and include it in documentation

ifdefs are not so bad as they looked

[joachimmarder]

ifdefs are not so bad as they looked

They are bad, believe me. I had a lot of trouble with them. You simply can't be sure that every code path compiles or that they work as intended. If you have many of them, you need to check the cross product of them to be sure.

[livius2]

Even though the current fork was sufficient for my purposes, I decided to get back into activity. Maybe we can do something useful for others. I will be uploading divided patches. And i will provide next patch after merging the previous one to gradually reconcile the code. I will add to the title [FMX port]. We will see how far it can be integrated. But I don't see an option without "ifdef" yet ;-)