http://bugs.winehq.org/show_bug.cgi?id=33378
Bug #: 33378 Summary: measuring/painting strings needs better tests Product: Wine Version: 1.5.28 Platform: x86 OS/Version: Linux Status: UNCONFIRMED Severity: normal Priority: P2 Component: gdiplus AssignedTo: wine-bugs@winehq.org ReportedBy: galtgendo@o2.pl Classification: Unclassified Regression SHA1: 6ab04040e52465e77558a067309e8a54bdc0f32c
Created attachment 44169 --> http://bugs.winehq.org/attachment.cgi?id=44169 minimal fix for a clipping problem
This is sort of bug 25717 revisited, in that that a problem, that happened shortly before it was fixed, has popped up again.
I haven't been checking it in awhile, but today I did, then had to trace it back to 1.5.26.
After a bisect, it came to commit "gdiplus: GdipMeasureCharacterRanges should treat empty layout extents as infinite when StringFormatFlagsNoClip is specified.". Clipping went broken again, resulting in invisible text.
Minimal fix attached and sent to wine-patches.
As I said, it's the clipping again - without resetting < 0.5 to (1<< 23), 'if (!(format_flags & StringFormatFlagsNoClip) && scaled_rect.Width != 1 << 23 && scaled_rect.Height != 1 << 23)' condition doesn't go into effect.
http://bugs.winehq.org/show_bug.cgi?id=33378
Dmitry Timoshkov dmitry@baikal.ru changed:
What |Removed |Added ---------------------------------------------------------------------------- Regression SHA1|6ab04040e52465e77558a067309 | |e8a54bdc0f32c |
http://bugs.winehq.org/show_bug.cgi?id=33378
--- Comment #1 from Rafał Mużyło galtgendo@o2.pl 2013-04-16 07:56:22 CDT --- So, as the patch got rejected, does anyone have a better idea of how to fix it ?
Cause, for whatever the reason, that opt-out from clipping for small areas seems to be needed here.
http://bugs.winehq.org/show_bug.cgi?id=33378
--- Comment #2 from Vincent Povirk madewokherd@gmail.com 2013-04-30 13:23:44 CDT --- I did a test on windows, and it appears we need to skip clipping if the ORIGINAL width and height are <= 0.
http://bugs.winehq.org/show_bug.cgi?id=33378
--- Comment #3 from Rafał Mużyło galtgendo@o2.pl 2013-04-30 22:12:48 CDT --- (In reply to comment #2)
I did a test on windows, and it appears we need to skip clipping if the ORIGINAL width and height are <= 0.
Do you mean something like '&& (rect->Width > 0.5) && (rect->Height > 0.5)' added to 'if (!(format_flags & StringFormatFlagsNoClip) && ...' condition ?
This indeed seems to work, though looks potentially fishy.
http://bugs.winehq.org/show_bug.cgi?id=33378
--- Comment #4 from Rafał Mużyło galtgendo@o2.pl 2013-06-08 12:02:46 CDT --- (In reply to comment #2)
I did a test on windows, and it appears we need to skip clipping if the ORIGINAL width and height are <= 0.
Well, out of curiosity, I've added a quite noisy TRACE to see if there's anything special about those rectangles.
All of those rectangles (the originals, not just scaled versions) have Width = Height = 0, according to debugstr_rectf (so they're simply empty). Chances are it's DrawString(WCHAR*,INT,Font*,PointF&,Brush*), that's really being called here, so Width/Height never really mattered.
http://bugs.winehq.org/show_bug.cgi?id=33378
--- Comment #5 from Rafał Mużyło galtgendo@o2.pl 2013-06-10 16:27:52 CDT --- Created attachment 44742 --> http://bugs.winehq.org/attachment.cgi?id=44742 the other (also working) fix
Given the previous comments here *and* the comment in the file, this whole condition needs to be reconsidered, but lets fix the regression first.
http://bugs.winehq.org/show_bug.cgi?id=33378
Rafał Mużyło galtgendo@o2.pl changed:
What |Removed |Added ---------------------------------------------------------------------------- CC| |dtimoshkov@codeweavers.com
--- Comment #6 from Rafał Mużyło galtgendo@o2.pl 2013-06-14 17:52:27 CDT --- Dmitry Timoshkov wrote:
It would be nice to see some test cases first.
It would be nice to know enough Win32 API to write one - not there yet.
OK, I might be a bit lazy about it, as chances are one of the existing tests could be adapted, on the other hand though, it seems to me that this test would need to be interactive - strings are simply not drawn, but there doesn't seem to be any testable error.
http://bugs.winehq.org/show_bug.cgi?id=33378
Dmitry Timoshkov dmitry@baikal.ru changed:
What |Removed |Added ---------------------------------------------------------------------------- CC|dtimoshkov@codeweavers.com |
http://bugs.winehq.org/show_bug.cgi?id=33378
--- Comment #7 from Rafał Mużyło galtgendo@o2.pl 2013-08-02 16:01:55 CDT --- Status update for 1.7.0: my patch dropped from the list during 1.6 cycle, I still don't have a testcase written (which I'm nearly sure would need to be interactive anyway) and GdipDrawString is still broken.
http://bugs.winehq.org/show_bug.cgi?id=33378
--- Comment #8 from Rafał Mużyło galtgendo@o2.pl 2013-08-02 21:04:07 CDT --- Created attachment 45493 --> http://bugs.winehq.org/attachment.cgi?id=45493 some code I've managed to google
This isn't quite a testcase, just a bit of code, I've managed to first google, then rewrite from C++ (and even finding that little was quite a bitch - everything seems either C#/VB or uses mfc). Rewriting was necessary, cause wine headers of gdiplus seem insufficient for C++.
With my hack, the string is drawn, without it - not. Now, a result from Windows is necessary, but even I was registered on WineTestBot (which I'm not), with this code there's only eyeball test.
http://bugs.winehq.org/show_bug.cgi?id=33378
--- Comment #9 from Rafał Mużyło galtgendo@o2.pl 2013-08-02 21:13:00 CDT --- ...of course, given the status of plain C API of gdiplus by Microsoft, there's a chance it's only reproducible only with C++ API and PointF& version of DrawString calls.
http://bugs.winehq.org/show_bug.cgi?id=33378
--- Comment #10 from Dmitry Timoshkov dmitry@baikal.ru 2013-08-02 21:47:25 CDT --- (In reply to comment #9)
...of course, given the status of plain C API of gdiplus by Microsoft, there's a chance it's only reproducible only with C++ API and PointF& version of DrawString calls.
There is no such a thing as C++ gdiplus API.
(In reply to comment #8)
With my hack, the string is drawn, without it - not. Now, a result from Windows is necessary, but even I was registered on WineTestBot (which I'm not), with this code there's only eyeball test.
You need to write the tests and run them under Windows before you'll start to play with them under Wine, otherwise it's pretty easy to confuse yourself with controversial results.
Probably bug 34102 is a duplicate of this one, but it's hard to tell since it was never clear in this bug what it is about.
http://bugs.winehq.org/show_bug.cgi?id=33378
--- Comment #11 from Rafał Mużyło galtgendo@o2.pl 2013-08-02 22:59:57 CDT --- (In reply to comment #10)
(In reply to comment #9)
...of course, given the status of plain C API of gdiplus by Microsoft, there's a chance it's only reproducible only with C++ API and PointF& version of DrawString calls.
There is no such a thing as C++ gdiplus API.
Sure there is. I.e. Graphics.DrawString(const WCHAR*, INT, const Font*, const PointF, const Brush*)
But it seems at least *parts* of it aren't implemented in wine. At very least namespacing seems broken.
(In reply to comment #8)
With my hack, the string is drawn, without it - not. Now, a result from Windows is necessary, but even I was registered on WineTestBot (which I'm not), with this code there's only eyeball test.
You need to write the tests and run them under Windows before you'll start to play with them under Wine, otherwise it's pretty easy to confuse yourself with controversial results.
...and I would build them with... That's assuming I'd be willing to install the whole toolchain under Windows (and that's assuming it would be an option for me in the first place).
With 6ab04040e52465e77558a067309e8a54bdc0f32c regression being clear, somebody does need to test it with Windows, but, as I said, I'm unsure if C++ won't be needed to show it.
http://bugs.winehq.org/show_bug.cgi?id=33378
--- Comment #12 from Rafał Mużyło galtgendo@o2.pl 2013-08-02 23:06:05 CDT --- ...and looking briefly at bug 34102bug 34102, it most likely is a duplicate of this one.
http://bugs.winehq.org/show_bug.cgi?id=33378
--- Comment #13 from Dmitry Timoshkov dmitry@baikal.ru 2013-08-03 00:57:03 CDT --- (In reply to comment #11)
...of course, given the status of plain C API of gdiplus by Microsoft, there's a chance it's only reproducible only with C++ API and PointF& version of DrawString calls.
There is no such a thing as C++ gdiplus API.
Sure there is. I.e. Graphics.DrawString(const WCHAR*, INT, const Font*, const PointF, const Brush*)
That's not an API, this is not a gdiplus.dll public export.
But it seems at least *parts* of it aren't implemented in wine. At very least namespacing seems broken.
What you are talking about is C++ wrappers around public gdiplus APIs.
You need to write the tests and run them under Windows before you'll start to play with them under Wine, otherwise it's pretty easy to confuse yourself with controversial results.
...and I would build them with... That's assuming I'd be willing to install the whole toolchain under Windows (and that's assuming it would be an option for me in the first place).
That's how it usually works, you can't assume how win32 API actually works without testing it under Windows. Testbot only helps to extend API coverage by providing more Windows platform versions, in general you can't create a test by observing only testbot VMs behaviour.
With 6ab04040e52465e77558a067309e8a54bdc0f32c regression being clear, somebody does need to test it with Windows, but, as I said, I'm unsure if C++ won't be needed to show it.
In order to test gdiplus behaviour it's not required to use C++ (and existing gdiplus tests prove this), moreover C++ usage is harmful since it hides actual details about public API usage.
http://bugs.winehq.org/show_bug.cgi?id=33378
--- Comment #14 from Rafał Mużyło galtgendo@o2.pl 2013-08-03 11:37:41 CDT --- (In reply to comment #13)
(In reply to comment #11) That's not an API, this is not a gdiplus.dll public export.
But it seems at least *parts* of it aren't implemented in wine. At very least namespacing seems broken.
What you are talking about is C++ wrappers around public gdiplus APIs.
...and just about everything I could google, including MSDN, shows that's the *primary* API, at least if you're not using C#.
Just, i.e. http://msdn.microsoft.com/en-us/library/windows/desktop/ms533969(v=vs.85).as....
In order to test gdiplus behaviour it's not required to use C++ (and existing gdiplus tests prove this), moreover C++ usage is harmful since it hides actual details about public API usage.
See above.
http://bugs.winehq.org/show_bug.cgi?id=33378
--- Comment #15 from Rafał Mużyło galtgendo@o2.pl 2013-08-03 11:40:03 CDT --- Created attachment 45495 --> http://bugs.winehq.org/attachment.cgi?id=45495 the original C++ code
...and just for the sake of the argument, this is the original C++ code, a copy-paste from a forum post I've managed to find. As noted, it fails to build under wine.
http://bugs.winehq.org/show_bug.cgi?id=33378
--- Comment #16 from Dmitry Timoshkov dmitry@baikal.ru 2013-08-03 12:32:55 CDT --- (In reply to comment #14)
That's not an API, this is not a gdiplus.dll public export.
But it seems at least *parts* of it aren't implemented in wine. At very least namespacing seems broken.
What you are talking about is C++ wrappers around public gdiplus APIs.
...and just about everything I could google, including MSDN, shows that's the *primary* API, at least if you're not using C#.
Just, i.e. http://msdn.microsoft.com/en-us/library/windows/desktop/ms533969(v=vs.85).as....
"Windows GDI+ exposes a flat API that consists of about 600 functions, which are implemented in Gdiplus.dll and declared in Gdiplusflat.h. The functions in the GDI+ flat API are wrapped by a collection of about 40 C++ classes."
That's a quote from MSDN referenced above. Do you see a difference between flat gdiplus APIs and a collection of C++ wrapper classes?
http://bugs.winehq.org/show_bug.cgi?id=33378
--- Comment #17 from Rafał Mużyło galtgendo@o2.pl 2013-08-03 15:18:34 CDT --- (In reply to comment #16)
"Windows GDI+ exposes a flat API that consists of about 600 functions, which are implemented in Gdiplus.dll and declared in Gdiplusflat.h. The functions in the GDI+ flat API are wrapped by a collection of about 40 C++ classes."
That's a quote from MSDN referenced above. Do you see a difference between flat gdiplus APIs and a collection of C++ wrapper classes?
You know, I can selectively read too. The next sentences are: "It is recommended that you do not directly call the functions in the flat API. Whenever you make calls to GDI+, you should do so by calling the methods and functions provided by the C++ wrappers. Microsoft Product Support Services will not provide support for code that calls the flat API directly."
So, it's a Microsoft *recommendation*, not a technical limitation, nevertheless the original upstream seen it the *other* way around.
http://bugs.winehq.org/show_bug.cgi?id=33378
--- Comment #18 from Dmitry Timoshkov dmitry@baikal.ru 2013-08-04 00:18:54 CDT --- (In reply to comment #17)
"Windows GDI+ exposes a flat API that consists of about 600 functions, which are implemented in Gdiplus.dll and declared in Gdiplusflat.h. The functions in the GDI+ flat API are wrapped by a collection of about 40 C++ classes."
That's a quote from MSDN referenced above. Do you see a difference between flat gdiplus APIs and a collection of C++ wrapper classes?
You know, I can selectively read too. The next sentences are: "It is recommended that you do not directly call the functions in the flat API. Whenever you make calls to GDI+, you should do so by calling the methods and functions provided by the C++ wrappers. Microsoft Product Support Services will not provide support for code that calls the flat API directly."
So, it's a Microsoft *recommendation*, not a technical limitation, nevertheless the original upstream seen it the *other* way around.
Using C++ gdiplus wrappers is a requirement for getting technical support from Microsoft, I don't think that Wine bugzilla counts as Microsoft support department.
http://bugs.winehq.org/show_bug.cgi?id=33378
--- Comment #19 from Rafał Mużyło galtgendo@o2.pl 2013-08-04 09:50:28 CDT --- FFS, this is getting off track fast.
My point is: awhile ago, a regression has been made it's been bisected to a particular commit a minimal revert has been proposed the revert has been later refined (here and on wine-devel) into a hypothesis of the origin of the problem based on the hypothesis an even smaller workaround has been offered (and sent to wine-patches)
yet the party that caused the regression still complains, not offering a better solution (and not offering any real help).
Well, 1.6 freeze muddled the situation a bit too, but that's a completely different matter.
https://mail.gnome.org/archives/gtk-devel-list/2013-April/msg00131.html (the above was between two gtk+ *devs*, but some of the sentiment holds)
I'm not saying original solution (pre-regression) wasn't a hack, cause it sort of was, just that the code was changed without taking into account why that hack was necessary.
If Microsoft pointed everybody to C++ classes, Point must have a very strict definition in terms of RectF and Width=Height=0 seems a natural one.
http://bugs.winehq.org/show_bug.cgi?id=33378
--- Comment #20 from Rafał Mużyło galtgendo@o2.pl 2013-08-04 12:02:03 CDT --- Just two minor things: - the latest version of my workaround is http://www.winehq.org/pipermail/wine-patches/2013-June/124783.html - in mingw, DrawString(const WCHAR *string, INT length, const Font *font, const PointF& origin, const Brush *brush) wrapper uses 'RectF layoutRect(origin.X, origin.Y, 0.0f, 0.0f);' as the argument of GdipDrawString call
http://bugs.winehq.org/show_bug.cgi?id=33378
--- Comment #21 from Bruno Jesus 00cpxxx@gmail.com 2013-08-15 13:23:20 CDT --- A patch related to this bug was commited: http://source.winehq.org/git/wine.git/?a=commit;h=814f9cf7e4af9afd91196b75c5...
http://bugs.winehq.org/show_bug.cgi?id=33378
--- Comment #22 from Rafał Mużyło galtgendo@o2.pl 2013-08-15 21:27:14 CDT --- (In reply to comment #21)
A patch related to this bug was commited: http://source.winehq.org/git/wine.git/?a=commit;h=814f9cf7e4af9afd91196b75c5...
Quite - when you're a senior dev, tests don't matter as much.
...
Well, at least it got in.
http://bugs.winehq.org/show_bug.cgi?id=33378
Dmitry Timoshkov dmitry@baikal.ru changed:
What |Removed |Added ---------------------------------------------------------------------------- Fixed by SHA1| |814f9cf7e4af9afd91196b75c58 | |1de792a239338 Status|UNCONFIRMED |RESOLVED Resolution| |FIXED
--- Comment #23 from Dmitry Timoshkov dmitry@baikal.ru 2013-08-15 21:50:48 CDT --- (In reply to comment #22)
(In reply to comment #21)
A patch related to this bug was commited: http://source.winehq.org/git/wine.git/?a=commit;h=814f9cf7e4af9afd91196b75c5...
Quite - when you're a senior dev, tests don't matter as much.
Quite the opposite: When you know what you're doing - tests don't matter much.
http://bugs.winehq.org/show_bug.cgi?id=33378
--- Comment #24 from Rafał Mużyło galtgendo@o2.pl 2013-08-16 01:02:42 CDT --- (In reply to comment #23)
(In reply to comment #22)
(In reply to comment #21)
A patch related to this bug was commited: http://source.winehq.org/git/wine.git/?a=commit;h=814f9cf7e4af9afd91196b75c5...
Quite - when you're a senior dev, tests don't matter as much.
Quite the opposite: When you know what you're doing - tests don't matter much.
Hmm... In the light of just whose regression this was (and that it wasn't the first regression in this code), this sort of makes the point of this bug.
http://bugs.winehq.org/show_bug.cgi?id=33378
--- Comment #25 from Dmitry Timoshkov dmitry@baikal.ru 2013-08-16 01:31:37 CDT --- (In reply to comment #24)
In the light of just whose regression this was (and that it wasn't the first regression in this code), this sort of makes the point of this bug.
Not that it matters much, but judging from the subject and comments in this report I don't see where you've got an idea that it's a regression.
And once again you either need to learn how to write gdiplus tests in plain C in order to demonstrate the problem, and be slightly more respectful to people who actually dedicate their own free time to work on Wine bugs.
http://bugs.winehq.org/show_bug.cgi?id=33378
--- Comment #26 from Rafał Mużyło galtgendo@o2.pl 2013-08-16 01:53:16 CDT --- (In reply to comment #25)
As the patch that got in was a result of *my* work (combined with a few hints from Vincent Povirk), it puts an odd spin on the things (and your comment).
As for why it was a regression, well obviously cause the things were working before a certain commit, where (just like in that gtk+ archive mail) somebody removed code cause it looked funny, without checking *why* it was funny.
Actually, you could argue, that the commit in question did the exact the opposite of what its message said it was supposed to do, at least in regard of GdipDrawString.
http://bugs.winehq.org/show_bug.cgi?id=33378
--- Comment #27 from Dmitry Timoshkov dmitry@baikal.ru 2013-08-16 01:59:56 CDT --- (In reply to comment #26)
As the patch that got in was a result of *my* work (combined with a few hints from Vincent Povirk), it puts an odd spin on the things (and your comment).
http://www.winehq.org/pipermail/wine-patches/2013-August/125803.html:
"Yes, this is almost byte-for-byte identical to what Rafał Mużyło sent earlier, but that's mostly because I told him this was how it should be done."
http://bugs.winehq.org/show_bug.cgi?id=33378
--- Comment #28 from Rafał Mużyło galtgendo@o2.pl 2013-08-16 04:20:13 CDT --- (In reply to comment #27)
You weren't a part of that irc discussion.
(I wonder if *this* "discussion" looks for the people not involved like a pissing contest too.)
http://bugs.winehq.org/show_bug.cgi?id=33378
--- Comment #29 from Rafał Mużyło galtgendo@o2.pl 2013-08-16 04:25:01 CDT --- Also, I kind of disagree with marking this as fixed.
The point of this bug was not quite to get this fix in, but rather stress that the current set of testcases is insufficient, as it wasn't first such a regression in this code. So, the fix would be adding at least a few testcases more.
http://bugs.winehq.org/show_bug.cgi?id=33378
--- Comment #30 from Dmitry Timoshkov dmitry@baikal.ru 2013-08-16 04:36:55 CDT --- (In reply to comment #29)
Also, I kind of disagree with marking this as fixed.
The point of this bug was not quite to get this fix in, but rather stress that the current set of testcases is insufficient, as it wasn't first such a regression in this code. So, the fix would be adding at least a few testcases more.
Please use forums to express an opinion about the tests or anything else, this is a bugzilla. Of course, you can start writing tests on your own, but I'm afraid that this suggestion may sound as a pissing contest to you.
http://bugs.winehq.org/show_bug.cgi?id=33378
--- Comment #31 from Rafał Mużyło galtgendo@o2.pl 2013-08-16 05:15:53 CDT --- Won by playing "*I* am the senior wine dev" card...again.
Sort of expected that.
Whatever...
http://bugs.winehq.org/show_bug.cgi?id=33378
Rafał Mużyło galtgendo@o2.pl changed:
What |Removed |Added ---------------------------------------------------------------------------- Resolution|FIXED |WONTFIX
http://bugs.winehq.org/show_bug.cgi?id=33378
Alexandre Julliard julliard@winehq.org changed:
What |Removed |Added ---------------------------------------------------------------------------- Resolution|WONTFIX |FIXED
--- Comment #32 from Alexandre Julliard julliard@winehq.org 2013-08-16 12:28:55 CDT --- The bug is fixed.
http://bugs.winehq.org/show_bug.cgi?id=33378
--- Comment #33 from Rafał Mużyło galtgendo@o2.pl 2013-08-16 19:48:50 CDT --- (In reply to comment #32)
The bug is fixed.
As I said, the regression itself was only a symptom of the problem, the problem being not thorough enough testsuite.
...but no point in arguing with who has the final word.
http://bugs.winehq.org/show_bug.cgi?id=33378
--- Comment #34 from Alexandre Julliard julliard@winehq.org 2013-08-17 02:56:18 CDT --- (In reply to comment #33)
(In reply to comment #32)
The bug is fixed.
As I said, the regression itself was only a symptom of the problem, the problem being not thorough enough testsuite.
You can make the same argument for every single bug. If we had tests for all possible behaviors, then clearly we'd never have bugs. That's not a practical goal though, or a useful use of Bugzilla.
http://bugs.winehq.org/show_bug.cgi?id=33378
--- Comment #35 from Rafał Mużyło galtgendo@o2.pl 2013-08-17 23:00:32 CDT --- (In reply to comment #34)
(In reply to comment #33)
You can make the same argument for every single bug. If we had tests for all possible behaviors, then clearly we'd never have bugs. That's not a practical goal though, or a useful use of Bugzilla.
To a point yes, a testsuite per its very definition can never be complete.
Here though, both 814f9cf7e4af9afd91196b75c581de792a239338 and those two my older regression fixes, that got in (fc2bb3bdc148f3071414cc9f1622fa1f0bc38518 and 48a2b48e16d751e0ddb72cb4e6729b943e018001) suggest that an additional testcase would be useful.
http://bugs.winehq.org/show_bug.cgi?id=33378
Alexandre Julliard julliard@winehq.org changed:
What |Removed |Added ---------------------------------------------------------------------------- Status|RESOLVED |CLOSED
--- Comment #36 from Alexandre Julliard julliard@winehq.org 2013-08-30 13:05:25 CDT --- Closing bugs fixed in 1.7.1.
http://bugs.winehq.org/show_bug.cgi?id=33378
Alexandre Julliard julliard@winehq.org changed:
What |Removed |Added ---------------------------------------------------------------------------- Target Milestone|--- |1.6.x
http://bugs.winehq.org/show_bug.cgi?id=33378
Alexandre Julliard julliard@winehq.org changed:
What |Removed |Added ---------------------------------------------------------------------------- Target Milestone|1.6.x |---
--- Comment #37 from Alexandre Julliard julliard@winehq.org 2013-11-15 13:39:55 CST --- Removing 1.6.x milestone from bugs included in 1.6.1.
-
wine-bugs＠winehq.org