Please note that most of this format (everything except the first 8 bytes of region_header) is documented in MS-EMFPLUS section 2.2.1.8: http://msdn.microsoft.com/en-us/library/cc230724.aspx
+ switch (path_header->flags & FLAGS_INTPATH)
You should check for other flags here, namely 0x1000 which indicates RLE compression of point types and 0x0400 which indicates that points are relative to the previous point. (Not saying we need to support them initially, but please check so we know if they appear. Also, the lower 16 bits other than those flags are supposed to always be 0, so maybe we should check them for unknown values as well.)
+ if (!region_header || region_header->magic != VERSION_MAGIC) + return InvalidParameter;
This should also accept 0xdbc01002, for GDI+ version 1.1.
+ if (i == 1) continue; /* data[1] never matches */
If that's the case, shouldn't you have a test that it doesn't match?
Vincent Povirk madewokherd@gmail.com wrote:
Please note that most of this format (everything except the first 8 bytes of region_header) is documented in MS-EMFPLUS section 2.2.1.8: http://msdn.microsoft.com/en-us/library/cc230724.aspx
switch (path_header->flags & FLAGS_INTPATH)You should check for other flags here, namely 0x1000 which indicates RLE compression of point types and 0x0400 which indicates that points are relative to the previous point. (Not saying we need to support them initially, but please check so we know if they appear. Also, the lower 16 bits other than those flags are supposed to always be 0, so maybe we should check them for unknown values as well.)
Supposedly Wine implementation of GdipCreateRegionRgnData should just support the format of data Wine's GdipGetRegionData. If there are cases when application feed data created by Windows gdiplus support for that cases could be added later.
- if (!region_header || region_header->magic != VERSION_MAGIC)
return InvalidParameter;This should also accept 0xdbc01002, for GDI+ version 1.1.
See above.
if (i == 1) continue; /* data[1] never matches */If that's the case, shouldn't you have a test that it doesn't match?
Existing tests just skip this DWORD as well, and I don't see much point in testing it either.
On Mon, Nov 18, 2013 at 8:04 PM, Dmitry Timoshkov dmitry@baikal.ru wrote:
Supposedly Wine implementation of GdipCreateRegionRgnData should just support the format of data Wine's GdipGetRegionData. If there are cases when application feed data created by Windows gdiplus support for that cases could be added later.
We have to support the same format as Windows because this is a serialization format. Why would you be writing tests for the output of GdipGetRegionData if you didn't intend to be compatible?
It's fine to not support the cases we don't need now, but we should have FIXME's in place so we can tell when that lack of support breaks an application.
Vincent Povirk madewokherd@gmail.com wrote:
Supposedly Wine implementation of GdipCreateRegionRgnData should just support the format of data Wine's GdipGetRegionData. If there are cases when application feed data created by Windows gdiplus support for that cases could be added later.
We have to support the same format as Windows because this is a serialization format. Why would you be writing tests for the output of GdipGetRegionData if you didn't intend to be compatible?
Just in case some applications depend on it. So far noone cared, at least regarding bug reports.
It's fine to not support the cases we don't need now, but we should have FIXME's in place so we can tell when that lack of support breaks an application.
Current GdipCreateRegionRgnData implementation fully supports data generated by GdipGetRegionData, and I don't have an intention to pollute the code with useless FIXMEs. If you have an application that doesn't work with my implementation then just file a bug report and I'll have a look.
Just in case some applications depend on it. So far noone cared, at least regarding bug reports.
So far Wine has never had an implementation of region deserialization functions, so how could anyone have complained about it?
Current GdipCreateRegionRgnData implementation fully supports data generated by GdipGetRegionData, and I don't have an intention to pollute the code with useless FIXMEs. If you have an application that doesn't work with my implementation then just file a bug report and I'll have a look.
This is using FIXME's for their intended purpose, namely highlighting missing features when applications use them. I'm trying to avoid a situation where an application uses one of those features and a developer wastes hours tracking down the bug when we could have made it obvious for them.
It's nice that you're willing to support your code, but that doesn't help if we can't track the problem to your code to begin with.
Vincent Povirk madewokherd@gmail.com wrote:
Just in case some applications depend on it. So far noone cared, at least regarding bug reports.
So far Wine has never had an implementation of region deserialization functions, so how could anyone have complained about it?
I was talking about data generated by GdipGetRegionData.
Current GdipCreateRegionRgnData implementation fully supports data generated by GdipGetRegionData, and I don't have an intention to pollute the code with useless FIXMEs. If you have an application that doesn't work with my implementation then just file a bug report and I'll have a look.
This is using FIXME's for their intended purpose, namely highlighting missing features when applications use them. I'm trying to avoid a situation where an application uses one of those features and a developer wastes hours tracking down the bug when we could have made it obvious for them.
If you see obvious places of adding such FIXMEs then send a patch which adds them, personally I don't care at this point about it.
-
Dmitry Timoshkov -
Vincent Povirk