Refactoring - splitting up VirtualTrees.pas

[vincentparrett]

So now that most of the reliance on friend access has been remove, we can attempt to split things up to make the code base easier to work on.

Proposed split units (and where I am up to)

• VirtualTrees.Types
• VirtualTrees.Options
• VirtualTrees.Columns
• VirtualTrees.Header
• VirtualTrees.DragnDrop
• VirtualTrees.DragImage
• VirtualTrees.DataObject
• VirtualTrees.EditLink
• VirtualTrees.Colors

There will likely be a few more as I work through this.

Now a lot of the types are used in published props and event signatures, so what I am doing is adding aliases at the top of VirtualTrees.pas for those types - that will hopefully avoid breaking anyone's code (although the worst that would happen is they need to add to their uses clause).

There will be some (minor) breaking interface changes with this. For example TVTHeader exposes a TreeView property of type TBaseVirtualTree - that was only possible due to them being in the same unit.. with the split this becomes an issue of circular references. I changed this to TCustomControl and cast where needed in the code. There are a few more cases like this that couldn't be helped. I also needed to change the scope of some methods to public, I will try to avoid that using a cracker class where possible.

I'll update this issue with any breaking changes as I come across them. In the work I have done so far, VirtualTrees.pas shrunk from 38K to 27K lines - still way to large for lots of tooling (like github) but more manageable - and there is still more to split out.

Hopefully I should have a PR ready for review in a day or so.

[vincentparrett]

So I learned something about delphi doing this.

When using type aliases

unit VirtualTrees.Options;
type
  TVTPaintOption = (
    toHideFocusRect,  
...

unit VirtualTrees;

uses
   VirtualTrees.Options;

type
  TVTPaintOption = VirtualTrees.Options.TVTPaintOption;

unit MyForm;
....
procedure TMyForm.FormCreate;
begin
  TreeList.TreeOptions.PaintOptions := TreeList.TreeOptions.PaintOptions + [toHideFocusRect];
end;

You see an error TMyForm.FormCreate - Undeclared identified 'toHideFocusRect'

So I guess type aliases for enums are pretty useless.

This makes it impossible to avoid breaking changes. In this case the breakage is just that the user need to add another unit to the uses clause, but I was hoping to avoid that as it's an inconvenience. Not sure if there is any other way around this.

[pyscripter]

Maybe it is time to enforce scoped enums (also breaking change).

[vincentparrett]

I did think about that, and that does actually solve the type alias issue.

[vincentparrett]

There are plenty of other code style issues I'd love to attack as well. My pet hate, with statements using multiple identifiers

e.g

with Header, FixedAreaConstraints, TreeView do
begin

makes the code very hard to understand and work with.

But for now, I'm focusing on just splitting out code in to separate units.

There's also a lot of drilling going on, ie column.columns.header.treeview.method sort of thing. Thats more of a design issue and not easily fixed.

VT has grown too big, with too many options and the interaction between the options is hard to reason because the code is so hard to read. Styling and scaling are still issues. I have another scaling issue to look at that I discovered today (start the app on a higher dpi screen and then move it to lower, header doesn't scale correctly).

Anyway, it's too big to rewrite, so all we can do is chip slowly away at it and try to improve it. It's still hands down the best tree control out there.

[luebbe]

There are plenty of other code style issues I'd love to attack as well. My pet hate, with statements using multiple identifiers

e.g

with Header, FixedAreaConstraints, TreeView do
begin

oh yes ...

Something about code formatting: I'd really love to see a Delphi formatter configuration checked in alongside the sources of as many Delphi projects as possible. I'm one of these guys who hits Ctrl+D more often than Ctrl+F9.
Especially when I'm working on open source stuff, I have to tie my left hand to my back in order to avoid formatting everything accidentally.
So if you split up the sources into different units, maybe you can agree upon a common formatting with @joachimmarder and check the configuration file in.
Since your work will result in a mega-diff anyway, it would not hurt so much to do it now.

[vincentparrett]

While delphi allows you to save/load formatter settings, it's not like other editors that support editorconfig where you can ship the editorconfig file with the code and the editor will respect the settings.

I typically have the delphi formatter disabled, as it really messes up generics.

[vincentparrett]

Delphi's code formatter will just not leave formatted comments alone, anyone know the right options to tell it to leave comments as is?

[luebbe]

While delphi allows you to save/load formatter settings, it's not like other editors that support editorconfig where you can ship the editorconfig file with the code and the editor will respect the settings.

I know, but the I can at least actively load the standard for the project.

I typically have the delphi formatter disabled, as it really messes up generics.

Really? Probably it formats them consistently in a way that you are not used to... ;)

We've got a company standard (mostly if-then-begin-end-linebreak stuff, the rest is pretty much default). Have neither looked at it nor touched it for years. Everyone is using generics and the formatter formats the code consistently. No "messing up" of generics observed. I also think that comments are left "in order".
Could you point me to a code snippet where you don't want comments to be touched? I'll run it through our formatter and see what happens.

[vincentparrett]

When I say messing up generics it was just a spacing issue.. I spent some time playing with the formatter today and managed to get it configured to roughly my liking. That said, it still messed with comments in ways I didn't like (for example the tabulated comments on the options enums).

[joachimmarder]

There will be some (minor) breaking interface changes with this. For example TVTHeader exposes a TreeView property of type TBaseVirtualTree - that was only possible due to them being in the same unit.. with the split this becomes an issue of circular references. I changed this to TCustomControl and cast where needed in the code

Well, I guess this will break a lot of code out there, while having only little benefit. Looking at the VCL or .Net TreeView It is also uncommon to use such base class types to reference the control.

It seems more beneficial to me to extract the TVirtualDrawTree and TVirtualStringTree to own units.

Although the worst that would happen is they need to add to their uses clause

I think this is acceptable. Especially if you extract an enum, const or a small type but add an alias to it, this does not really reduce the size of the unit.

[luebbe]

That said, it still messed with comments in ways I didn't like (for example the tabulated comments on the options enums).

  TPoOption = (
    removecomments,  // remove all comment lines from the .po file
    normalizeheader, // remove header lines that contain potential differences
                     // Another blank comment line
    ignoreheaderdiff // don't save the downloaded file when there are only header changes
    );

This is the result of running the delphi formatter with our settings over an enum with comments. "Another blank comment line" isn't touched by the formatter, so you have to adjust it manually if the lengths of the enum names change. Is this ok for you?

[vincentparrett]

Well, I guess this will break a lot of code out there, while having only little benefit

Well I thought that would be the case - I use Virtual Treeview extensively and the only breaking change was with the enums. YMMV. The benefit for the users initially will be zero.. that was never my intention with this - the benefit will be easier maintenance of the code in the future.

My use of type aliases was an attempt to avoid too many (even minor) breaking changes.

I wonder how many people use the Header.TreeView property - if you have access to the header you likely also have access to the tree control. Same applies to Columns.Header prop. I commented both out, the code still compiles. Internally in the implementation section I used class helpers to avoid casting everywhere

type
  TBaseVirtualTreeCracker = class(TBaseVirtualTree);
  TVTHeaderCracker = class(TVTHeader);

  TVirtualTreeColumnHelper = class helper for TVirtualTreeColumn
    function TreeView : TBaseVirtualTreeCracker;
    function Header : TVTHeaderCracker;
  end;

  TVirtualTreeColumnsHelper = class helper for TVirtualTreeColumns
    function Header : TVTHeaderCracker;
    function TreeView : TBaseVirtualTreeCracker;
  end;

{ TVirtualTreeColumnHelper }

function TVirtualTreeColumnHelper.Header: TVTHeaderCracker;
begin
  result := TVTHeaderCracker(Owner.Header);
end;

function TVirtualTreeColumnHelper.TreeView: TBaseVirtualTreeCracker;
begin
  result := TBaseVirtualTreeCracker(TVTHeaderCracker(Owner.Header).GetOwner);
end;


{ TVirtualTreeColumnsHelper }

function TVirtualTreeColumnsHelper.Header: TVTHeaderCracker;
begin
  result := TVTHeaderCracker(FHeader);
end;

function TVirtualTreeColumnsHelper.TreeView: TBaseVirtualTreeCracker;
begin
  result := TBaseVirtualTreeCracker(Header.GetOwner);
end;

@pyscripter suggestion to use scoped enums actually solves the type alias issue, in that you won't need to add another unit to the uses, just prefix the values with the enum type.. it's really 6 of one half a dozen of another and I think adding to the uses clause is less effort that fixing up every reference to enum values. I used scoped enums in my own code, but that's because I do a lot of c# work and it keeps it consistent between languages (easier on my brain) - but I wouldn't force it on the thousands of users of VT at this stage.

As for extracting the TVirtualDrawTree and TVirtualStringTree to own units, absolutely.. I just didn't want to do too many changes in one go.. what I have done so far is already a lot.. (way more than I would usually do for a PR) but mostly it's just moving code from VirtualTrees.pas to new units and then fixing up access to protected and private members.

[vincentparrett]

This is the result of running the delphi formatter with our settings over an enum with comments.

Can you share your formatter settings?