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.

I did a test on windows, and it appears we need to skip clipping if the ORIGINAL width and height are <= 0.

(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.

(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.

Created attachment 44742 [details]
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.

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.

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.

Created attachment 45493 [details]
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.

...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.

(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.

(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.

...and looking briefly at bug 34102bug 34102, it most likely is a duplicate of this one.

(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.

(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).aspx.


> 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.

Created attachment 45495 [details]
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.

(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).aspx.

"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?

(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.

(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.

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.

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

A patch related to this bug was commited:
http://source.winehq.org/git/wine.git/?a=commit;h=814f9cf7e4af9afd91196b75c581de792a239338

(In reply to comment #21)
> A patch related to this bug was commited:
> http://source.winehq.org/git/wine.git/?a=commit;h=814f9cf7e4af9afd91196b75c581de792a239338

Quite - when you're a senior dev, tests don't matter as much.

...

Well, at least it got in.

(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=814f9cf7e4af9afd91196b75c581de792a239338
> 
> 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.

(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=814f9cf7e4af9afd91196b75c581de792a239338
> > 
> > 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.

(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.

(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.

(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."

(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.)

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.

(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.

Won by playing "*I* am the senior wine dev" card...again.

Sort of expected that.

Whatever...

The bug is fixed.

(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.

(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.

(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.

Closing bugs fixed in 1.7.1.

Removing 1.6.x milestone from bugs included in 1.6.1.