search

EtheaDev / SVGIconImageList

Three engines to render SVG (Delphi Image32, Skia4Delphi, Direct2D wrapper) and four components to simplify use of SVG images (resize, fixedcolor, grayscale...)
Apache License 2.0
321 stars 93 forks source link

Multiple invocations of "SVG" property "Get" accessor of TSVGIconBitmapItep #226

Closed birbilis closed 2 years ago

birbilis commented 2 years ago

The following code (from FMX.SVGIconImageList) seems to invoke the SVG property "get accessor" multiple times. I suppose the Delphi compiler isn't optimising such things. So why not use a local variable or a with statement? (in my opinion "with" IS useful, especially for such cases)

procedure TSVGIconBitmapItem.DrawSVGIcon;
var
  LBitmap: TBitmap;
  LBitmapWidth, LBitmapHeight: Integer;
begin
  LBitmap := inherited Bitmap;
  LBitmapWidth := Round(FWidth * Scale);
  LBitmapHeight := Round(FHeight * Scale);
  LBitmap.SetSize(LBitmapWidth, LBitmapHeight);
  SVG.Opacity := Opacity;
  SVG.FixedColor := FixedColor;
  SVG.Grayscale := GrayScale;
  PaintToBitmap(LBitmap, SVG, FZoom);
end;

maybe other code is similar too

luebbe commented 2 years ago

Not sure about using with, but I guess you saw this yourself ;)

procedure TSVGIconBitmapItem.DrawSVGIcon;
var
  LBitmap: TBitmap;
  LBitmapWidth, LBitmapHeight: Integer;
begin
  LBitmap := inherited Bitmap;
  LBitmapWidth := Round(FWidth * Scale);
  LBitmapHeight := Round(FHeight * Scale);
  LBitmap.SetSize(LBitmapWidth, LBitmapHeight);
  with SVG do
  begin
    Opacity := Opacity;
    FixedColor := FixedColor;
    Grayscale := GrayScale;
  end;
  PaintToBitmap(LBitmap, SVG, FZoom);
end;
carloBarazzetta commented 2 years ago

I don't love "with" keyword, I corrected the code using a local variable:

  LSVG := SVG;
  LSVG.Opacity := Opacity;
  LSVG.FixedColor := FixedColor;
  LSVG.Grayscale := GrayScale;
  PaintToBitmap(LBitmap, LSVG, FZoom);
birbilis commented 2 years ago

Not sure about using with, but I guess you saw this yourself ;)

procedure TSVGIconBitmapItem.DrawSVGIcon;
var
  LBitmap: TBitmap;
  LBitmapWidth, LBitmapHeight: Integer;
begin
  LBitmap := inherited Bitmap;
  LBitmapWidth := Round(FWidth * Scale);
  LBitmapHeight := Round(FHeight * Scale);
  LBitmap.SetSize(LBitmapWidth, LBitmapHeight);
  with SVG do
  begin
    Opacity := Opacity;
    FixedColor := FixedColor;
    Grayscale := GrayScale;
  end;
  PaintToBitmap(LBitmap, SVG, FZoom);
end;

yep, that's why I suggested https://quality.embarcadero.com/browse/RSP-36984 to Embarcadero

would then have .Opacity = Opacity etc. as long as one had used compiler define in that unit/code area to say they didn't want the old "unsafe" with behaviour (the one that's falling back to the outer [of any nested withs] context). Original with wasn't designed for OOP obviously, was more for structure programming