Re: [PATCH] d2d1: Stub and add ID2D1PathGeometry1 interface to d2d1_1.idl.

[Nikolay Sivov]

On 5/12/22 03:02, James McDonnell wrote:
> Signed-off-by: James McDonnell
>
> dlls/d2d1/device.c   | 22 ++++++------
>   dlls/d2d1/factory.c  |  4 +--
>   dlls/d2d1/geometry.c | 80 +++++++++++++++++++++++++-------------------
>   include/d2d1_1.idl   | 17 +++++++++-
>   4 files changed, 75 insertions(+), 48 deletions(-)

Hi, James.

Thanks for the patch, a few comments.
> @@ -563,23 +563,23 @@ static void STDMETHODCALLTYPE d2d_device_context_DrawLine(ID2D1DeviceContext *if
>           D2D1_POINT_2F p0, D2D1_POINT_2F p1, ID2D1Brush *brush, float stroke_width, ID2D1StrokeStyle *stroke_style)
>   {
>       struct d2d_device_context *render_target = impl_from_ID2D1DeviceContext(iface);
> - ID2D1PathGeometry *geometry;
> -
>
> ID2D1PathGeometry1 *geometry;
>  ID2D1GeometrySink *sink;
>  HRESULT hr;
>
> TRACE("iface %p, p0 %s, p1 %s, brush %p, stroke_width %.8e, stroke_style %p.\n",
>          iface, debug_d2d_point_2f(&p0), debug_d2d_point_2f(&p1), brush, stroke_width, stroke_style);
> - if (FAILED(hr = ID2D1Factory_CreatePathGeometry(render_target->factory, &geometry)))
> - if (FAILED(hr = ID2D1Factory1_CreatePathGeometry(render_target->factory, &geometry)))
>  {
>      WARN("Failed to create path geometry, hr %#lx.\n", hr);
>      return;
>  }
> - if (FAILED(hr = ID2D1PathGeometry_Open(geometry, &sink)))
> - if (FAILED(hr = ID2D1PathGeometry1_Open(geometry, &sink)))
>  {
>      WARN("Failed to open geometry sink, hr %#lx.\n", hr);
> -
>    ID2D1PathGeometry_Release(geometry);
> -
>    ID2D1PathGeometry1_Release(geometry);
>     return;
> }
>
> @@ -591,7 +591,7 @@ static void STDMETHODCALLTYPE d2d_device_context_DrawLine(ID2D1DeviceContext *if
>       ID2D1GeometrySink_Release(sink);
>   ID2D1DeviceContext_DrawGeometry(iface, (ID2D1Geometry *)geometry, brush, stroke_width, stroke_style);
> - ID2D1PathGeometry_Release(geometry);
> - ID2D1PathGeometry1_Release(geometry);
> }

You don't need to update drawing parts to use newer interface. 
Additionally this will give warnings, because render_target->factory is 
ID2D1Factory and not ID2D1Factory1.
> static void STDMETHODCALLTYPE d2d_device_context_DrawRectangle(ID2D1DeviceContext *iface,
> @@ -1226,20 +1226,20 @@ static void d2d_device_context_draw_glyph_run_outline(struct d2d_device_context
>   {
>       D2D1_MATRIX_3X2_F *transform, prev_transform;
>       D2D1_ANTIALIAS_MODE prev_antialias_mode;
> - ID2D1PathGeometry *geometry;
> - ID2D1PathGeometry1 *geometry;
>  ID2D1GeometrySink *sink;
>  HRESULT hr;
> - if (FAILED(hr = ID2D1Factory_CreatePathGeometry(render_target->factory, &geometry)))
> - if (FAILED(hr = ID2D1Factory1_CreatePathGeometry(render_target->factory, &geometry)))
>  {
>      ERR("Failed to create geometry, hr %#lx.\n", hr);
>      return;
>  }
> - if (FAILED(hr = ID2D1PathGeometry_Open(geometry, &sink)))
> - if (FAILED(hr = ID2D1PathGeometry1_Open(geometry, &sink)))
>  {
>      ERR("Failed to open geometry sink, hr %#lx.\n", hr);
> -
>    ID2D1PathGeometry_Release(geometry);
> -
>    ID2D1PathGeometry1_Release(geometry);
>     return;
> }
>
> @@ -1249,7 +1249,7 @@ static void d2d_device_context_draw_glyph_run_outline(struct d2d_device_context
>       {
>           ERR("Failed to get glyph run outline, hr %#lx.\n", hr);
>           ID2D1GeometrySink_Release(sink);
> -
>    ID2D1PathGeometry_Release(geometry);
> -
>    ID2D1PathGeometry1_Release(geometry);
>     return;
> }
>
> @@ -1267,7 +1267,7 @@ static void d2d_device_context_draw_glyph_run_outline(struct d2d_device_context
>       render_target->drawing_state.antialiasMode = prev_antialias_mode;
>       *transform = prev_transform;
> - ID2D1PathGeometry_Release(geometry);
> - ID2D1PathGeometry1_Release(geometry);
> }

Same here.
> static void d2d_device_context_draw_glyph_run_bitmap(struct d2d_device_context *render_target,
> diff --git a/dlls/d2d1/factory.c b/dlls/d2d1/factory.c
> index f0f61674889..754975544e5 100644
> --- a/dlls/d2d1/factory.c
> +++ b/dlls/d2d1/factory.c
> @@ -521,9 +521,9 @@ static HRESULT STDMETHODCALLTYPE d2d_factory_CreateStrokeStyle1(ID2D1Factory3 *i
>
> static HRESULT STDMETHODCALLTYPE d2d_factory_CreatePathGeometry1(ID2D1Factory3 *iface, ID2D1PathGeometry1 **geometry)
>   {
> - FIXME("iface %p, geometry %p stub!\n", iface, geometry);
> - TRACE("iface %p, geometry %p.\n", iface, geometry);
> - return E_NOTIMPL;
> - return d2d_factory_CreatePathGeometry(iface, (ID2D1PathGeometry **)geometry);
> }

I think it's better to explicitly duplicate CreatePathGeometry here, 
this is how it's done for some existing methods.
> static HRESULT STDMETHODCALLTYPE d2d_factory_CreateDrawingStateBlock1(ID2D1Factory3 *iface,
> diff --git a/dlls/d2d1/geometry.c b/dlls/d2d1/geometry.c
> index eb1bb353b4c..bc032399fad 100644
> --- a/dlls/d2d1/geometry.c
> +++ b/dlls/d2d1/geometry.c
> @@ -3354,21 +3354,22 @@ static const struct ID2D1GeometrySinkVtbl d2d_geometry_sink_vtbl =
>       d2d_geometry_sink_AddArc,
>   };
>
> -static inline struct d2d_geometry *impl_from_ID2D1PathGeometry(ID2D1PathGeometry *iface)
> +static inline struct d2d_geometry *impl_from_ID2D1PathGeometry1(ID2D1PathGeometry1 *iface)
>   {
>       return CONTAINING_RECORD(iface, struct d2d_geometry, ID2D1Geometry_iface);
>   }
>
> -static HRESULT STDMETHODCALLTYPE d2d_path_geometry_QueryInterface(ID2D1PathGeometry *iface, REFIID iid, void **out)
> +static HRESULT STDMETHODCALLTYPE d2d_path_geometry_QueryInterface(ID2D1PathGeometry1 *iface, REFIID iid, void **out)
>   {
>       TRACE("iface %p, iid %s, out %p.\n", iface, debugstr_guid(iid), out);
>   if (IsEqualGUID(iid, &IID_ID2D1PathGeometry)
> -
>        || IsEqualGUID(iid, &IID_ID2D1PathGeometry1)
>         || IsEqualGUID(iid, &IID_ID2D1Geometry)
>         || IsEqualGUID(iid, &IID_ID2D1Resource)
>         || IsEqualGUID(iid, &IID_IUnknown))
> {
> -
>    ID2D1PathGeometry_AddRef(iface);
> -
>    ID2D1PathGeometry1_AddRef(iface);
>     *out = iface;
>     return S_OK;
> }

Please keep the order of IIDs, so (n+1) comes before (n).
> --- a/include/d2d1_1.idl
> +++ b/include/d2d1_1.idl
> @@ -20,7 +20,6 @@ import "d2d1.idl";
>   import "d2d1effects.idl";
>
> interface ID2D1DeviceContext;
> -interface ID2D1PathGeometry1;
>   interface ID2D1Properties;
>   interface IPrintDocumentPackageTarget;
>   interface ID2D1PrintControl;
> @@ -349,6 +348,22 @@ interface ID2D1GdiMetafile : ID2D1Resource
>       );
>   }
>
> +[
> - object,
> - uuid(62baa2d2-ab54-41b7-b872-787e0106a421),
> - local,
>
> +]
> +interface ID2D1PathGeometry1 : ID2D1PathGeometry
> +{
> - HRESULT ComputePointAndSegmentAtLength(
> -
>    [in] float length,
> -
>    [in] UINT32 startSegment,
> -
>    [in, optional] const D2D1_MATRIX_3X2_F *worldTransform,
> -
>    [in] float flatteningTolerance,
> -
>    [out] D2D1_POINT_DESCRIPTION *pointDescription
> - );
>
> +};
> -

Let's use consistent argument names, take a look at what d2d1.idl is 
using for same kind of arguments. Also you don't need closing comma for 
interface definition.