View Issue Details

IDProjectCategoryView StatusLast Update
0035390LazarusTAChartpublic2019-04-27 21:27
ReporterMarcin WiazowskiAssigned Towp 
PrioritynormalSeverityminorReproducibilityalways
Status closedResolutionfixed 
Product Version2.1 (SVN)Product Build60989 
Target Version2.2Fixed in Version 
Summary0035390: TAChart: small, but useful improvement for extent validation
DescriptionWhen I was thinking about solutions for 0035344, I realized, that it would be very useful to have a possibility to modify logical extent, when it is going to be set - in particular, when some third-party code modifies the logical extent arbitrarily, and we need - for example - to limit the extent in some way.

So I'm attaching a small patch, that adds OnExtentValidation event, which is called before any other actions in TChart.SetLogicalExtent().
TagsNo tags attached.
Fixed in Revision61017, 61021
LazTarget2.2
WidgetsetWin32/Win64
Attached Files
  • patch.diff (2,028 bytes)
    Index: components/tachart/tagraph.pas
    ===================================================================
    --- components/tachart/tagraph.pas	(revision 60989)
    +++ components/tachart/tagraph.pas	(working copy)
    @@ -167,6 +167,8 @@
         var ADoDefaultDrawing: Boolean) of object;
       TChartDrawEvent = procedure (
         ASender: TChart; ADrawer: IChartDrawer) of object;
    +  TChartExtentValidationEvent = procedure (
    +    ASender: TChart; var ALogicalExtent: TDoubleRect) of object;
     
       TChartRenderingParams = record
         FClipRect: TRect;
    @@ -232,6 +234,7 @@
         FOnAfterPaint: TChartEvent;
         FOnExtentChanged: TChartEvent;
         FOnExtentChanging: TChartEvent;
    +    FOnExtentValidation: TChartExtentValidationEvent;
         FPrevLogicalExtent: TDoubleRect;
         FScale: TDoublePoint;    // Coordinates transformation
         FScalingValid: Boolean;
    @@ -263,7 +266,7 @@
         procedure SetFrame(Value: TChartPen);
         procedure SetGUIConnector(AValue: TChartGUIConnector);
         procedure SetLegend(Value: TChartLegend);
    -    procedure SetLogicalExtent(const AValue: TDoubleRect);
    +    procedure SetLogicalExtent(AValue: TDoubleRect);
         procedure SetMargins(AValue: TChartMargins);
         procedure SetMarginsExternal(AValue: TChartMargins);
         procedure SetMinDataSpace(const AValue: Integer);
    @@ -454,6 +457,8 @@
           read FOnExtentChanged write FOnExtentChanged;
         property OnExtentChanging: TChartEvent
           read FOnExtentChanging write FOnExtentChanging;
    +    property OnExtentValidation: TChartExtentValidationEvent
    +      read FOnExtentValidation write FOnExtentValidation;
     
       published
         property Align;
    @@ -1675,10 +1680,12 @@
       StyleChanged(Self);
     end;
     
    -procedure TChart.SetLogicalExtent(const AValue: TDoubleRect);
    +procedure TChart.SetLogicalExtent(AValue: TDoubleRect);
     var
       w, h: Double;
     begin
    +  if Assigned(OnExtentValidation) then
    +    OnExtentValidation(Self, AValue);
       if FLogicalExtent = AValue then exit;
       w := Abs(AValue.a.X - AValue.b.X);
       h := Abs(AValue.a.Y - AValue.b.Y);
    
    patch.diff (2,028 bytes)
  • patch_ver2.diff (2,952 bytes)
    Index: components/tachart/tagraph.pas
    ===================================================================
    --- components/tachart/tagraph.pas	(revision 61004)
    +++ components/tachart/tagraph.pas	(working copy)
    @@ -167,6 +167,8 @@
         var ADoDefaultDrawing: Boolean) of object;
       TChartDrawEvent = procedure (
         ASender: TChart; ADrawer: IChartDrawer) of object;
    +  TChartExtentValidationEvent = procedure (
    +    ASender: TChart; var ALogicalExtent: TDoubleRect; var AllowChange: Boolean) of object;
     
       TChartRenderingParams = record
         FClipRect: TRect;
    @@ -213,6 +215,7 @@
         FOnDrawLegend: TChartDrawLegendEvent;
         FProportional: Boolean;
         FSeries: TChartSeriesList;
    +    FSetLogicalExtentCount: Integer;
         FTitle: TChartTitle;
         FToolset: TBasicChartToolset;
     
    @@ -232,6 +235,7 @@
         FOnAfterPaint: TChartEvent;
         FOnExtentChanged: TChartEvent;
         FOnExtentChanging: TChartEvent;
    +    FOnExtentValidation: TChartExtentValidationEvent;
         FPrevLogicalExtent: TDoubleRect;
         FScale: TDoublePoint;    // Coordinates transformation
         FScalingValid: Boolean;
    @@ -263,7 +267,7 @@
         procedure SetFrame(Value: TChartPen);
         procedure SetGUIConnector(AValue: TChartGUIConnector);
         procedure SetLegend(Value: TChartLegend);
    -    procedure SetLogicalExtent(const AValue: TDoubleRect);
    +    procedure SetLogicalExtent(AValue: TDoubleRect);
         procedure SetMargins(AValue: TChartMargins);
         procedure SetMarginsExternal(AValue: TChartMargins);
         procedure SetMinDataSpace(const AValue: Integer);
    @@ -454,6 +458,8 @@
           read FOnExtentChanged write FOnExtentChanged;
         property OnExtentChanging: TChartEvent
           read FOnExtentChanging write FOnExtentChanging;
    +    property OnExtentValidation: TChartExtentValidationEvent
    +      read FOnExtentValidation write FOnExtentValidation;
     
       published
         property Align;
    @@ -505,6 +511,7 @@
     
     const
       SScalingNotInitialized = '[%s.%s]: Image-graph scaling not yet initialized.';
    +  SNestedSetLogicalExtentCall = '%s: Don''t set LogicalExtent in OnExtentValidation handler - modify %s parameter instead.';
     
     function CompareZPosition(AItem1, AItem2: Pointer): Integer;
     begin
    @@ -1675,10 +1682,24 @@
       StyleChanged(Self);
     end;
     
    -procedure TChart.SetLogicalExtent(const AValue: TDoubleRect);
    +procedure TChart.SetLogicalExtent(AValue: TDoubleRect);
     var
    +  AllowChange: Boolean;
       w, h: Double;
     begin
    +  if FSetLogicalExtentCount <> 0 then
    +    raise EChartError.CreateFmt(SNestedSetLogicalExtentCall, [NameOrClassName(Self), 'ALogicalExtent']);
    +
    +  if Assigned(OnExtentValidation) then begin
    +    AllowChange := true;
    +    Inc(FSetLogicalExtentCount);
    +    try
    +      OnExtentValidation(Self, AValue, AllowChange);
    +    finally
    +      Dec(FSetLogicalExtentCount);
    +    end;
    +    if not AllowChange then exit;
    +  end;
       if FLogicalExtent = AValue then exit;
       w := Abs(AValue.a.X - AValue.b.X);
       h := Abs(AValue.a.Y - AValue.b.Y);
    
    patch_ver2.diff (2,952 bytes)

Activities

Marcin Wiazowski

2019-04-16 02:42

reporter  

patch.diff (2,028 bytes)
Index: components/tachart/tagraph.pas
===================================================================
--- components/tachart/tagraph.pas	(revision 60989)
+++ components/tachart/tagraph.pas	(working copy)
@@ -167,6 +167,8 @@
     var ADoDefaultDrawing: Boolean) of object;
   TChartDrawEvent = procedure (
     ASender: TChart; ADrawer: IChartDrawer) of object;
+  TChartExtentValidationEvent = procedure (
+    ASender: TChart; var ALogicalExtent: TDoubleRect) of object;
 
   TChartRenderingParams = record
     FClipRect: TRect;
@@ -232,6 +234,7 @@
     FOnAfterPaint: TChartEvent;
     FOnExtentChanged: TChartEvent;
     FOnExtentChanging: TChartEvent;
+    FOnExtentValidation: TChartExtentValidationEvent;
     FPrevLogicalExtent: TDoubleRect;
     FScale: TDoublePoint;    // Coordinates transformation
     FScalingValid: Boolean;
@@ -263,7 +266,7 @@
     procedure SetFrame(Value: TChartPen);
     procedure SetGUIConnector(AValue: TChartGUIConnector);
     procedure SetLegend(Value: TChartLegend);
-    procedure SetLogicalExtent(const AValue: TDoubleRect);
+    procedure SetLogicalExtent(AValue: TDoubleRect);
     procedure SetMargins(AValue: TChartMargins);
     procedure SetMarginsExternal(AValue: TChartMargins);
     procedure SetMinDataSpace(const AValue: Integer);
@@ -454,6 +457,8 @@
       read FOnExtentChanged write FOnExtentChanged;
     property OnExtentChanging: TChartEvent
       read FOnExtentChanging write FOnExtentChanging;
+    property OnExtentValidation: TChartExtentValidationEvent
+      read FOnExtentValidation write FOnExtentValidation;
 
   published
     property Align;
@@ -1675,10 +1680,12 @@
   StyleChanged(Self);
 end;
 
-procedure TChart.SetLogicalExtent(const AValue: TDoubleRect);
+procedure TChart.SetLogicalExtent(AValue: TDoubleRect);
 var
   w, h: Double;
 begin
+  if Assigned(OnExtentValidation) then
+    OnExtentValidation(Self, AValue);
   if FLogicalExtent = AValue then exit;
   w := Abs(AValue.a.X - AValue.b.X);
   h := Abs(AValue.a.Y - AValue.b.Y);
patch.diff (2,028 bytes)

wp

2019-04-16 19:36

developer   ~0115561

When the user sets Chart.LogicalExtent in this routine directly (instead of assigning something to ALogicalExtent) there's a perfect infinite loop.

The other problem that I have is that there is already an "OnExtentChanging" which, similar to TPageControl.OnChanging, I would expect to be thought for just the purpose of validatation. I am aware that the current OnExtentChanging does not work as good as your event. But why do we need another one? Or conversely, can I deprecate OnExtentChanging in favor of OnExtentValidation? What would be the disadvantages of doing so?

In an ExtentValidation event I would also expect a parameter "AllowChange" which would simply allow to keep the current extent value.

Marcin Wiazowski

2019-04-17 03:37

reporter   ~0115573

> When the user sets Chart.LogicalExtent in this routine directly (instead of assigning something to ALogicalExtent) there's a perfect infinite loop.

Sure - probably we could create an unlimited number of such loops by using various chart's events. And this is exactly the reason, why ALogicalExtent is passed as a var parameter - so we can do for example:

  procedure TForm1.Chart1ExtentValidation(ASender: TChart; var ALogicalExtent: TDoubleRect);
  begin
    with ASender.GetFullExtent do
      if ALogicalExtent.b.X > b.X then
        ALogicalExtent.b.X := b.X;
  end;

There is some important notice:
- OnExtentValidation is called BEFORE applying a new LOGICAL extent,
- OnExtentChanging and OnExtentChanged are called AFTER the LOGICAL extent has been already used to calculate CURRENT extent (i.e logical extent enlarged by margins for marks, etc.).



> The other problem that I have is that there is already an "OnExtentChanging" which, similar to TPageControl.OnChanging, I would expect to be thought for just the purpose of validatation.

Unfortunately, it's too late. Setting Chart.LogicalExtent in OnExtentChanging will do just nothing...



> I am aware that the current OnExtentChanging does not work as good as your event. But why do we need another one? Or conversely, can I deprecate OnExtentChanging in favor of OnExtentValidation? What would be the disadvantages of doing so?

I can't see any advantage of having current OnExtentChanging implementation. In fact, OnExtentChanging / OnExtentChanged should be rather named OnExtentChanged1 / OnExtentChanged2...


The only reference to OnExtentChanging in TAChart sources is in "tachart\tutorials\func_series\main.pas", where we can read:

"you can use the event Chart.OnExtentChanging for updating the domain exclusions"

Unfortunately, this sample code is currently misleading - domain exclusions may be needed to calculate series extent, which is then needed to calculate chart's current extent - and this is performed few lines of code BEFORE OnExtentChanging is called...


So it seems, that the original OnExtentChanging idea can be implemented by:
- removing current OnExtentChanging implementation,
- renaming OnExtentValidation event to OnExtentChanging

but - after the change - OnExtentChanging must have same parameters as OnExtentValidation currently has, i.e. (ASender: TChart; var ALogicalExtent: TDoubleRect).

This change will be incompatible, so nobody will be surprised - but nobody uses OnExtentChanging, because it's completely undocumented - only OnExtentChanged is documented, which, probably in all cases, can be used instead of OnExtentChanging.



> In an ExtentValidation event I would also expect a parameter "AllowChange" which would simply allow to keep the current extent value.

It doesn't seem to be needed - it's enough to not modify the passed ALogicalExtent, if it doesn't need to be modified...

TPageControl.OnChanging passes "var AllowChange: Boolean", because it wishes to ask us for a decision about allowing the change (in the control's state).

OnExtentChanging may pass "var ALogicalExtent: TDoubleRect", because it wishes to ask us for a decision about the extent...

Similarly, TForm.OnConstrainedResize passes "var MinWidth, MinHeight, MaxWidth, MaxHeight: TConstraintSize", to ask us about our decision in the matter of the form's size - we can modify these parameters, ore just leave them as they are...

wp

2019-04-17 10:43

developer   ~0115581

> probably we could create an unlimited number of such loops by using various chart's events.

I agree that there may be some (however not "unlimited") ways to do so, but it probably is nowhere as easy as here. Therefore I will not accept the patch in this form. You could implement a locking counter to detect whether an extent change has been requested until the current one is completed; when such a case happens you could either ignore the new request or raise an exception :

  var
    ExtentChangeCounter: Integer = 0;

  procedure TChart.SetLogicalExtent(AValue: TDoubleRect);
  var
    w, h: Double;
  begin
    if ExtentChangeCounter > 0 then exit; // or raise...
    try
      inc(ExtentChangeCounter);
      if Assigned(OnExtentValidation) then
        OnExtentValidation(Self, AValue);
      if FLogicalExtent = AValue then
        exit;
      w := Abs(AValue.a.X - AValue.b.X);
      h := Abs(AValue.a.Y - AValue.b.Y);
      with ExtentSizeLimit do
        if
          UseXMin and (w < XMin) or UseXMax and (w > XMax) or
          UseYMin and (h < YMin) or UseYMax and (h > YMax)
        then
          exit;
      FLogicalExtent := AValue;
      FIsZoomed := true;
      StyleChanged(Self);
    finally
      dec(ExtentChangeCounter);
    end;
  end;

----

> Setting Chart.LogicalExtent in OnExtentChanging will do just nothing

Wrong. The following code will effectively inhibit panning although the visual effect is far from perfect:

  procedure TForm1.Chart1ExtentChanging(ASender: TChart);
  begin
    Chart1.LogicalExtent := Chart1.GetFullExtent;
  end;
  
----

>> [...] a parameter "AllowChange" which would simply allow to keep the current extent value.
>
> It doesn't seem to be needed - it's enough to not modify the passed ALogicalExtent, if it
> doesn't need to be modified...

Suppose a scenario which has 4 edit controls for the 4 extent coordinates. The user types the values for a new extent and presses an Apply button which applies the new extent by trying to set Chart.LogicalExtent. When the new extent is not valid he expects the extent to remain unchanged, he wants to show a message box and prompt for a new set of input values. This is not possible with your event, it will always change the extent.

Marcin Wiazowski

2019-04-17 12:13

reporter   ~0115587

I can add reference counter, no problem. I think, that raising an exception is a good idea to clearly show, that setting Chart.LogicalExtent the handler is always a bug.


> The following code will effectively inhibit panning

Ok, this probably works in this way:
- we set LogicalExtent in the OnExtentChanging call,
- chart's execution path in TAChart.Draw() doesn't use this logical extent anymore - current extend is used instead, and logical extent is ignored,
- but we start panning then, so panning tool uses the LogicalExtent values set in the last OnExtentChanging call,
- but chart finished its drawing process - i.e. erased its "Invalidate" status - AFTER LogicalExtent was changed in the OnExtentChanging call, so this change in LogicalExtent doesn't (successfully) initiate new chart redrawing (as it is done under normal circumstances),
- and we ended up with logical extent desynchronized with current extent.

So setting logical extent in OnExtentChanging should also raise an exception... Or maybe OnExtentChanging is not in fact needed and should be removed?


> Suppose a scenario which has 4 edit controls

Now I get the idea:

  AllowChange := true;
  if Assigned(OnExtentValidation) then
    OnExtentValidation(Self, AValue, AllowChange);
  if not AllowChange then exit;

In this case name OnExtentValidation would be better than OnExtentChanging, I think.

wp

2019-04-17 16:51

developer   ~0115595

> I can add reference counter, no problem. I think, that raising an exception is a
> good idea to clearly show, that setting Chart.LogicalExtent the handler is
> always a bug.

Then go ahead. With "AllowChange", please.

Marcin Wiazowski

2019-04-18 00:58

reporter  

patch_ver2.diff (2,952 bytes)
Index: components/tachart/tagraph.pas
===================================================================
--- components/tachart/tagraph.pas	(revision 61004)
+++ components/tachart/tagraph.pas	(working copy)
@@ -167,6 +167,8 @@
     var ADoDefaultDrawing: Boolean) of object;
   TChartDrawEvent = procedure (
     ASender: TChart; ADrawer: IChartDrawer) of object;
+  TChartExtentValidationEvent = procedure (
+    ASender: TChart; var ALogicalExtent: TDoubleRect; var AllowChange: Boolean) of object;
 
   TChartRenderingParams = record
     FClipRect: TRect;
@@ -213,6 +215,7 @@
     FOnDrawLegend: TChartDrawLegendEvent;
     FProportional: Boolean;
     FSeries: TChartSeriesList;
+    FSetLogicalExtentCount: Integer;
     FTitle: TChartTitle;
     FToolset: TBasicChartToolset;
 
@@ -232,6 +235,7 @@
     FOnAfterPaint: TChartEvent;
     FOnExtentChanged: TChartEvent;
     FOnExtentChanging: TChartEvent;
+    FOnExtentValidation: TChartExtentValidationEvent;
     FPrevLogicalExtent: TDoubleRect;
     FScale: TDoublePoint;    // Coordinates transformation
     FScalingValid: Boolean;
@@ -263,7 +267,7 @@
     procedure SetFrame(Value: TChartPen);
     procedure SetGUIConnector(AValue: TChartGUIConnector);
     procedure SetLegend(Value: TChartLegend);
-    procedure SetLogicalExtent(const AValue: TDoubleRect);
+    procedure SetLogicalExtent(AValue: TDoubleRect);
     procedure SetMargins(AValue: TChartMargins);
     procedure SetMarginsExternal(AValue: TChartMargins);
     procedure SetMinDataSpace(const AValue: Integer);
@@ -454,6 +458,8 @@
       read FOnExtentChanged write FOnExtentChanged;
     property OnExtentChanging: TChartEvent
       read FOnExtentChanging write FOnExtentChanging;
+    property OnExtentValidation: TChartExtentValidationEvent
+      read FOnExtentValidation write FOnExtentValidation;
 
   published
     property Align;
@@ -505,6 +511,7 @@
 
 const
   SScalingNotInitialized = '[%s.%s]: Image-graph scaling not yet initialized.';
+  SNestedSetLogicalExtentCall = '%s: Don''t set LogicalExtent in OnExtentValidation handler - modify %s parameter instead.';
 
 function CompareZPosition(AItem1, AItem2: Pointer): Integer;
 begin
@@ -1675,10 +1682,24 @@
   StyleChanged(Self);
 end;
 
-procedure TChart.SetLogicalExtent(const AValue: TDoubleRect);
+procedure TChart.SetLogicalExtent(AValue: TDoubleRect);
 var
+  AllowChange: Boolean;
   w, h: Double;
 begin
+  if FSetLogicalExtentCount <> 0 then
+    raise EChartError.CreateFmt(SNestedSetLogicalExtentCall, [NameOrClassName(Self), 'ALogicalExtent']);
+
+  if Assigned(OnExtentValidation) then begin
+    AllowChange := true;
+    Inc(FSetLogicalExtentCount);
+    try
+      OnExtentValidation(Self, AValue, AllowChange);
+    finally
+      Dec(FSetLogicalExtentCount);
+    end;
+    if not AllowChange then exit;
+  end;
   if FLogicalExtent = AValue then exit;
   w := Abs(AValue.a.X - AValue.b.X);
   h := Abs(AValue.a.Y - AValue.b.Y);
patch_ver2.diff (2,952 bytes)

Marcin Wiazowski

2019-04-18 00:58

reporter   ~0115616

Attached.

I defined exception string directly in TAGraph unit (similarly to SScalingNotInitialized), because it's a design-phase exception, and the application's end-user will never see it - so, I think, there is no need to define this string as resourcestring, or to translate it.

Setting LogicalExtent in OnExtentValidation handler raises an exception - this leaves the chart in a unpainted state. Again, It's a design-phase error, so I can live with this unpainted chart...

wp

2019-04-18 16:12

developer   ~0115652

Applied with minor changes. Thanks.

Marcin Wiazowski

2019-04-19 05:05

reporter   ~0115667

According to current naming convention, the SNestedSetLogicalExtentCall message should be changed a bit: OnExtentValidation -> OnExtentValidate.


Maybe you could also take a look at tachart\tutorials\func_series\main.pas, where:
- there is a comment about OnExtentChanging (currently deprecated)
- Chart1AfterDrawBackWall() is used to update domain exclusions - probably OnExtentValidate should be used instead.


I'm leaving for few days, so I'll be back here probably in the middle of the next week - no new bug reports for few days :)

wp

2019-04-19 14:30

developer   ~0115673

Fixed.

Marcin Wiazowski

2019-04-27 21:27

reporter   ~0115857

Thanks!

Issue History

Date Modified Username Field Change
2019-04-16 02:42 Marcin Wiazowski New Issue
2019-04-16 02:42 Marcin Wiazowski File Added: patch.diff
2019-04-16 09:42 wp Assigned To => wp
2019-04-16 09:42 wp Status new => assigned
2019-04-16 19:36 wp Note Added: 0115561
2019-04-16 20:01 wp LazTarget => -
2019-04-16 20:01 wp Status assigned => feedback
2019-04-17 03:37 Marcin Wiazowski Note Added: 0115573
2019-04-17 03:37 Marcin Wiazowski Status feedback => assigned
2019-04-17 10:43 wp Note Added: 0115581
2019-04-17 12:13 Marcin Wiazowski Note Added: 0115587
2019-04-17 16:51 wp Note Added: 0115595
2019-04-18 00:58 Marcin Wiazowski File Added: patch_ver2.diff
2019-04-18 00:58 Marcin Wiazowski Note Added: 0115616
2019-04-18 16:12 wp Fixed in Revision => 61017
2019-04-18 16:12 wp LazTarget - => 2.2
2019-04-18 16:12 wp Note Added: 0115652
2019-04-18 16:12 wp Status assigned => resolved
2019-04-18 16:12 wp Resolution open => fixed
2019-04-18 16:12 wp Target Version => 2.2
2019-04-19 05:05 Marcin Wiazowski Note Added: 0115667
2019-04-19 05:05 Marcin Wiazowski Status resolved => assigned
2019-04-19 05:05 Marcin Wiazowski Resolution fixed => reopened
2019-04-19 14:30 wp Fixed in Revision 61017 => 61017, 61021
2019-04-19 14:30 wp Note Added: 0115673
2019-04-19 14:30 wp Status assigned => resolved
2019-04-19 14:30 wp Resolution reopened => fixed
2019-04-27 21:27 Marcin Wiazowski Status resolved => closed
2019-04-27 21:27 Marcin Wiazowski Note Added: 0115857