Reviewing a patch for the chart component

The chart component in Lazarus was written two years ago by Philippe Martinole for his TeleAuto project. He submitted the component to the Lazarus Code and Component Repository. Later Luis Rodrigues extended it and made it more Delphi compatible. The component was considered stable and useful enough to add it to the default installation of Lazarus. Luis Rodrigues now is the main maintainer of the component.

A couple of weeks ago Luis submitted a patch for the chart component. Because I don't know the inner workings of the chart components and don't have a test program, I usually do two things:

• Do a visual inspection of the patch to see if it contains sensible things.
  
• Check if the patched version compiles on under windows, my main development OS.

People submitting these patch are major users of the component, so I assume they are better testers than I possibly can be.

This particular patch contained the following code to fix clipping:

      //set cliping region so we don't draw outsite
-     Rgn := CreateRectRgn(XImageMin, YImageMax, XImageMax, YImageMin);
+     p[0].x := XImageMin;
+     p[0].y := YImageMax;
+     p[1].x := XImageMax;
+     p[1].y := YImageMin;
+
+     {$IFDEF windows}
+     LPtoDP(Canvas.Handle, p, 2);
+     {$ENDIF}
+     Rgn := CreateRectRgn(p[0].x, p[0].y, p[1].x, p[1].y);
    SelectClipRgn (Canvas.Handle, Rgn);

I don't know how clipping works and what LPtoDP exactly does, but having a {$IFDEF} here in the middle of a procedure, doesn't look like good portable code. There seemed to be some inconstencies between the CreateRectRgn functions for win32 widgetset and the gtk widgetset: one expects logical points and the other device points.

Even if we wanted to use a {$IFDEF} the current one is probably not correct. The difference is not the OS, but the used widget set, so using {$IFDEF LCLwin32} would have been better. Remember, that the qt and the gtk2 widget can be compiled on windows too.

I asked Luis to find a better solution and maybe some people on the mailing list could help (unfortunately I had no clue how to fix this properly). Luis found another way without IFDEFS:

      //set cliping region so we don't draw outsite
-     Rgn := CreateRectRgn(XImageMin, YImageMax, XImageMax, YImageMin);
-     SelectClipRgn (Canvas.Handle, Rgn);
+     IntersectClipRect(Canvas.Handle, XImageMin, YImageMax, XImageMax, YImageMin);

This patch I happily applied after I tested the compilation.