View Issue Details

IDProjectCategoryView StatusLast Update
0035705LazarusTAChartpublic2019-06-19 22:12
ReporterMarcin WiazowskiAssigned Towp 
PrioritynormalSeverityminorReproducibilityalways
Status closedResolutionfixed 
Product Version2.1 (SVN)Product Build61359 
Target VersionFixed in Version 
Summary0035705: TAChart: added lacking protection for TCustomColorMapSeries color source
DescriptionRecently, some protection was added for series' built-in sources: XCount / YCount can't be set below the required minimum values.

There is still one series family, that hasn't this protection implemented: TCustomColorMapSeries. This is because:
- it has other inheritance path, than all the other series having source,
- the source is unusual in that sense, that it provides colors, and is assigned by ColorSource property (not just Source).

Since ColorSource uses extent calculation for internal color transformation, it's important to avoid decreasing its XCount to 0 - because XCount = 0 case has now some special extent calculation algorithm, which is not valid for color source.



The attached patch adds the lacking protection, mainly by copying lacking functions from TChartSeries:
- TChartSeries.CheckSource() -> TCustomColorMapSeries.CheckColorSource()
- TChartSeries.SourceChanged() -> TCustomColorMapSeries.ColorSourceChanged()
- TChartSeries.GetXYCountNeeded() -> TCustomColorMapSeries.GetXYCountNeeded()



Other changes are:

1) built-in color source (i.e. FBuiltinColorSource) is now declared as TListChartSource - typecasting in TCustomColorMapSeries.BuildPalette() is no longer needed (sorry for not providing a separate patch for this trivial change)

2) In TCustomColorMapSeries.BuildPalette(), all the code is located between BeginUpdate() and EndUpdate() again: I moved some of the code below the EndUpdate() call some time ago and this was a mistake - this is because:

    for i:=0 to Count-1 do
      Item[i]^.X := (Item[i]^.X - ex.a.x) * factor + cmin;

also modifies the source.

3) In TCustomColorMapSeries.BuildPalette(), UpdateColorExtent() is no longer called - all source changes are now handled by TCustomColorMapSeries.ColorSourceChanged() handler (where the UpdateColorExtent() call is made).

4) In TCustomColorMapSeries.Create():

- listener for color source changes uses now a dedicated TCustomColorMapSeries.ColorSourceChanged() method

- built-in color source is now created as TBuiltinListChartSource

- built-in color source has now XCount and YCount manually assigned, which decreases YCount to 0 (thus making extent calculation faster)

5) In TCustomColorMapSeries.Destroy(), listener is freed before the built-in color source, to avoid SIGSEV

6) As already mentioned, all color source changes are now handled by TCustomColorMapSeries.ColorSourceChanged() - so UpdateParentChart() calls are no longer needed after BuildPalette() calls in TCustomColorMapSeries.SetBuiltinPalette() / SetPaletteMax() / SetPaletteMin()

7) TCustomColorMapSeries.SetColorSource() was adjusted a bit, to follow current TChartSeries.SetSource() implementation:

- removed UpdateColorExtent() is now called in TCustomColorMapSeries.ColorSourceChanged()

- removed UpdateParentChart() is now replaced with StyleChanged() call in TCustomColorMapSeries.ColorSourceChanged()



By it's nature, the attached patch fixes also some other problem: TCustomColorMapSeries.ColorSourceChanged() handler calls now UpdateColorExtent() - thanks to this, changes in the external color source are no longer able to make the color extent out-of-sync with the source itself.
TagsNo tags attached.
Fixed in Revision61409
LazTarget2.2
WidgetsetWin32/Win64
Attached Files
  • patch.diff (6,515 bytes)
    Index: components/tachart/tafuncseries.pas
    ===================================================================
    --- components/tachart/tafuncseries.pas	(revision 61359)
    +++ components/tachart/tafuncseries.pas	(working copy)
    @@ -18,7 +18,7 @@
     
     uses
       Classes, Graphics, typ, Types,
    -  TAChartUtils, TACustomFuncSeries, TACustomSeries, TACustomSource,
    +  TAChartUtils, TACustomFuncSeries, TACustomSeries, TACustomSource, TASources,
       TADrawUtils, TAFitUtils, TALegend, TATypes, TAFitLib, TAStyles;
     
     const
    @@ -417,7 +417,7 @@
         FStepY: TFuncSeriesStep;
         FUseImage: TUseImage;
         FColorExtentMin, FColorExtentMax: Double;
    -    FBuiltinColorSource: TCustomChartSource;
    +    FBuiltinColorSource: TListChartSource;
         FBuiltinPalette: TColormapPalette;
         FPaletteMax: Double;
         FPaletteMin: Double;
    @@ -437,9 +437,12 @@
       protected
         FMinZ, FMaxZ: Double;
         procedure BuildPalette(APalette: TColorMapPalette);
    +    procedure CheckColorSource(ASource: TCustomChartSource);
    +    procedure ColorSourceChanged(ASender: TObject); virtual;
         procedure GetLegendItems(AItems: TChartLegendItems); override;
         procedure GetZRange(ARect: TRect; dx, dy: Integer);
         procedure UpdateColorExtent;
    +    class procedure GetXYCountNeeded(out AXCount, AYCount: Cardinal); virtual;
     
       public
         procedure Assign(ASource: TPersistent); override;
    @@ -498,7 +501,7 @@
     uses
       {$IF FPC_FullVersion >= 30101}ipf{$ELSE}ipf_fix{$ENDIF},
       GraphType, GraphUtil, IntfGraphics, Math, spe, StrUtils, SysUtils,
    -  TAChartStrConsts, TAGeometry, TAGraph, TAMath, TASources;
    +  TAChartStrConsts, TAGeometry, TAGraph, TAMath;
     
     const
       DEF_PARAM_MIN = 0.0;
    @@ -2374,7 +2377,7 @@
       cmax, cmin, factor: Double;
       ex: TDoubleRect;
     begin
    -  with FBuiltinColorSource as TListChartSource do begin
    +  with FBuiltinColorSource do begin
         BeginUpdate;
         try
           Clear;
    @@ -2417,32 +2420,40 @@
           else
             raise EChartError.CreateFmt('[%s.BuildPalette] Palette not supported', [NameOrClassName(Self)]);
           end;
    +
    +      if FPaletteMin < FPaletteMax then begin
    +        cmin := FPaletteMin;
    +        cmax := FPaletteMax;
    +       end else
    +      if FPaletteMax < FPaletteMin then begin
    +        cmin := FPaletteMax;
    +        cmax := FPaletteMin;
    +      end else
    +        exit;
    +
    +      ex := Extent;
    +      if (ex.a.x = ex.b.x) then
    +        exit;
    +      factor := (cmax - cmin) / (ex.b.x - ex.a.x);
    +      for i:=0 to Count-1 do
    +        Item[i]^.X := (Item[i]^.X - ex.a.x) * factor + cmin;
         finally
           EndUpdate;
         end;
       end;
    +end;
     
    -  if FPaletteMin < FPaletteMax then begin
    -    cmin := FPaletteMin;
    -    cmax := FPaletteMax;
    -   end else
    -  if FPaletteMax < FPaletteMin then begin
    -    cmin := FPaletteMax;
    -    cmax := FPaletteMin;
    -  end else
    +procedure TCustomColorMapSeries.CheckColorSource(ASource: TCustomChartSource);
    +var
    +  nx, ny: Cardinal;
    +begin
    +  if ASource = nil then
         exit;
    -
    -  with FBuiltInColorSource do begin
    -    ex := Extent;
    -    if (ex.a.x = ex.b.x) then
    -      exit;
    -    factor := (cmax - cmin) / (ex.b.x - ex.a.x);
    -    for i:=0 to Count-1 do
    -      Item[i]^.X := (Item[i]^.X - ex.a.x) * factor + cmin;
    -  end;
    -
    -  if FColorSource = nil then
    -    UpdateColorExtent;
    +  GetXYCountNeeded(nx, ny);
    +  if ASource.XCount < nx then
    +    raise EXCountError.CreateFmt(rsSourceCountError, [ClassName, nx, 'x']);
    +  if ASource.YCount < ny then
    +    raise EYCountError.CreateFmt(rsSourceCountError, [ClassName, ny, 'y']);
     end;
     
     function TCustomColorMapSeries.ColorByValue(AValue: Double): TColor;
    @@ -2474,10 +2485,15 @@
     constructor TCustomColorMapSeries.Create(AOwner: TComponent);
     const
       BUILTIN_SOURCE_NAME = 'BuiltinColors';
    +var
    +  nx, ny: Cardinal;
     begin
       inherited Create(AOwner);
    -  FColorSourceListener := TListener.Create(@FColorSource, @StyleChanged);
    -  FBuiltinColorSource := TListChartSource.Create(self);
    +  FColorSourceListener := TListener.Create(@FColorSource, @ColorSourceChanged);
    +  GetXYCountNeeded(nx, ny);
    +  FBuiltinColorSource := TBuiltinListChartSource.Create(self, nx, ny);
    +  FBuiltinColorSource.XCount := nx;
    +  FBuiltinColorSource.YCount := ny;
       FBuiltinColorSource.Name := BUILTIN_SOURCE_NAME;
       FBuiltinColorSource.Broadcaster.Subscribe(FColorSourceListener);
       FBrush := TBrush.Create;
    @@ -2489,9 +2505,9 @@
     
     destructor TCustomColorMapSeries.Destroy;
     begin
    +  FreeAndNil(FColorSourceListener);
    +  FreeAndNil(FBuiltinColorSource);
       FreeAndNil(FBrush);
    -  FreeAndNil(FBuiltinColorSource);
    -  FreeAndNil(FColorSourceListener);
       inherited Destroy;
     end;
     
    @@ -2679,6 +2695,12 @@
       end;
     end;
     
    +class procedure TCustomColorMapSeries.GetXYCountNeeded(out AXCount, AYCount: Cardinal);
    +begin
    +  AXCount := 1;
    +  AYCount := 0;
    +end;
    +
     procedure TCustomColorMapSeries.GetZRange(ARect: TRect; dx, dy: Integer);
     var
       gp: TDoublePoint;
    @@ -2747,18 +2769,20 @@
     begin
       FBuiltinPalette := AValue;
       BuildPalette(FBuiltinPalette);
    -  UpdateParentChart;
     end;
     
     procedure TCustomColorMapSeries.SetColorSource(AValue: TCustomChartSource);
     begin
    -  if FColorSource = AValue then exit;
    +  if AValue = FBuiltinColorSource then
    +    AValue := nil;
    +  if FColorSource = AValue then
    +    exit;
    +  CheckColorSource(AValue);
       if FColorSourceListener.IsListening then
         ColorSource.Broadcaster.Unsubscribe(FColorSourceListener);
       FColorSource := AValue;
       ColorSource.Broadcaster.Subscribe(FColorSourceListener);
    -  UpdateColorExtent;
    -  UpdateParentChart;
    +  ColorSourceChanged(Self);
     end;
     
     procedure TCustomColorMapSeries.SetInterpolate(AValue: Boolean);
    @@ -2773,7 +2797,6 @@
       if AValue = FPaletteMax then exit;
       FPaletteMax := AValue;
       BuildPalette(FBuiltinPalette);
    -  UpdateParentChart;
     end;
     
     procedure TCustomColorMapSeries.SetPaletteMin(AValue: Double);
    @@ -2781,7 +2804,6 @@
       if AValue = FPaletteMin then exit;
       FPaletteMin := AValue;
       BuildPalette(FBuiltinPalette);
    -  UpdateParentChart;
     end;
     
     procedure TCustomColorMapSeries.SetStepX(AValue: TFuncSeriesStep);
    @@ -2805,6 +2827,19 @@
       UpdateParentChart;
     end;
     
    +procedure TCustomColorMapSeries.ColorSourceChanged(ASender: TObject);
    +begin
    +  if (ASender <> FBuiltinColorSource) and (ASender is TCustomChartSource) then
    +    try
    +      CheckColorSource(TCustomChartSource(ASender));
    +    except
    +      ColorSource := nil; // revert to built-in source
    +      raise;
    +    end;
    +  UpdateColorExtent;
    +  StyleChanged(ASender);
    +end;
    +
     procedure TCustomColorMapSeries.UpdateColorExtent;
     var
       ext: TDoubleRect;
    
    patch.diff (6,515 bytes)

Relationships

related to 0035665 closedwp TAChart: deriving from TListChartSource may lead to infinite loop 

Activities

Marcin Wiazowski

2019-06-12 00:00

reporter  

patch.diff (6,515 bytes)
Index: components/tachart/tafuncseries.pas
===================================================================
--- components/tachart/tafuncseries.pas	(revision 61359)
+++ components/tachart/tafuncseries.pas	(working copy)
@@ -18,7 +18,7 @@
 
 uses
   Classes, Graphics, typ, Types,
-  TAChartUtils, TACustomFuncSeries, TACustomSeries, TACustomSource,
+  TAChartUtils, TACustomFuncSeries, TACustomSeries, TACustomSource, TASources,
   TADrawUtils, TAFitUtils, TALegend, TATypes, TAFitLib, TAStyles;
 
 const
@@ -417,7 +417,7 @@
     FStepY: TFuncSeriesStep;
     FUseImage: TUseImage;
     FColorExtentMin, FColorExtentMax: Double;
-    FBuiltinColorSource: TCustomChartSource;
+    FBuiltinColorSource: TListChartSource;
     FBuiltinPalette: TColormapPalette;
     FPaletteMax: Double;
     FPaletteMin: Double;
@@ -437,9 +437,12 @@
   protected
     FMinZ, FMaxZ: Double;
     procedure BuildPalette(APalette: TColorMapPalette);
+    procedure CheckColorSource(ASource: TCustomChartSource);
+    procedure ColorSourceChanged(ASender: TObject); virtual;
     procedure GetLegendItems(AItems: TChartLegendItems); override;
     procedure GetZRange(ARect: TRect; dx, dy: Integer);
     procedure UpdateColorExtent;
+    class procedure GetXYCountNeeded(out AXCount, AYCount: Cardinal); virtual;
 
   public
     procedure Assign(ASource: TPersistent); override;
@@ -498,7 +501,7 @@
 uses
   {$IF FPC_FullVersion >= 30101}ipf{$ELSE}ipf_fix{$ENDIF},
   GraphType, GraphUtil, IntfGraphics, Math, spe, StrUtils, SysUtils,
-  TAChartStrConsts, TAGeometry, TAGraph, TAMath, TASources;
+  TAChartStrConsts, TAGeometry, TAGraph, TAMath;
 
 const
   DEF_PARAM_MIN = 0.0;
@@ -2374,7 +2377,7 @@
   cmax, cmin, factor: Double;
   ex: TDoubleRect;
 begin
-  with FBuiltinColorSource as TListChartSource do begin
+  with FBuiltinColorSource do begin
     BeginUpdate;
     try
       Clear;
@@ -2417,32 +2420,40 @@
       else
         raise EChartError.CreateFmt('[%s.BuildPalette] Palette not supported', [NameOrClassName(Self)]);
       end;
+
+      if FPaletteMin < FPaletteMax then begin
+        cmin := FPaletteMin;
+        cmax := FPaletteMax;
+       end else
+      if FPaletteMax < FPaletteMin then begin
+        cmin := FPaletteMax;
+        cmax := FPaletteMin;
+      end else
+        exit;
+
+      ex := Extent;
+      if (ex.a.x = ex.b.x) then
+        exit;
+      factor := (cmax - cmin) / (ex.b.x - ex.a.x);
+      for i:=0 to Count-1 do
+        Item[i]^.X := (Item[i]^.X - ex.a.x) * factor + cmin;
     finally
       EndUpdate;
     end;
   end;
+end;
 
-  if FPaletteMin < FPaletteMax then begin
-    cmin := FPaletteMin;
-    cmax := FPaletteMax;
-   end else
-  if FPaletteMax < FPaletteMin then begin
-    cmin := FPaletteMax;
-    cmax := FPaletteMin;
-  end else
+procedure TCustomColorMapSeries.CheckColorSource(ASource: TCustomChartSource);
+var
+  nx, ny: Cardinal;
+begin
+  if ASource = nil then
     exit;
-
-  with FBuiltInColorSource do begin
-    ex := Extent;
-    if (ex.a.x = ex.b.x) then
-      exit;
-    factor := (cmax - cmin) / (ex.b.x - ex.a.x);
-    for i:=0 to Count-1 do
-      Item[i]^.X := (Item[i]^.X - ex.a.x) * factor + cmin;
-  end;
-
-  if FColorSource = nil then
-    UpdateColorExtent;
+  GetXYCountNeeded(nx, ny);
+  if ASource.XCount < nx then
+    raise EXCountError.CreateFmt(rsSourceCountError, [ClassName, nx, 'x']);
+  if ASource.YCount < ny then
+    raise EYCountError.CreateFmt(rsSourceCountError, [ClassName, ny, 'y']);
 end;
 
 function TCustomColorMapSeries.ColorByValue(AValue: Double): TColor;
@@ -2474,10 +2485,15 @@
 constructor TCustomColorMapSeries.Create(AOwner: TComponent);
 const
   BUILTIN_SOURCE_NAME = 'BuiltinColors';
+var
+  nx, ny: Cardinal;
 begin
   inherited Create(AOwner);
-  FColorSourceListener := TListener.Create(@FColorSource, @StyleChanged);
-  FBuiltinColorSource := TListChartSource.Create(self);
+  FColorSourceListener := TListener.Create(@FColorSource, @ColorSourceChanged);
+  GetXYCountNeeded(nx, ny);
+  FBuiltinColorSource := TBuiltinListChartSource.Create(self, nx, ny);
+  FBuiltinColorSource.XCount := nx;
+  FBuiltinColorSource.YCount := ny;
   FBuiltinColorSource.Name := BUILTIN_SOURCE_NAME;
   FBuiltinColorSource.Broadcaster.Subscribe(FColorSourceListener);
   FBrush := TBrush.Create;
@@ -2489,9 +2505,9 @@
 
 destructor TCustomColorMapSeries.Destroy;
 begin
+  FreeAndNil(FColorSourceListener);
+  FreeAndNil(FBuiltinColorSource);
   FreeAndNil(FBrush);
-  FreeAndNil(FBuiltinColorSource);
-  FreeAndNil(FColorSourceListener);
   inherited Destroy;
 end;
 
@@ -2679,6 +2695,12 @@
   end;
 end;
 
+class procedure TCustomColorMapSeries.GetXYCountNeeded(out AXCount, AYCount: Cardinal);
+begin
+  AXCount := 1;
+  AYCount := 0;
+end;
+
 procedure TCustomColorMapSeries.GetZRange(ARect: TRect; dx, dy: Integer);
 var
   gp: TDoublePoint;
@@ -2747,18 +2769,20 @@
 begin
   FBuiltinPalette := AValue;
   BuildPalette(FBuiltinPalette);
-  UpdateParentChart;
 end;
 
 procedure TCustomColorMapSeries.SetColorSource(AValue: TCustomChartSource);
 begin
-  if FColorSource = AValue then exit;
+  if AValue = FBuiltinColorSource then
+    AValue := nil;
+  if FColorSource = AValue then
+    exit;
+  CheckColorSource(AValue);
   if FColorSourceListener.IsListening then
     ColorSource.Broadcaster.Unsubscribe(FColorSourceListener);
   FColorSource := AValue;
   ColorSource.Broadcaster.Subscribe(FColorSourceListener);
-  UpdateColorExtent;
-  UpdateParentChart;
+  ColorSourceChanged(Self);
 end;
 
 procedure TCustomColorMapSeries.SetInterpolate(AValue: Boolean);
@@ -2773,7 +2797,6 @@
   if AValue = FPaletteMax then exit;
   FPaletteMax := AValue;
   BuildPalette(FBuiltinPalette);
-  UpdateParentChart;
 end;
 
 procedure TCustomColorMapSeries.SetPaletteMin(AValue: Double);
@@ -2781,7 +2804,6 @@
   if AValue = FPaletteMin then exit;
   FPaletteMin := AValue;
   BuildPalette(FBuiltinPalette);
-  UpdateParentChart;
 end;
 
 procedure TCustomColorMapSeries.SetStepX(AValue: TFuncSeriesStep);
@@ -2805,6 +2827,19 @@
   UpdateParentChart;
 end;
 
+procedure TCustomColorMapSeries.ColorSourceChanged(ASender: TObject);
+begin
+  if (ASender <> FBuiltinColorSource) and (ASender is TCustomChartSource) then
+    try
+      CheckColorSource(TCustomChartSource(ASender));
+    except
+      ColorSource := nil; // revert to built-in source
+      raise;
+    end;
+  UpdateColorExtent;
+  StyleChanged(ASender);
+end;
+
 procedure TCustomColorMapSeries.UpdateColorExtent;
 var
   ext: TDoubleRect;
patch.diff (6,515 bytes)

wp

2019-06-17 19:53

developer   ~0116763

Applied, thanks.

Marcin Wiazowski

2019-06-19 22:12

reporter   ~0116797

Thanks.

Issue History

Date Modified Username Field Change
2019-06-12 00:00 Marcin Wiazowski New Issue
2019-06-12 00:00 Marcin Wiazowski File Added: patch.diff
2019-06-12 14:19 wp Assigned To => wp
2019-06-12 14:19 wp Status new => assigned
2019-06-12 14:23 wp Relationship added related to 0035089
2019-06-12 14:24 wp Relationship deleted related to 0035089
2019-06-12 14:27 wp Relationship added related to 0035665
2019-06-17 19:53 wp Status assigned => resolved
2019-06-17 19:53 wp Resolution open => fixed
2019-06-17 19:53 wp Fixed in Revision => 61409
2019-06-17 19:53 wp LazTarget => 2.2
2019-06-17 19:53 wp Widgetset Win32/Win64 => Win32/Win64
2019-06-17 19:53 wp Note Added: 0116763
2019-06-19 22:12 Marcin Wiazowski Status resolved => closed
2019-06-19 22:12 Marcin Wiazowski Note Added: 0116797