[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 <gang65(a)poczta.onet.pl>

---
 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);
-- 
GitLab

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

[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.

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

[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.

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

[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.

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

[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.

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

[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) ?

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

[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.

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

[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?

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

[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.

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

[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...

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

[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.

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

[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:
> ```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?
> How it could be done?

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

[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)?

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