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?

[luebbe]

here you go. I appended .txt to make Github happy.

Formatter_Luebbe.config.txt

[vincentparrett]

Can you share your formatter settings?

Actually never mind, I found the setting, Align end of line comments. It kinda works ok, but still messes up in places

  TVTPaintOption = (toHideFocusRect,  // Avoid drawing the dotted rectangle around the currently focused node.
    toHideSelection,                  // Selected nodes are drawn as unselected nodes if the tree is unfocused.
    toHotTrack,                       // Track which node is under the mouse cursor.
    toFullVertGridLines,              // Display vertical lines over the full client area, not only the space occupied by nodes.
                                      // This option only has an effect if toShowVertGridLines is enabled too.
    toAlwaysHideSelection,            // Do not draw node selection, regardless of focused state.
    toUseBlendedSelection,            // Enable alpha blending for node selections.

Gets changed to

  TVTPaintOption = (toHideFocusRect,  // Avoid drawing the dotted rectangle around the currently focused node.
    toHideSelection,                  // Selected nodes are drawn as unselected nodes if the tree is unfocused.
    toHotTrack,                       // Track which node is under the mouse cursor.
    toFullVertGridLines,              // Display vertical lines over the full client area, not only the space occupied by nodes.
// This option only has an effect if toShowVertGridLines is enabled too.
    toAlwaysHideSelection,  // Do not draw node selection, regardless of focused state.
    toUseBlendedSelection,  // Enable alpha blending for node selections.

I can cope with that.. I ran it on the new units I created but won't run it on the existing units. I'll add a formatter.config file with my PR.

[luebbe]

I guess it'll work fine if you put toHideFocusRect on a separate line. Edit: no you don't

This is the unedited result with my settings. I just pasted your original and closed the enum:

  TVTPaintOption = (toHideFocusRect, // Avoid drawing the dotted rectangle around the currently focused node.
    toHideSelection,                 // Selected nodes are drawn as unselected nodes if the tree is unfocused.
    toHotTrack,                      // Track which node is under the mouse cursor.
    toFullVertGridLines,             // Display vertical lines over the full client area, not only the space occupied by nodes.
                                      // This option only has an effect if toShowVertGridLines is enabled too.
    toAlwaysHideSelection,           // Do not draw node selection, regardless of focused state.
    toUseBlendedSelection,           // Enable alpha blending for node selections.
    asdf
    );

[vincentparrett]

I guess it'll work fine if you put toHideFocusRect on a separate line.

Except the formatter puts it back on the first line. Looking for that option now (if it exists).

[vincentparrett]

Try it on the full TVTPaintOption as it is in VirtualTrees.pas

[luebbe]

Unedited. Just pressed Ctrl+D on VirtualTreevies.pas (Delphi 10.4.2) Maybe they have improved the Formatter...

// There is a heap of switchable behavior in the tree. Since published properties may never exceed 4 bytes,
  // which limits sets to at most 32 members, and because for better overview tree options are splitted
  // in various sub-options and are held in a commom options class.
  //
  // Options to customize tree appearance:
  TVTPaintOption = (
    toHideFocusRect,         // Avoid drawing the dotted rectangle around the currently focused node.
    toHideSelection,         // Selected nodes are drawn as unselected nodes if the tree is unfocused.
    toHotTrack,              // Track which node is under the mouse cursor.
    toPopupMode,             // Paint tree as would it always have the focus (useful for tree combo boxes etc.)
    toShowBackground,        // Use the background image if there's one.
    toShowButtons,           // Display collapse/expand buttons left to a node.
    toShowDropmark,          // Show the dropmark during drag'n drop operations.
    toShowHorzGridLines,     // Display horizontal lines to simulate a grid.
    toShowRoot,              // Show lines also at top level (does not show the hidden/internal root node).
    toShowTreeLines,         // Display tree lines to show hierarchy of nodes.
    toShowVertGridLines,     // Display vertical lines (depending on columns) to simulate a grid.
    toThemeAware,            // Draw UI elements (header, tree buttons etc.) according to the current theme if
                               // enabled (Windows XP+ only, application must be themed).
    toUseBlendedImages,      // Enable alpha blending for ghosted nodes or those which are being cut/copied.
    toGhostedIfUnfocused,    // Ghosted images are still shown as ghosted if unfocused (otherwise the become non-ghosted
                               // images).
    toFullVertGridLines,     // Display vertical lines over the full client area, not only the space occupied by nodes.
                               // This option only has an effect if toShowVertGridLines is enabled too.
    toAlwaysHideSelection,   // Do not draw node selection, regardless of focused state.
    toUseBlendedSelection,   // Enable alpha blending for node selections.
    toStaticBackground,      // Show simple static background instead of a tiled one.
    toChildrenAbove,         // Display child nodes above their parent.
    toFixedIndent,           // Draw the tree with a fixed indent.
    toUseExplorerTheme,      // Use the explorer theme if run under Windows Vista (or above).
    toHideTreeLinesIfThemed, // Do not show tree lines if theming is used.
    toShowFilteredNodes      // Draw nodes even if they are filtered out.
    );
  TVTPaintOptions = set of TVTPaintOption;

[vincentparrett]

I'm using 10.4.2 but see different results.. I will compare formatter settings and see what is causing the difference.

[joachimmarder]

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

We have a derived control class and some helper functions that use Header.TreeView or Column.Header. Nothing that coudn't be fixed with 15 minutes if work. I don't say it's a no-go, but it should be a well thought out decision.

I still think extracting TVirtualDrawTree and TVirtualStringTree would be the better next step.

[vincentparrett]

Draft PR submitted .

[joachimmarder]

Some observations:

• I vote for merging the const, enums and sets in one central unit, like VirtualTrees.Types. There should be aliases for the most common ones in the VirtualTrees unit, like sdDescending, sdAsending or NoColumn and the TCheckState members. This is especially important as we sometimes use them full qualified in our code to avoid ambiguities.
• The type TCheckState shouldn't be in the unit VirtualTrees.Columns, at least it is not the place where I would have searched it as it is not entirely column-related.
• It seems that the property TVirtualTreeColumns.TreeView was lost.

For a complex project like TreeSize, that makes intense use of Virtual TreeView, the breaking changes are annoying. For the moment, I will give up after 40 minutes of compiler errors.

I still think we are starting this from the wrong end and should begin with extracting TVirtualDrawTree and TVirtualStringTree.

[vincentparrett]

Points taken, I will move things around as suggested.

As for extracting TVirtualDrawTree and TVirtualStringTree - I wanted to start with smaller breaking changes. I can do those too in this PR if you want, my thought was to do them in a follow up.

[vincentparrett]

I have pushed the changes and confirmed they build with XE3-10.4

[vincentparrett]

I looked at extracting TVirtualDrawTree and TVirtualStringTree and it looks pretty simple, however we can't add alias's in VirtualTrees for them due to circular references - this would require that users update their uses clause. Let me know if you want me to do it in this PR.

[joachimmarder]

Thanks for making the recent changes. I reverted my TreeSize project and gave it another try. I found these chnages that could STRONGLY reduce the necessary changes:

• Integrate VirtualTrees.Options in VirtualTrees.Types. Then the typical change that is necessary in an exisiting unit WHICH uses VTV is to add VirtualTrees.Types to the uses.
• I needed to add VirtualTrees.Columns to some of our units just because enum members like coVisible were used. If we move those enums to VirtualTrees.Types as well, then adding this Types unit would be sufficient.

[joachimmarder]

I looked at extracting TVirtualDrawTree and TVirtualStringTree and it looks pretty simple, however we can't add alias's in VirtualTrees for them due to circular references - this would require that users update their uses clause. Let me know if you want me to do it in this PR.

I would definitely vote for extracting TVirtualDrawTree into a new unit as it is rarely used. I am unsure about TVirtualStringTree.

An option would be to make a major release V8.0 and include a few other breaking changes, especially #832. I don't think there is a benefit in having several releases of that each has smaller breaking changes.

[vincentparrett]

I have pushed the suggested changes, including splitting out TVirtualDrawTree. I didn't tackle #832 (although I think it's a good idea).

[joachimmarder]

Thank you! I found two cases that cause a breaking change beyond adding VirtualTrees.Types to the uses clause:

• Move enum TSmartAutoFitType from unit VirtualTrees.Header would fix the first one and seems more consistent.
• Accessing TVirtualTreeColumns.Header.Treeview.GetCurrentPPI no longer compiles. Merging 'VirtualTrees.Columns in unit VirtualTrees.Header would allow strong typing of the Header property.

[vincentparrett]

I have pushed the change for the first one.

Merging columns and headers seems wrong to me.. slowly heading back to one large file. Surely you can Type Cast where needed - or do what I did in the implementation section, create a helper for it - see TVirtualTreeColumnHelper in the columns unit.

[joachimmarder]

create a helper for it

Copying around such helpers violates the DRY principle and so should not be part of a solution.

slowly heading back to one large file

Both units have an interface section of roughly 250 lines. I don't think merging them would create a too large file. Having this split in two units does not create any benefit I can see.

Delphi with its single pass compiler is not a good match with having super-small units, that produces large unit sections and/or the need of defining aliases all along. One needs to find a smooth point.

[vincentparrett]

I have merged columns into header and reverted the columns.header property type to TVTHeader.

[joachimmarder]

Thanks @vincentparrett for all the work, the pull request is now merged and breaking chnages are documented here: https://github.com/JAM-Software/Virtual-TreeView/wiki/Breaking-Changes-in-upcoming-V8