[PATCH v2 0/1] MR3581: gdiplus: add support for GdipPathIterNextPathType

[Bartosz Kosiorek (＠gang65)]

I have covered implementation with additional test cases.

--
  v2: gdiplus: add support for GdipPathIterNextPathType

https://gitlab.winehq.org/wine/wine/-/merge_requests/3581

[PATCH v2 1/1] gdiplus: add support for GdipPathIterNextPathType

[Bartosz Kosiorek]

From: Bartosz Kosiorek

---
 dlls/gdiplus/pathiterator.c       |  39 ++++++++-
 dlls/gdiplus/tests/pathiterator.c | 127 ++++++++++++++++++++++++------
 2 files changed, 141 insertions(+), 25 deletions(-)

diff --git a/dlls/gdiplus/pathiterator.c b/dlls/gdiplus/pathiterator.c
index 2addbc3bc57..72d7ad74b87 100644
--- a/dlls/gdiplus/pathiterator.c
+++ b/dlls/gdiplus/pathiterator.c
@@ -222,6 +222,8 @@ GpStatus WINGDIPAPI GdipPathIterNextSubpath(GpPathIterator* iterator,
     }

*startIndex = iterator->subpath_pos;
+    /* Set new pathtype position */
+    iterator->pathtype_pos = iterator->subpath_pos;

for(i = iterator->subpath_pos + 1; i < count &&
         !(iterator->pathdata.Types[i] == PathPointTypeStart); i++);
@@ -238,6 +240,7 @@ GpStatus WINGDIPAPI GdipPathIterNextSubpath(GpPathIterator* iterator,

return Ok;
 }
+// FIXME not returning anything
 GpStatus WINGDIPAPI GdipPathIterRewind(GpPathIterator *iterator)
 {
     TRACE("(%p)\n", iterator);
@@ -295,12 +298,42 @@ GpStatus WINGDIPAPI GdipPathIterIsValid(GpPathIterator* iterator, BOOL* valid)
 GpStatus WINGDIPAPI GdipPathIterNextPathType(GpPathIterator* iter, INT* result,
     BYTE* type, INT* start, INT* end)
 {
-    FIXME("(%p, %p, %p, %p, %p) stub\n", iter, result, type, start, end);
+    INT i, count, tempType;
+    TRACE("(%p, %p, %p, %p, %p)\n", iter, result, type, start, end);

-    if(!iter || !result || !type || !start || !end)
+    if (!iter || !result || !type || !start || !end)
         return InvalidParameter;

-    return NotImplemented;
+    count = iter->subpath_pos;
+    i = iter->pathtype_pos;
+    if (i >= count) {
+        *result = 0;
+        return Ok;
+    }
+
+    /* set starting point to first PathPointTypeStart and */
+    if ((iter->pathdata.Types[i] & PathPointTypePathTypeMask) == PathPointTypeStart) {
+        *start = i;
+        i++;
+        *end = i;
+        *type = iter->pathdata.Types[i] & PathPointTypePathTypeMask;
+        i++;
+    }
+    else
+        return Ok;
+
+    if (iter->pathtype_pos < count) {
+        for ( ; i < count; i++) {
+            tempType = iter->pathdata.Types[i] & PathPointTypePathTypeMask;
+            if ((tempType != *type) || (tempType == PathPointTypeStart))
+                break;
+        }
+        *end = i - 1;
+        iter->pathtype_pos = i;
+    }
+    *result = *end - *start + 1;
+
+    return Ok;
 }

GpStatus WINGDIPAPI GdipPathIterNextSubpathPath(GpPathIterator* iter, INT* result,
diff --git a/dlls/gdiplus/tests/pathiterator.c b/dlls/gdiplus/tests/pathiterator.c
index 9bd3ac5452e..64affafca8f 100644
--- a/dlls/gdiplus/tests/pathiterator.c
+++ b/dlls/gdiplus/tests/pathiterator.c
@@ -471,11 +471,32 @@ static void test_nextsubpath(void)
     GdipCreatePath(FillModeAlternate, &path);
     GdipCreatePathIter(&iter, path);

-    result = -2;
+    result = start = end = (INT)0xdeadbeef;
     closed = TRUE;
     stat = GdipPathIterNextSubpath(iter, &result, &start, &end, &closed);
     expect(Ok, stat);
     expect(0, result);
+    expect((INT)0xdeadbeef, start);
+    expect((INT)0xdeadbeef, end);
+    expect(TRUE, closed); /* Not changed */
+
+    /* four points figure */
+    GdipAddPathLine(path, 0.0, 0.0, 10.0, 30.0);
+    GdipAddPathLine(path, -10.0, 5.0, 15.0, 8.0);
+    GdipCreatePathIter(&iter, path);
+    stat = GdipPathIterNextSubpath(iter, &result, &start, &end, &closed);
+    expect(Ok, stat);
+    expect(4, result);
+    expect(0, start);
+    expect(3, end);
+    expect(FALSE, closed);
+
+    /* No more subpaths*/
+    stat = GdipPathIterNextSubpath(iter, &result, &start, &end, &closed);
+    expect(Ok, stat);
+    expect(0, result);
+    expect(0, start);
+    expect(0, end);
     expect(TRUE, closed);

GdipDeletePathIter(iter);
@@ -487,8 +508,9 @@ static void test_nextpathtype(void)
     GpPath *path;
     GpPathIterator *iter;
     GpStatus stat;
-    INT start, end, result;
+    INT count, start, end, result;
     BYTE type;
+    BOOL closed;

GdipCreatePath(FillModeAlternate, &path);
     GdipCreatePathIter(&iter, path);
@@ -512,49 +534,110 @@ static void test_nextpathtype(void)
     /* empty path */
     start = end = result = (INT)0xdeadbeef;
     stat = GdipPathIterNextPathType(iter, &result, &type, &start, &end);
-    todo_wine expect(Ok, stat);
+    expect(Ok, stat);
     expect((INT)0xdeadbeef, start);
     expect((INT)0xdeadbeef, end);
-    todo_wine expect(0, result);
+    expect(0, result);
     GdipDeletePathIter(iter);

-    /* single figure */
-    GdipAddPathLine(path, 0.0, 0.0, 10.0, 30.0);
+    /* figure with three subpaths */
+    stat = GdipAddPathLine(path, 0.0, 0.0, 10.0, 30.0);
+    expect(Ok, stat);
+    stat = GdipAddPathRectangle(path, 1.0, 2.0, 3.0, 4.0);
+    expect(Ok, stat);
+    stat = GdipAddPathEllipse(path, 0.0, 0.0, 35.0, 70.0);
+    expect(Ok, stat);
     GdipCreatePathIter(&iter, path);
+
+    /* When subPath is not set,
+       it is not possible to get Path Type */
+    result = -2;
     start = end = result = (INT)0xdeadbeef;
     type = 255; /* out of range */
     stat = GdipPathIterNextPathType(iter, &result, &type, &start, &end);
-    todo_wine expect(Ok, stat);
+    expect(Ok, stat);
     expect((INT)0xdeadbeef, start);
     expect((INT)0xdeadbeef, end);
     expect(255, type);
-    todo_wine expect(0, result);
-    GdipDeletePathIter(iter);
+    expect(0, result);

-    stat = GdipAddPathEllipse(path, 0.0, 0.0, 35.0, 70.0);
+    /* When subPath is not set,
+       it is possible to get number of path points */
+    stat = GdipPathIterGetCount(iter, &count);
     expect(Ok, stat);
-    GdipCreatePathIter(&iter, path);
+    expect(19, count);
+
+    /* Set subPath (Line) */
+    result = start = end = (INT)0xdeadbeef;
+    closed = TRUE;
+    stat = GdipPathIterNextSubpath(iter, &result, &start, &end, &closed);
+    expect(Ok, stat);
+    expect(2, result);
+    expect(0, start);
+    expect(1, end);
+    expect(FALSE, closed);
+
+    /* When subPath is set (Line), return position of points with the same type */
+    result = -2;
     start = end = result = (INT)0xdeadbeef;
     type = 255; /* out of range */
     stat = GdipPathIterNextPathType(iter, &result, &type, &start, &end);
-    todo_wine expect(Ok, stat);
-    expect((INT)0xdeadbeef, start);
-    expect((INT)0xdeadbeef, end);
-    expect(255, type);
-    todo_wine expect(0, result);
-    GdipDeletePathIter(iter);
+    expect(Ok, stat);
+    expect(0, start);
+    expect(1, end);
+    expect(PathPointTypeLine, type);
+    expect(2, result);

-    /* closed */
-    GdipClosePathFigure(path);
-    GdipCreatePathIter(&iter, path);
+    /* When NextSubpath is running twice, without invocation NextPathType
+       (skipping Rectangle figure), make sure that PathType index is updated */
+    result = start = end = (INT)0xdeadbeef;
+    closed = TRUE;
+    stat = GdipPathIterNextSubpath(iter, &result, &start, &end, &closed);
+    expect(Ok, stat);
+    expect(4, result);
+    expect(2, start);
+    expect(5, end);
+    expect(TRUE, closed);
+
+    /* Increment subPath (to Ellipse figure) */
+    result = start = end = (INT)0xdeadbeef;
+    closed = TRUE;
+    stat = GdipPathIterNextSubpath(iter, &result, &start, &end, &closed);
+    expect(Ok, stat);
+    expect(13, result);
+    expect(6, start);
+    expect(18, end);
+    expect(TRUE, closed);
+
+    /* When subPath is set to Ellipse figure, the type is PathPointTypeBezier */
+    stat = GdipPathIterNextPathType(iter, &result, &type, &start, &end);
+    expect(Ok, stat);
+    expect(6, start);
+    expect(18, end);
+    expect(PathPointTypeBezier, type);
+    expect(13, result);
+
+    /* As there is no more subpaths, when subPath is incremented,
+       it returns zeros */
+    result = start = end = (INT)0xdeadbeef;
+    closed = TRUE;
+    stat = GdipPathIterNextSubpath(iter, &result, &start, &end, &closed);
+    expect(Ok, stat);
+    expect(0, result);
+    expect(0, start);
+    expect(0, end);
+    expect(TRUE, closed);
+
+    /* When no more elements, after invoking NextPathType
+       the output numbers are not changed */
     start = end = result = (INT)0xdeadbeef;
     type = 255; /* out of range */
     stat = GdipPathIterNextPathType(iter, &result, &type, &start, &end);
-    todo_wine expect(Ok, stat);
+    expect(Ok, stat);
     expect((INT)0xdeadbeef, start);
     expect((INT)0xdeadbeef, end);
     expect(255, type);
-    todo_wine expect(0, result);
+    expect(0, result);
     GdipDeletePathIter(iter);

GdipDeletePath(path);

[Jeffrey Smith (＠whydoubt)]

Jeffrey Smith (@whydoubt) commented about dlls/gdiplus/tests/pathiterator.c:
> - result = start = end = (INT)0xdeadbeef;
> - closed = TRUE;
> - stat = GdipPathIterNextSubpath(iter, &result, &start, &end, &closed);
> - expect(Ok, stat);
> - expect(13, result);
> - expect(6, start);
> - expect(18, end);
> - expect(TRUE, closed);
> -
> - /* When subPath is set to Ellipse figure, the type is PathPointTypeBezier */
> - stat = GdipPathIterNextPathType(iter, &result, &type, &start, &end);
> - expect(Ok, stat);
> - expect(6, start);
> - expect(18, end);
> - expect(PathPointTypeBezier, type);
> - expect(13, result);

- Seems more consistent to check these in the order: result, type, start, end.
- You didn't initialize those values before the call.

[Jeffrey Smith (＠whydoubt)]

Jeffrey Smith (@whydoubt) commented about dlls/gdiplus/tests/pathiterator.c:
>  expect(Ok, stat);
> - GdipCreatePathIter(&iter, path);
> - expect(19, count);
> -
> - /* Set subPath (Line) */
> - result = start = end = (INT)0xdeadbeef;
> - closed = TRUE;
> - stat = GdipPathIterNextSubpath(iter, &result, &start, &end, &closed);
> - expect(Ok, stat);
> - expect(2, result);
> - expect(0, start);
> - expect(1, end);
> - expect(FALSE, closed);
> -
> - /* When subPath is set (Line), return position of points with the same type */
> - result = -2;

Looks like a left-over.  Not doing anything useful.

[Jeffrey Smith (＠whydoubt)]

On Wed Aug 16 18:21:31 2023 +0000, Jeffrey Smith wrote:
> - Seems more consistent to check these in the order: result, type,
>
> start, end.
> - You didn't initialize those values before the call.

I know the existing code is inconsistent, I was just thinking that checking the value in parameter order would be nice, here and a few other places in this MR.

[Jeffrey Smith (＠whydoubt)]

Jeffrey Smith (@whydoubt) commented about dlls/gdiplus/tests/pathiterator.c:
>  GdipCreatePath(FillModeAlternate, &path);
>  GdipCreatePathIter(&iter, path);
>
> - result = -2;
> - result = start = end = (INT)0xdeadbeef;
> closed = TRUE;
> stat = GdipPathIterNextSubpath(iter, &result, &start, &end, &closed);
> expect(Ok, stat);
> expect(0, result);
> - expect((INT)0xdeadbeef, start);
> - expect((INT)0xdeadbeef, end);
> - expect(TRUE, closed); /* Not changed */

FWIW, to _really_ test that it wasn't changed, it seems better to set it to a more-unlikely value.  Since BOOL is actually an int, you could set it to and check for -1 or 0xdeadbeef.

[Jeffrey Smith (＠whydoubt)]

Jeffrey Smith (@whydoubt) commented about dlls/gdiplus/pathiterator.c:
>  return Ok;
>
> }
> +// FIXME not returning anything

_What_ is not returning anything (but is supposed to) ?

[Jeffrey Smith (＠whydoubt)]

Jeffrey Smith (@whydoubt) commented about dlls/gdiplus/tests/pathiterator.c:
>  GdipDeletePathIter(iter);
>
> - /* single figure */
> - GdipAddPathLine(path, 0.0, 0.0, 10.0, 30.0);
> - /* figure with three subpaths */
> - stat = GdipAddPathLine(path, 0.0, 0.0, 10.0, 30.0);
> - expect(Ok, stat);
> - stat = GdipAddPathRectangle(path, 1.0, 2.0, 3.0, 4.0);
> - expect(Ok, stat);
> - stat = GdipAddPathEllipse(path, 0.0, 0.0, 35.0, 70.0);
> - expect(Ok, stat);
> GdipCreatePathIter(&iter, path);
> -
> - /* When subPath is not set,
> -
>   it is not possible to get Path Type */
> - result = -2;

Another left-over.

[Jeffrey Smith (＠whydoubt)]

Jeffrey Smith (@whydoubt) commented about dlls/gdiplus/pathiterator.c:
> - i = iter->pathtype_pos;
> - if (i >= count) {
> -
>    *result = 0;
> -
>    return Ok;
> - }
> -
> - /* set starting point to first PathPointTypeStart and */
> - if ((iter->pathdata.Types[i] & PathPointTypePathTypeMask) == PathPointTypeStart) {
> -
>    *start = i;
> -
>    i++;
> -
>    *end = i;
> -
>    *type = iter->pathdata.Types[i] & PathPointTypePathTypeMask;
> -
>    i++;
> - }
> - else
> -
>    return Ok;

- If you swap the if/else blocks, the current if body doesn't need to be in a block:
```suggestion:-8+0
    if ((iter->pathdata.Types[i] & PathPointTypePathTypeMask) != PathPointTypeStart)
        return Ok;

*start = i;
    i++;
    *end = i;
    *type = iter->pathdata.Types[i] & PathPointTypePathTypeMask;
    i++;
```
- Why do you not set *result when you return?

[Esme Povirk (＠madewokherd)]

Esme Povirk (@madewokherd) commented about dlls/gdiplus/pathiterator.c:
>      return InvalidParameter;
>
> - return NotImplemented;
> - count = iter->subpath_pos;
> - i = iter->pathtype_pos;
> - if (i >= count) {
> -
>    *result = 0;
> -
>    return Ok;
> - }
> -
> - /* set starting point to first PathPointTypeStart and */
> - if ((iter->pathdata.Types[i] & PathPointTypePathTypeMask) == PathPointTypeStart) {
> -
>    *start = i;
> -
>    i++;
> -
>    *end = i;
> -
>    *type = iter->pathdata.Types[i] & PathPointTypePathTypeMask;

I'm not sure, but I think this could access pathdata out of range if the path ends with PathPointTypeStart.

[Bartosz Kosiorek (＠gang65)]

On Wed Aug 16 19:08:13 2023 +0000, Jeffrey Smith wrote:
> _What_ is not returning anything (but is supposed to) ?

I mean it should not return anything (void):
https://learn.microsoft.com/en-us/windows/win32/api/gdipluspath/nf-gdipluspa...

[Nikolay Sivov (＠nsivov)]

On Wed Aug 16 21:17:48 2023 +0000, Bartosz Kosiorek wrote:
> I mean it should not return anything (void):
> https://learn.microsoft.com/en-us/windows/win32/api/gdipluspath/nf-gdipluspa...

What you linked are C++ wrappers.

[Bartosz Kosiorek (＠gang65)]

On Wed Aug 16 19:29:46 2023 +0000, Jeffrey Smith wrote:
> - If you swap the if/else blocks, the current if body doesn't need to be
>
> in a block:
>     if ((iter->pathdata.Types[i] & PathPointTypePathTypeMask) != PathPointTypeStart)
>         return Ok;
>     *start = i;
>     i++;
>     *end = i;
>     *type = iter->pathdata.Types[i] & PathPointTypePathTypeMask;
>     i++;
> - Why do you not set *result when you return?

How it could be done?

[Bartosz Kosiorek (＠gang65)]

On Wed Aug 16 19:32:02 2023 +0000, Esme Povirk wrote:
> I'm not sure, but I think this could access pathdata out of range if the
> path ends with PathPointTypeStart.

Good point. 
Do you know how to create such test case. I am not familiar what values should be set (start, end, result)?