On 6/27/07, Evan Stade <estade at gmail.com> wrote:
Hi,
[try2] The patch I sent yesterday was not properly todo_wined. Also, this test is a bit more extensive (about twice as many points drawn). It uses various point-type combination (even non-sensical ones such as PT_LINETO | PT_MOVETO) to test the exact logic of PolyDraw with an open path.
changelog:
- added polydraw test to path testing (5 more todo_wines)
dlls/gdi32/tests/path.c | 70 +++++++++++++++++++++++++++++++++++++++++++++++ 1 files changed, 70 insertions(+), 0 deletions(-)
-- Evan Stade
Also, I'd add a PT_MOVETO into your first PolyDraw call so you can tell whether PT_CLOSEFIGURE adds a PT_MOVETO to orig_pos or lastmove, as the current version does not differentiate this.
Misha
On 6/29/07, Koshelev, Misha Vladislavo mk144210@bcm.tmc.edu wrote:
On 6/27/07, Evan Stade <estade at gmail.com> wrote:
Hi,
[try2] The patch I sent yesterday was not properly todo_wined. Also, this test is a bit more extensive (about twice as many points drawn). It uses various point-type combination (even non-sensical ones such as PT_LINETO | PT_MOVETO) to test the exact logic of PolyDraw with an open path.
changelog:
- added polydraw test to path testing (5 more todo_wines)
dlls/gdi32/tests/path.c | 70
+++++++++++++++++++++++++++++++++++++++++++++++
1 files changed, 70 insertions(+), 0 deletions(-)
-- Evan Stade
Also, I'd add a PT_MOVETO into your first PolyDraw call so you can tell whether PT_CLOSEFIGURE adds a PT_MOVETO to orig_pos or lastmove, as the current version does not differentiate this.
Misha
Thanks a lot for your help.
Your shorter diff may make the tests conform, but it incorrectly assigns lastmove.x = dc->CursPosX. This only makes it conform to the tests because the tests happen to have a MoveToEx as the last path point before the PolyDraw is called. Also, your diff doesn't correctly check the point types. The test if( lpbTypes[i] == PT_MOVETO) (which was there before your changes) is wrong because it returns false for PT_MOVETO | PT_LINETO, when it should return true (according to my testing which was not a part of the test I submitted). Also, your diff does not MoveToEx to orig_pos at the second "return FALSE". This is incorrect but again does not show up in the tests.
So while a small and simple diff can make these specific tests conform, it is not correct. However I still don't understand how this influences whether the test patch got accepted. Your explanation of why the fix patch didn't get accepted on the other hand does make sense.
On Fri, 2007-06-29 at 14:49 -0700, Evan Stade wrote:
Thanks a lot for your help.
No prob.
Your shorter diff may make the tests conform, but it incorrectly assigns lastmove.x = dc->CursPosX. This only makes it conform to the tests because the tests happen to have a MoveToEx as the last path point before the PolyDraw is called.
You should include this in your tests, e.g., by calling LineTo after the MoveToEx and before the first PolyDraw. This immediately makes it clear why you need to have separate path and non-path cases. (I personally think about conformance tests like this - you worked hard to figure out how the function works on native and implemented your own version, but someone might commit a patch sometime in the future to fix something else that screws up all your hard work; that's why you need to put this knowledge into conformance tests, so once that feature is implemented in wine it will never break again).
Also, your diff doesn't correctly check the point types. The test if( lpbTypes[i] == PT_MOVETO) (which was there before your changes) is wrong because it returns false for PT_MOVETO | PT_LINETO, when it should return true (according to my testing which was not a part of the test I submitted). Also, your diff does not MoveToEx to orig_pos at the second "return FALSE". This is incorrect but again does not show up in the tests.
Again, these are things that should ideally be a part of your conformance test. That would make the rationale for your extensive changes in your second patch clearer and would improve the likelihood of the patches going in.
So while a small and simple diff can make these specific tests conform, it is not correct. However I still don't understand how this influences whether the test patch got accepted. Your explanation of why the fix patch didn't get accepted on the other hand does make sense.
I am not sure why the test patch was not accepted (unless maybe Alexandre wants both patches ready to go before committing either, as if you make any changes to the second one you might find you need to add something to the tests... don't know), but having the whole patchset complete and good to go certainly wouldn't hurt either patch.
Misha
-
Evan Stade -
Koshelev, Misha Vladislavo -
Misha Koshelev