implement PS_USERSTYLE handling - obviously not correct ;)
Hi, just read again http://www.winehq.org/sending_patches and noticed that my first wine patch may fall in the category of not deserving an answer at all (to your standards). The one thing I noticed is my mail client putting line-wrapping into the patch, which should be easy to correct. But before resending it, I would appreciate some feedback if something else is 'obviously' wrong with it. To me it seems to be very difficult for the issue at hand to fall into the category of being "Obvious(ly) Correct". Although the X11 and win32 concepts of extended line styles are very similar, at least I did not know the scaling issues involved without trying it out. One thing which may be considered critical is my selection of the test program. As noted it is Dia for win32 (ported by me [1]) through gdk-win32-drawing code (PS_USERSTYLE usage implemented by me [2]). So if I have misunderstood something there seven years ago, that two bugs could compensate each other ;) Another issue might be the unconditional implementation of PS_USERSTYLE. After all it is only available with NT-based windows. Should x11drv check for the version being emulated to also reproduce the failing behaviour? Or would that be too much compatibility? What can I do to increase the probability of the patch being accepted? Thanks, Hans [1] http://hans.breuer.org/dia/ [2] http://svn.gnome.org/viewvc/gtk%2B?view=revision&revision=6330 [3] http://hans.breuer.org/dia/dia-wine.htm -------- Original-Nachricht -------- Betreff: implement PS_USERSTYLE handling, tested with Dia(win32) Datum: Sun, 29 Mar 2009 19:45:05 +0200 Von: Hans Breuer <hans(a)breuer.org> An: wine-patches(a)winehq.org From b89af7d06fc8cbf5210c61fd58ed62caeddad968 Mon Sep 17 00:00:00 2001 From: Hans Breuer <hans(a)breuer.org> Date: Sun, 29 Mar 2009 18:27:29 +0200 Subject: implement PS_USERSTYLE handling, tested with Dia(win32) - see http://hans.breuer.org/dia/dia-wine.htm --- dlls/winex11.drv/pen.c | 31 +++++++++++++++++++++++++++---- 1 files changed, 27 insertions(+), 4 deletions(-) diff --git a/dlls/winex11.drv/pen.c b/dlls/winex11.drv/pen.c index 49fe74c..039798e 100644 --- a/dlls/winex11.drv/pen.c +++ b/dlls/winex11.drv/pen.c @@ -54,12 +54,33 @@ HPEN CDECL X11DRV_SelectPen( X11DRV_PDEVICE *physDev, HPEN hpen ) elp = HeapAlloc( GetProcessHeap(), 0, size ); GetObjectW( hpen, size, elp ); - /* FIXME: add support for user style pens */ logpen.lopnStyle = elp->elpPenStyle; logpen.lopnWidth.x = elp->elpWidth; logpen.lopnWidth.y = 0; logpen.lopnColor = elp->elpColor; - + /* support for user style pens */ + if (MAX_DASHLEN < elp->elpNumEntries) + { + FIXME("PS_USERSTYLE: len(dashes)>MAX_DASHLEN %d,%d\n", elp->elpNumEntries, MAX_DASHLEN); + physDev->pen.dash_len = 0; + } + else + { + BOOL overflow = FALSE; + physDev->pen.dash_len = elp->elpNumEntries; + for (i = 0; i < elp->elpNumEntries; ++i) + { + if (elp->elpStyleEntry[i] > 255) + { + overflow = TRUE; /* can't fit the type */ + physDev->pen.dashes[i] = 255; + } + else + physDev->pen.dashes[i] = (char)elp->elpStyleEntry[i]; + } + if (overflow) + FIXME("PS_USERSTYLE: dashes clipped (type overflow)\n"); + } HeapFree( GetProcessHeap(), 0, elp ); } else @@ -108,14 +129,16 @@ HPEN CDECL X11DRV_SelectPen( X11DRV_PDEVICE *physDev, HPEN hpen ) memcpy(physDev->pen.dashes, PEN_alternate, physDev->pen.dash_len); break; case PS_USERSTYLE: - FIXME("PS_USERSTYLE is not supported\n"); + /* handled above */ + break; /* fall through */ default: physDev->pen.dash_len = 0; break; } if(physDev->pen.ext && physDev->pen.dash_len && - (logpen.lopnStyle & PS_STYLE_MASK) != PS_ALTERNATE) + (logpen.lopnStyle & PS_STYLE_MASK) != PS_ALTERNATE && + (logpen.lopnStyle & PS_STYLE_MASK) != PS_USERSTYLE) for(i = 0; i < physDev->pen.dash_len; i++) physDev->pen.dashes[i] *= (physDev->pen.width ? physDev->pen.width : 1); -- 1.6.0.6 -------- Hans "at" Breuer "dot" Org ----------- Tell me what you need, and I'll tell you how to get along without it. -- Dilbert
On Sat, Apr 4, 2009 at 4:49 PM, Hans Breuer <hans(a)breuer.org> wrote:
Hi, just read again http://www.winehq.org/sending_patches and noticed that my first wine patch may fall in the category of not deserving an answer at all (to your standards).
Not always the case. It could be just that no one is familiar enough with that exact part of the code to provide feed back at a glance.
But before resending it, I would appreciate some feedback if something else is 'obviously' wrong with it. To me it seems to be very difficult for the issue at hand to fall into the category of being "Obvious(ly) Correct".
What can I do to increase the probability of the patch being accepted?
Adding test cases goes along way towards being obviously correct and getting accepted into mainstream wine. --John Klehm
Hi Hans, I have run the patch against my application and the line-styles draw as far as I can tell are the same as on Windows when a single line segment where the pixel width of one is used. However when a polyline is drawn the dash style does not get drawn correctly. There seems to be an issue with repeat of the style when the pixel width of the line is increased. It looks like the style gets multiplied up. I've not looked too deeply into this, however it looks like all pen styles are affected by the requested line width. I also have an issue with the standard styles as they are defined as I belive they are not correct. I'm comparing on a pixel to pixel basis between Windows and Wine. However your patch does not affect this. I just wished to make you aware. When I have a sample to demonstrate this I will raise a bug report. I've made some additional comments below on the patch and on the platforms supporting PS_USERSTYLE. In addition there is also the odd even count for the bits that needs to be dealt with. However in most cases I doubt this would be an issue. I'm also a bit of a newbie here and am not exactly sure how to advance this patch. I can create a simple Win32 test which demonstrates the behaviour I am looking for. Of course the Windows results will be a screenshoot which will have to be compared against the test running under Wine. Where would I post this sample? (no guarentees on how long it's going to take me as I'm still trying to write a series of alpha blend tests). Regards Damian On Sat, Apr 4, 2009 at 9:49 PM, Hans Breuer <hans(a)breuer.org> wrote:
Hi, just read again http://www.winehq.org/sending_patches and noticed that my first wine patch may fall in the category of not deserving an answer at all (to your standards).
The one thing I noticed is my mail client putting line-wrapping into the patch, which should be easy to correct.
But before resending it, I would appreciate some feedback if something else is 'obviously' wrong with it. To me it seems to be very difficult for the issue at hand to fall into the category of being "Obvious(ly) Correct".
Although the X11 and win32 concepts of extended line styles are very similar, at least I did not know the scaling issues involved without trying it out.
One thing which may be considered critical is my selection of the test program. As noted it is Dia for win32 (ported by me [1]) through gdk-win32-drawing code (PS_USERSTYLE usage implemented by me [2]). So if I have misunderstood something there seven years ago, that two bugs could compensate each other ;)
Another issue might be the unconditional implementation of PS_USERSTYLE. After all it is only available with NT-based windows. Should x11drv check for the version being emulated to also reproduce the failing behaviour? Or would that be too much compatibility?
PS_USERSTYLE is available on all the systems I am currently using: 2000, XP & Vista.
What can I do to increase the probability of the patch being accepted?
Thanks, Hans
[1] http://hans.breuer.org/dia/ [2] http://svn.gnome.org/viewvc/gtk%2B?view=revision&revision=6330 [3] http://hans.breuer.org/dia/dia-wine.htm
-------- Original-Nachricht -------- Betreff: implement PS_USERSTYLE handling, tested with Dia(win32) Datum: Sun, 29 Mar 2009 19:45:05 +0200 Von: Hans Breuer <hans(a)breuer.org> An: wine-patches(a)winehq.org
From b89af7d06fc8cbf5210c61fd58ed62caeddad968 Mon Sep 17 00:00:00 2001 From: Hans Breuer <hans(a)breuer.org> Date: Sun, 29 Mar 2009 18:27:29 +0200 Subject: implement PS_USERSTYLE handling, tested with Dia(win32) - see http://hans.breuer.org/dia/dia-wine.htm
--- dlls/winex11.drv/pen.c | 31 +++++++++++++++++++++++++++---- 1 files changed, 27 insertions(+), 4 deletions(-)
diff --git a/dlls/winex11.drv/pen.c b/dlls/winex11.drv/pen.c index 49fe74c..039798e 100644 --- a/dlls/winex11.drv/pen.c +++ b/dlls/winex11.drv/pen.c @@ -54,12 +54,33 @@ HPEN CDECL X11DRV_SelectPen( X11DRV_PDEVICE *physDev, HPEN hpen ) elp = HeapAlloc( GetProcessHeap(), 0, size );
GetObjectW( hpen, size, elp ); - /* FIXME: add support for user style pens */ logpen.lopnStyle = elp->elpPenStyle; logpen.lopnWidth.x = elp->elpWidth; logpen.lopnWidth.y = 0; logpen.lopnColor = elp->elpColor; - + /* support for user style pens */ + if (MAX_DASHLEN < elp->elpNumEntries) + { + FIXME("PS_USERSTYLE: len(dashes)>MAX_DASHLEN %d,%d\n", elp->elpNumEntries, MAX_DASHLEN); + physDev->pen.dash_len = 0; + } + else + { + BOOL overflow = FALSE; + physDev->pen.dash_len = elp->elpNumEntries; + for (i = 0; i < elp->elpNumEntries; ++i) + { + if (elp->elpStyleEntry[i] > 255) + { + overflow = TRUE; /* can't fit the type */ + physDev->pen.dashes[i] = 255; + } + else + physDev->pen.dashes[i] = (char)elp->elpStyleEntry[i]; + } + if (overflow) + FIXME("PS_USERSTYLE: dashes clipped (type overflow)\n"); + } HeapFree( GetProcessHeap(), 0, elp ); } else @@ -108,14 +129,16 @@ HPEN CDECL X11DRV_SelectPen( X11DRV_PDEVICE *physDev, HPEN hpen ) memcpy(physDev->pen.dashes, PEN_alternate, physDev->pen.dash_len); break; case PS_USERSTYLE: - FIXME("PS_USERSTYLE is not supported\n"); + /* handled above */ + break; /* fall through */
The comment here is no longer required.
default: physDev->pen.dash_len = 0; break; } if(physDev->pen.ext && physDev->pen.dash_len && - (logpen.lopnStyle & PS_STYLE_MASK) != PS_ALTERNATE) + (logpen.lopnStyle & PS_STYLE_MASK) != PS_ALTERNATE && + (logpen.lopnStyle & PS_STYLE_MASK) != PS_USERSTYLE) for(i = 0; i < physDev->pen.dash_len; i++) physDev->pen.dashes[i] *= (physDev->pen.width ? physDev->pen.width : 1);
-- 1.6.0.6
-------- Hans "at" Breuer "dot" Org ----------- Tell me what you need, and I'll tell you how to get along without it. -- Dilbert
Hi Daminan, thanks for looking into this. At 06.04.2009 15:03, Damian Dixon wrote:
Hi Hans,
I have run the patch against my application and the line-styles draw as far as I can tell are the same as on Windows when a single line segment where the pixel width of one is used.
However when a polyline is drawn the dash style does not get drawn correctly. There seems to be an issue with repeat of the style when the pixel width of the line is increased. It looks like the style gets multiplied up.
The scaling with linestyle is intentionally and at least with my (extended) test it seems to be. I've now run two versions of Dia 0.97 on three platforms. The second screenshot at http://hans.breuer.org/dia/dia-wine.htm shows my standard render test with wine, X11 and win2k side by side.
I've not looked too deeply into this, however it looks like all pen styles are affected by the requested line width.
Yes, look at the third last line of the patch. The line pattern is scaled by the line width for PS_ALTERNATE and PS_USERSTYLE.
I also have an issue with the standard styles as they are defined as I belive they are not correct. I'm comparing on a pixel to pixel basis between Windows and Wine. However your patch does not affect this. I just wished to make you aware. When I have a sample to demonstrate this I will raise a bug report.
As simple test program showing the different line styles would certainly help.
I've made some additional comments below on the patch and on the platforms supporting PS_USERSTYLE.
Yes, as I said: supported for all NT based versions; not supported at all for win9x versions. IIRC even the normal PS_(DASH)?(DOT)* are limited there.
In addition there is also the odd even count for the bits that needs to be dealt with. However in most cases I doubt this would be an issue.
Are you talking about PS_GEOMETRIC?
I'm also a bit of a newbie here and am not exactly sure how to advance this patch.
I can create a simple Win32 test which demonstrates the behaviour I am looking for. Of course the Windows results will be a screenshoot which will have to be compared against the test running under Wine.
I've done exactly this, although my test program is not simple, but a cross platfom diagramming tool: http://projects.gnome.org/dia .
Where would I post this sample? (no guarentees on how long it's going to take me as I'm still trying to write a series of alpha blend tests).
I'm not sure for wine. For other projects I work on bugzilla would be the place to go. Thanks, Hans -------- Hans "at" Breuer "dot" Org ----------- Tell me what you need, and I'll tell you how to get along without it. -- Dilbert
participants (3)
-
Damian Dixon -
Hans Breuer -
John Klehm