Re: gdiplus: fix GdipFillRectangleI no effect on memoryDCwhosewindoworiginpoint has changed
Hi,
I would prefer that you use the expect() macro when comparing integers in tests.
If you don't expect a function (in this case GetDIBits, in patch 2) to fail when running a test, use ok() to check that it succeeded. It's OK if the test will crash when the ok() check fails.
If GetDIBits really does fail sometimes, we need to understand why that happens and document it in the test.
When writing comments about this, make sure you specify that you are talking about gdi32 device coordinates, which are different from gdiplus device coordinates.
I have modified, please see the new patch file.
- status = GdipSetClipRectI(graphics, rect.left+width/2, rect.top+height/2,
rect.right, rect.bottom, CombineModeReplace);This looks wrong to me. GdipSetClipRectI takes a width and height, not right and bottom.
The above statement is setting a clip range: (rect.left+width/2, rect.top+height/2) - (rect.left+rect.right+width/2, rect.top+rect.bottom+height/2). Because the rect is {100, 100, 180, 180}, so the clip range is: (140,140) - (320,320). In fact GdipSetClipRectI can take any int value to specify a clip region. But according to your advice, I have changed to take a width and height .
For helping review, I attached a BMP file which is the output of each test_GdipFillRectangle drawing on Memory DC. Thank you.
------------------ Regards.
The patches look good to me.
On Mon, Dec 22, 2014 at 6:40 AM, Changhui Liu liuchanghui@linuxdeepin.com wrote:
Hi,
I would prefer that you use the expect() macro when comparing integers in tests.
If you don't expect a function (in this case GetDIBits, in patch 2) to fail when running a test, use ok() to check that it succeeded. It's OK if the test will crash when the ok() check fails.
If GetDIBits really does fail sometimes, we need to understand why that happens and document it in the test.
When writing comments about this, make sure you specify that you are talking about gdi32 device coordinates, which are different from gdiplus device coordinates.
I have modified, please see the new patch file.
- status = GdipSetClipRectI(graphics, rect.left+width/2,
rect.top+height/2,
rect.right, rect.bottom, CombineModeReplace);This looks wrong to me. GdipSetClipRectI takes a width and height, not right and bottom.
The above statement is setting a clip range: (rect.left+width/2, rect.top+height/2) - (rect.left+rect.right+width/2, rect.top+rect.bottom+height/2). Because the rect is {100, 100, 180, 180}, so the clip range is: (140,140) - (320,320). In fact GdipSetClipRectI can take any int value to specify a clip region. But according to your advice, I have changed to take a width and height .
For helping review, I attached a BMP file which is the output of each test_GdipFillRectangle drawing on Memory DC. Thank you.
Regards.
I have modified, please see the new patch file.
It seems the "new patch file" was not sent to public mailing list. Changhui, would you mind resend your final patch to wine-patches? Also, Vincent, if you could suggest for a better commit message, that would be nice :) You are native English speaker, neither I or Changhui is native. But if the commit message looks good to you, then please ignore me.
It seems the "new patch file" was not sent to public mailing list. Changhui, would you mind resend your final patch to wine-patches?
That's odd. Wine-devel was CCed on the mail. Maybe the image attachment made the email too large, and it was filtered.
They'll have to be sent to wine-patches anyway for proper review.
Also, Vincent, if you could suggest for a better commit message, that would be nice :) You are native English speaker, neither I or Changhui is native. But if the commit message looks good to you, then please ignore me.
Ah, I sort of skipped over the commit messages, sorry. I would just remove "no effect" from the third commit message, because there can be an effect sometimes, it's just wrong. You could also remove the word "error" from the first two.
The messages don't sound completely natural in English to me, but there is also limited space, and I can't think of a way to make them more natural without also making them longer or less descriptive.
Dear all: Thanks for your help. I have sent the patch to wine-patches.
------------------ Regards.
------------------ Original ------------------ From: "Vincent Povirk"madewokherd@gmail.com; Date: Tue, Dec 23, 2014 02:05 AM To: "Qian Hong"fracting@gmail.com; Cc: "Changhui Liu"liuchanghui@linuxdeepin.com; "wine-devel"wine-devel@winehq.org; Subject: Re: gdiplus: fix GdipFillRectangleI no effect on memoryDCwhosewindoworiginpointhas changed
It seems the "new patch file" was not sent to public mailing list. Changhui, would you mind resend your final patch to wine-patches?
That's odd. Wine-devel was CCed on the mail. Maybe the image attachment made the email too large, and it was filtered.
They'll have to be sent to wine-patches anyway for proper review.
Also, Vincent, if you could suggest for a better commit message, that would be nice :) You are native English speaker, neither I or Changhui is native. But if the commit message looks good to you, then please ignore me.
Ah, I sort of skipped over the commit messages, sorry. I would just remove "no effect" from the third commit message, because there can be an effect sometimes, it's just wrong. You could also remove the word "error" from the first two.
The messages don't sound completely natural in English to me, but there is also limited space, and I can't think of a way to make them more natural without also making them longer or less descriptive.
-
Changhui Liu -
Qian Hong -
Vincent Povirk