Incorrect scaling of DefaultNodeHeight about virtual-treeview HOT 34 CLOSED

O(1) stuff also take time. There is no significant performance penalty in using Begin/EndUpdate (or am I getting this wrong?) . Delphi is already very slow when you move a form to a different monitor. The main form of PyScripter contains about 5 VTs. Saving 10 calls to UpdateScrollbars when you switch monitors would not harm. But it is not just UpdateScrollbars. For example TBaseVirtualTree.SetDefaultNodeHeight contains:

OK, convinced and changed in master. Let me know if you see any difference in your app, then I can merge it to the V7_stable branch.

from virtual-treeview.

I would also remove the IsDPIChange parameter from [...] AutoScale.

Agreed and changed in master.

from virtual-treeview.

toAutoChnageSale used to scale according to the font height before, so this is not entirely new. The height was only increased to a minimum width so that the text is not cut off. But the height was never scaled down if the control was moved to a monitor with a smaller scaling, which didn't make much sense. This is done now in V8.

Of course the name toAutoChangeSale is debatable, but changing a flag's name in Delphi is a pain regarding backwards compatibility.

from virtual-treeview.

Just to add to @joachimmarder comment. Previously if toAutoChnageSale was not set, DPI scaling would not take place. So this flag had a dual role (enabling DPI scaling and adjusting the DefaultNodeHeight). Disabling DPI scaling in a DPI aware application makes little sense. Now DPI scaling always happens in DPI aware applications.

from virtual-treeview.

unexpectedly breaking long time existing applications

I wouldn't say it breaks them, but, yes, it is a change in behavior, especially if one used a very large node height.

To offer a little more control over the auto adjusted node height, I am considering to use the TextMargin property value instead of the hard-coded 2 pixels (scaled) to add an additional padding in AutoScale().

from virtual-treeview.

(half above this line and half below the previous line).

Since I am adding TextMargin once, half of the value is (virtually) added above and half on the bottom. I tested this and it looks good in my opinion. It is indeed slightly higher than with the default value 18 for DefaultNodeHeight, but this look very "1999". We should instead think about slightly increasing the default value of 18, maybe to 20. I just extracted a constant for it, so that would be easy.

from virtual-treeview.

Update:
It turns out that the issue was a result of changing a font to the wrong size in a parent Window. So, in a way this was a false alarm. However my comments about ChangeScale and AutoScale still hold.

Here is my go at simplifying ChangeScale:

procedure TBaseVirtualTree.ChangeScale(M, D: Integer{$if CompilerVersion >= 31}; isDpiChange: Boolean{$ifend});
begin
  // Needs to be called before inherited.  Does nothing if M=D. 
  SetDefaultNodeHeight(MulDiv(FDefaultNodeHeight, M, D));
  // changes Fonts and FCurrentPPI.
  inherited ChangeScale(M, D{$if CompilerVersion >= 31}, isDpiChange{$ifend});

  if M = D then
    Exit;

  TVTHeaderCracker(FHeader).ChangeScale(M, D, {$if CompilerVersion >= 31}isDpiChange{$ELSE} M <> D{$ifend});
  Indent := MulDiv(Indent, M, D);
  FTextMargin := MulDiv(FTextMargin, M, D);
  FMargin := MulDiv(FMargin, M, D);
  FImagesMargin := MulDiv(FImagesMargin, M, D);
  // Scale utility images, #796
  if FCheckImageKind = ckSystemDefault then begin
    FreeAndNil(FCheckImages);
    if HandleAllocated then
      FCheckImages := CreateSystemImageSet(Self);
  end;
  UpdateHeaderRect();
  ScaleNodeHeights(M, D);

  PrepareBitmaps(True, False); // See issue #991
end;

In my testing with different monitor configurations, the above scales both the tree and the header correctly, but clearly more testing is needed.

Comments:

• There is no point in supporting DPI awareness in Delphi version earlier than Berlin (I would even say Rio). It just does not work. Users of such versions should be advised to develop DPI Unaware applications.
• ChangeScale with IsDPIChange set to False is only called internally to clear the ScalingFlags. There is no need to take action. See TControl.ChangeScale, TCustomForm.ScaleForCurrentDPI (comment at the end) and TWinControl.ScaleControls.
• I would also remove the IsDPIChange parameter from the Header ChangeScale method (does not make sense for a TPersistent) and AutoScale.
• If AutoScale changes the default node height, then the height of existing nodes should also change when node height is fixed. Otherwise you end up having nodes of different height. This could be done in SetDefaultNodeHeight. In that case ScaleNodeHeights(M, D) above should only be called if node height is variable.
• The scaling code above should probably be surrounded by Begin/EndUpdate.

If you agree with the above changes I could submit a PR.

from virtual-treeview.

I think it makes absolutely no sense to disable DPI scaling in a DPI aware application. Everything would look wrong. I think that toAutoChangeScale should just control the optimization of the NodeHeight. What do you think?

I agree. It was the way as it is now when I took over the project, and at least some developers using Virtual-Treeview wanted full control over scaling. But I am very open for changing this for the next version 8.0, but I would like to keep it that way in V7, whcih will only receive bugfixes.

from virtual-treeview.

There is not point in supporting DPI awareness in Delphi version earlier than Berlin (I would even say Rio). It just does not work.

In an environment where all monitors have the same scaling, but >100%, I think the code still does anything useful. I can't proof this at the moment, but I don't see any benefit in changing this.

I would also remove the IsDPIChange parameter from the Header ChangeScale method (does not make sense for a TPersistent) and AutoScale.

But the parameter is used, and I don't see that it does any harm. What is the exact benefit of this change? We can remove it from TVirtualTreeColumn.ChangeScale().

The scaling code above should probably be surrounded by Begin/EndUpdate.

The only that changes multiple nodes is in ScaleNodeHeights(), and this has calls to Begin/EndUpdat.

from virtual-treeview.

In an environment where all monitors have the same scaling, but >100%, I think the code still does anything useful. I can't proof this at the moment, but I don't see any benefit in changing this.

In a DPI unaware application DPI is always 96 and no scaling is required. If on the other hand users develop DPI aware applications with Delphi versions earlier than Berlin, good luck to them. The scaling of VT would be the least of their problems. But in any case. we can keep the scaling for versions earlier than Berlin. I have modified the code above accordingly.

But the parameter is used, and I don't see that it does any harm.

The point is that it shouldn't be used. Not a major issue, but rather a design principle. Calls to the ChangeScale of a TPersistent are not made by VCL but by the component when scaling is needed. With the suggested changes above the header ChangeScale or AutoSize would never be called when IsDPIChange if False or when M=D.

The only that changes multiple nodes is in ScaleNodeHeights(), and this has calls to Begin/EndUpdat.

You are changing node height, margins, header size etc. Don't each of these result in calls to UpdateScrollbars ? It would save multiple calls to UpdateScrollbars for once (see SetDefaultNodeHeight).

from virtual-treeview.

ChangeScale with IsDPIChange set to False is only called internally to clear the ScalingFlags. There is no need to take action.

Are you sure, even if M<>D?

If AutoScale changes the default node height, then the height of existing nodes should also change when node height is fixed.

This should already be the case.

This could be done in SetDefaultNodeHeight.

I don't think this breaking change is a good idea. OK, someone needs to call ScaleNodeHeights() now to achieve this, but vice versa there would be no good way to prevent this automatic scaling.

You are changing node height, margins, header size etc. Don't each of these result in calls to UpdateScrollbars ? It would save multiple calls to UpdateScrollbars for once (see SetDefaultNodeHeight).

I typically use Begin/EndUpdate only for code that has a complexity of O(n), not to surround code with a complexity of O(1). You are right about UpdateScrollbars(), it is called 3 times in this case, but as far as I know it does not run over all nodes. Under this premise, I don't expect any significant effect.

Please excuse that I am a bit reluctant to do changes here, but it took several iterations to get to the current code. I see a chance of breaking things here again, without getting a good value in return.

from virtual-treeview.

Are you sure, even if M<>D?

In recent versions when IsDpiChange=False then M=D. See TControl.ChangeScale, TCustomForm.ScaleForCurrentDPI (comment at the end) and TWinControl.ScaleControls. Note that TCustomForm.ChangeScale is not used at all (see https://quality.embarcadero.com/browse/RSP-41988). You can confirm using the debugger.

It would certainly occur in versions earlier than Berlin, before IsDPIChange was introduced. It would also occur if one explicitly calls TWinControl.ScaleBy. But Vcl does not use that method.

In any case I have removed the IsDPIChange check in the code above.

This should already be the case.

I think not if the change happens via AutoSize.

I typically use Begin/EndUpdate only for code that has a complexity of O(n), not to surround code with a complexity of O(1).

O(1) stuff also take time. There is no significant performance penalty in using Begin/EndUpdate (or am I getting this wrong?) . Delphi is already very slow when you move a form to a different monitor. The main form of PyScripter contains about 5 VTs. Saving 10 calls to UpdateScrollbars when you switch monitors would not harm. But it is not just UpdateScrollbars. For example TBaseVirtualTree.SetDefaultNodeHeight contains:

    if (FUpdateCount = 0) and HandleAllocated and not (csLoading in ComponentState) then
    begin
      ValidateCache;
      UpdateScrollBars(True);
      ScrollIntoView(FFocusedNode, toCenterScrollIntoView in FOptions.SelectionOptions, True);
      Invalidate;
    end;

You are saving calls to ValidateCache, which is slow. Note that AutoSize may change the default node size for a second time.

from virtual-treeview.

Please excuse that I am a bit reluctant to do changes here,

I fully understand! I also manage open source projects and I know very well what you mean.

from virtual-treeview.

Let me know if you see any difference in your app

I did some measurements and you save a few milliseconds. However the time it takes to move my form between monitors varies between 400-900 milliseconds!, so the difference unfortunately is imperceptible (as I was expecting - see https://en.delphipraxis.net/topic/4768-vcl-handling-of-dpi-changes-poor-performance).

By the way, I noticed:

procedure TVTHeader.FontChanged(Sender : TObject);
begin
  inherited;
  {$IF CompilerVersion < 31}
  AutoScale(false);
  {$IFEND}
end;

Is the conditional compilation intentional?

from virtual-treeview.

Is the conditional compilation intentional?

Well, I guess it was intentional, that does not mean I can explain it right now. ;-)

from virtual-treeview.

Good to see we converged a bit. Some of the remaining differences:

ScalingFlags

There are about 50 overrides of ChangeScale in the Vcl. None of these checks the ScalingFlags. The only units that use the ScalingFlags are Forms and Controls. The flag sfHeight is used to decide whether to scale the overall height of the control or the ClientHeigth. If M<>D the header and the rest of the stuff needs to be scaled. And the way it is now, it only affects the control during loading,

ScaleForPPI

See

procedure TControl.ScaleForPPI(NewPPI: Integer);
begin
  if not FIScaling and (NewPPI > 0) then
  begin
    FIScaling := True;
    try
      if FCurrentPPI = 0 then
        FCurrentPPI := GetDesignDpi;

      if NewPPI <> FCurrentPPI then
        ChangeScale(NewPPI, FCurrentPPI, True);
    finally
      FIScaling := False;
    end;
  end;
end;

As you can see it either does nothing (if FIScaling is True) or calls ChangeScale (in this case recursively). Not a good idea in either case. Fortunately it does nothing.

I think the idea was to set the FCurrentPPI early. Instead move the inherited call at the top to set the Font and the FCurrentPPI right. Initially, the inherited call was at the end, and was moved up a bit.

from virtual-treeview.

Out of curiosity, I was looking into the role of scaling flags, and why the current code works. sfHeight is set by TControl.SetHeight and since Height is a stored property, it is always read from the dfm. So sfHeight would always be set at loading time.

But the question remained for what kind of controls sfHeight might not be set. The answer relates to embedded sub-controls.
This is an example:

constructor TSpTBXCustomTabControl.Create(AOwner: TComponent);
begin
  inherited;
  FPages := TList.Create;
  FEmptyTabSheet := TSpTBXTabSheet.Create(Self);
  FEmptyTabSheet.Parent := Self;

The tabsheet is created and the TabControl is set as Parent without setting Left, Top, Width or Height (alClient alignment). So when ChangeScale is called on the TabSheet the scaling flags will be empty and the Left, Top, Width and Height will not be scaled.

In any case the scaling of the VT header should not be related to the scaling flags.

from virtual-treeview.

The way how sfHeight was obviously wrong, thanks for pointing this out. I guess this code origins from the early days of multi monitor scaling. I removed the check for sfHeight and merged this change to V7_stable.

Are there any other changes in your code that you think are important or have a true positive impact? Especially moving up the inherited call?

from virtual-treeview.

Are there any other changes in your code that you think are important or have a true positive impact? Especially moving up the inherited call?

Remaining issues:

a) Scaling Flags:

The following code is still in ChangeScale:

        // It is important to evaluate the TScalingFlags before calling inherited, becuase they are differetn afterwards!
        if csLoading in ComponentState then
          Flags := ScalingFlags
        else
          Flags := DefaultScalingFlags; // Important for #677

This code was important when you were checking for scaling flags. With the check of scaling flags gone, this code is not needed. The local variable Flags is not used anywhere below!

b) ScaleForPPI

Please see the arguments for removing above. ChangeScale is called by ScaleforPPI. Calling it from ChangeScale makes no sense.

c) Remove IsPPIChange from the header ChangeScale parameters

Although less important, it would improve clarity and simplicity. When this routine is called it will scale the header. Always. And you only call it when M <> D.

d) Position of the inherited call

It will work as long as it is before the PrepareBitmaps and AutoScale calls, but also after the scaling of the DefaultNodeHeight. This last thing originally escaped me. The inherited call will change the font size if ParentFont is False. This will result in a call to AutoScale, which may change DefaultNodeSize if say the font size has increased. If the scaling of the DefaultNodeSize was to follow, it would result in double scaling of DefaultNodeSize. Please note also that when ParentFont is True (the common case), then the font change will occur after ChangeScale exits and would work correctly independently of the position of the inherited call.

To sum up the inherited call is OK where it is now!. Alternatively see the updated ChangeScale function at the top.

e) Linking scaling to toAutoChangeScale

I think we agreed that toAutoChangeScale should only affect the AutoScale node height optimization and would not be linked to DPI scaling.

f) Potential node height inconsistency if you change default node height with AutoScale

This is not directly related to ChangeScale. It could happen if

• toVariableNodeHeight is not set
• you have some nodes
• ChangeScale is called which call AutoScale
• AutoScale changes the DefaultNodeSize
• add some new nodes.

It could also happen if:

• toVariableNodeHeight is not set
• you have some nodes
• change the font
• AutoScale changes the DefaultNodeSize
• add some new nodes

I would address these issues by changing the node height of all existing nodes when the default node size changes, if toVariableNodeHeight is not set.

g) Calling AutoScale from ChangeScale

AutoScale is called when the font changes (but not when loading - see CMFontChanged method). So when ParentFont is False, ChangeScale will change the font and AutoSize will be called indirectly. When ParentFont is True, then AutoScale will be called after a DPI change when the parent font is scaled. This will occur after ChangeScale exits.
I am not sure what the point is in calling AutoSize from ChangeScale (when M=D as it was recently changed). When M=D the font will not be changed and there is no need to call AutoSize. It is worth considering removing that call. Also, It may be worth calling AutoSize from the Loaded method, to ensure it is called at least once, whether the application is DPI aware or not.

from virtual-treeview.

f) Potential node height inconsistency if you change default node height with AutoScale

This is not directly related to ChangeScale. It could happen if
- 
-     toVariableNodeHeight is not set
-     you have some nodes
-     ChangeScale is called which call AutoScale
-     AutoScale changes the DefaultNodeSize
-     add some new nodes.

After applying changes to f), this cannot happen any more.

DefaultNodeSize is the default size of new nodes, and I don't think it is a good idea if the setter changes existing nodes. But I see your point and think AutoScale() should, just like ChangeScale() already does, scale the existing nodes. It should do the same with the header height.

from virtual-treeview.

I'm not sure if I should open a new issue or comment here, but I've ran into a problem that seems to be caused by these modifications.

I have updated from version 7.6.4 to 8.0.1 and at the same time from Delphi 11 to Delphi 12.
Without any changes to my project, I now see an incorrect scaling of the node heights when I move the application between monitors with different DPI, which was not a problem before.

My main monitor is at 100% and DefaultNodeHeight is set to 24 in the IDE Object Inspector. I am not adjusting it anywhere in the code. If I now move the window to a 200% screen, the height changes to 31 (I checked it with vst.RootNode.NodeHeight) and looks to small (it should have changed to 48 obviously). Then I move the window back to the 100% screen and it changes to 15! While the calculation was correct this time, the height is now even too small on the initial screen.

Again, this problem was not present in 7.6.4 with Delphi 11. It scaled absolutely without any problems there, which I can easily verify with all previous binaries of my application.

from virtual-treeview.

@IgitBuh What has changed is how toAutoChangeScale works:

toAutoChangeScale,               // Change default node height and header height automatically according to the used font.

If you want you DeafaultNodeHeight to be 24 px at 96 PPI and scale accordingly for different PPI then just remove toAutoChangeScale from the tree options.

from virtual-treeview.

@pyscripter First of all, thank you very much for your comment as it indeed solves my issue.
I have now read through #1206 and the breaking changes (which I already did before, but didn't realize this is related to this issue).

Frankly, this is an extremely confusing and unnecessarily problematic decision. Adjusting the height based on the font should not be called "AutoChangeScale", but something like "AutoFontHeight". The name of this option results from what it was made for and what people expect it to be: automatically adjust on changed (dpi-)scaling.

This is even more confusing, because it seems to have no effect at the initial launch. It only affects the height as soon as a DPI change occurs (see my previous description of the issue). For this reason I ended up in this issue here.

If this is indeed a final decision to use toAutoChangeScale in this new unexpected way, then the current behaviour is a bug, because it is not applied at program start, but only when the DPI is changed.

from virtual-treeview.

@IgitBuh

it indeed solves my issue

Glad it did.

Agree that the name does not reflect well its function. "AutoFontHeight" does not either. Something like "AdjustDefalutNodeHeightAndHeaderHeightOnFontChange" would more accurately reflect its function, but this is way too long. In any case the precise role of this option is accurately documented in the source code.

As to whether the node height change is also applied when you load a form, I also agree with you.

TBaseVirtualTree.CMFontChanged contains the following code:

  if not (csLoading in ComponentState) then
  begin
    if HandleAllocated then begin
      AutoScale();
      Invalidate;
    end
  end;

If you remove the outer "if", then the DefaultNodeHeight would be adjusted when you load a form containing at VT. Not sure why this is not done at loading time.

from virtual-treeview.

In any case the precise role of this option is accurately documented in the source code.

I'm sorry to disagree, but it is not:
Change default node height and header height automatically according to the used font.

It does not say "adjust to the font height".
Instead, the wording says that if you change the font, the node height should scale accordingly. Which absolutely makes sense.

So lets say, I have font settings resulting in a text height of 10px and I have set DefaultNodeHeight to 15px. If I now at runtime modify the font and it results in a height of 20px, then this option should scale accordingly, i.e. either absolutely to 25px or relatively to 30px (debatable). If there was an intended padding, then no one would ever want to suddenly change this to exactly the font height. Again, I understand that there are usecases for such an option but it should be set consciously with a suitable name and not by a breaking change of a default option that nobody ever touched. The effect isn't even apparent after compiling the application, but only to people who test with different DPI screens.

from virtual-treeview.

toAutoChangeScale has nothing to do with font scaling. It does not change the font size/height.
Font scaling is done by VCL in DPI aware applications. Fonts will scale also when toAutoChangeScale is not set.

toAutoChangeScale does exactly what the documentation says: "change default node height and header height automatically according to the used font". So when a font is DPI scaled (therefore changed) and the option is set, it will try to "optimize" the DefaultNodeHeight according to the new font height. If it is not set the DefaultNodeHeight will be scaled directly.

from virtual-treeview.

All I'm saying is that this breaking change is unnecessarily and unexpectedly breaking long time existing applications without developers even realizing it, because the deliberately configured DefaultNodeHeight will be overruled by default as soon as a DPI change occurs (and only then). This doesn't make sense. At the very least it should be applied at design time and at program start as well.

Even then, allowing to set DefaultNodeHeight when it is going to be ignored/changed anyway by a default option, is confusing. I don't know if it is possible, but if you really want to use toAutoChangeSale that way, then DefaultNodeHeight should be set to 0 or -1 by default and if it is changed then toAutoChangeSale should be automatically set to False.

from virtual-treeview.

DefaultNodeHeight will be overruled by default as soon as a DPI change occurs (and only then).

If you do not want VirtualTrees to "optimize" (or mess up with, if you prefer,) DefaultNodeHide according to the font remove this option.

At the very least it should be applied at program start as well.

I would agree with that, See above.

from virtual-treeview.

Not sure why this is not done at loading time.

I guess this wasn't done to avoid double scaling when this was done differently. I will chnage that.

At the very least it should be applied at program start as well.

Agreed.

from virtual-treeview.

If you do not want VirtualTrees to "optimize" (or mess up with, if you prefer,) DefaultNodeHide according to the font remove this option.

I already did, thank you. But I'm talking about all the other software that is using VTV and where developers won't notice this.

Why did you both change my sentence while quoting it? The quote is silently missing the first part "at design time and at program start".

from virtual-treeview.

Why did you both change my sentence while quoting it?

I just kept the part I agree. Sorry...

from virtual-treeview.

Why did you both change my sentence while quoting it? T

Sorry, I just copied the quote.

from virtual-treeview.

OK, changes are committed, let me know what you think.

from virtual-treeview.

@joachimmarder
Looks good to me.

Maybe

lTextHeight := Canvas.TextHeight('Tg') + TextMargin;

should be

lTextHeight := Canvas.TextHeight('Tg') + TextMargin div 2;

This matches the current margin with default values and the distance between text lines would be equal to TextMargin (half above this line and half below the previous line).

from virtual-treeview.