Re: [PATCH 3/3] user32: Fix groupbox rectangle calculation in the button's WM_SETTEXT handler.

[Dmitry Timoshkov]

Nikolay Sivov  wrote:
> -
>        /* FIXME: check other BS_* handlers */
> -
>        if (btn_type == BS_GROUPBOX)
> -
>            InflateRect(&rc, -7, 1); /* GB_Paint does this */

How about doing what FIXME suggests? Adding some tests wouldn't hurt either.

[PATCH 3/3] user32: Fix groupbox rectangle calculation in the button's WM_SETTEXT handler.

[Nikolay Sivov]

On 06.02.2017 7:34, Dmitry Timoshkov wrote:
> Nikolay Sivov  wrote:
> > -
> >        /* FIXME: check other BS_* handlers */
> > -
> >        if (btn_type == BS_GROUPBOX)
> > -
> >            InflateRect(&rc, -7, 1); /* GB_Paint does this */
>
> How about doing what FIXME suggests?

Sure, at some point. Original report is for group box, fix is limited to it.
> Adding some tests wouldn't hurt either.

Test what, filled rectangle size? I don't think it makes sense.

[PATCH 3/3] user32: Fix groupbox rectangle calculation in the button's WM_SETTEXT handler.

[Dmitry Timoshkov]

Nikolay Sivov  wrote:
> > > -
> > >        /* FIXME: check other BS_* handlers */
> > > -
> > >        if (btn_type == BS_GROUPBOX)
> > > -
> > >            InflateRect(&rc, -7, 1); /* GB_Paint does this */
> >
> > How about doing what FIXME suggests?
>
> Sure, at some point. Original report is for group box, fix is limited to it.

Yes, my original patch is only for BS_GROUPBOX buttons, but that doesn't
mean that other button style handlers don't have similar bugs. There are
not that much of button styles to carfully review and fix if required,
and if for some reason you've decided to resubmit the staged patches
you should be prepared to spend time to improve the original patches,
in this particular case take care of the FIXME.
> > Adding some tests wouldn't hurt either.
>
> Test what, filled rectangle size? I don't think it makes sense.

This patchset fixes several things, and each of them deserves its own
testing. For instance testing an original font selected into DC should
be easy enough.

There are multiple reasons why staged patches are not sent directly to
wine-patches, and one of them is that they need more work. If you are
not ready to do that part of the work (except of just copy/pasting an
original patch) then you shouldn't touch the staged patches.

[PATCH 3/3] user32: Fix groupbox rectangle calculation in the button's WM_SETTEXT handler.

[Nikolay Sivov]

On 06.02.2017 9:55, Dmitry Timoshkov wrote:
> Nikolay Sivov  wrote:
> > > > -
> > > >        /* FIXME: check other BS_* handlers */
> > > > -
> > > >        if (btn_type == BS_GROUPBOX)
> > > > -
> > > >            InflateRect(&rc, -7, 1); /* GB_Paint does this */
> > >
> > > How about doing what FIXME suggests?
> >
> > Sure, at some point. Original report is for group box, fix is limited to it.
>
> Yes, my original patch is only for BS_GROUPBOX buttons, but that doesn't
> mean that other button style handlers don't have similar bugs. There are
> not that much of button styles to carfully review and fix if required,

It doesn't, it also doesn't mean they do. What's important is that it
doesn't break other style.
> and if for some reason you've decided to resubmit the staged patches
> you should be prepared to spend time to improve the original patches,
> in this particular case take care of the FIXME.

I worked on this bug during code freeze and have a fix attached there,
before your set was linked. So I consider mine as original, and yours as
improved ones touching the same issue, whether it was based on mine, or
developed separately. That "some reason" you mentioned is that I already
spent time on it while working on the report.

[PATCH 3/3] user32: Fix groupbox rectangle calculation in the button's WM_SETTEXT handler.

[Dmitry Timoshkov]

Nikolay Sivov  wrote:
> > > > > -
> > > > >        /* FIXME: check other BS_* handlers */
> > > > > -
> > > > >        if (btn_type == BS_GROUPBOX)
> > > > > -
> > > > >            InflateRect(&rc, -7, 1); /* GB_Paint does this */
> > > >
> > > > How about doing what FIXME suggests?
> > >
> > > Sure, at some point. Original report is for group box, fix is limited to it.
> >
> > Yes, my original patch is only for BS_GROUPBOX buttons, but that doesn't
> > mean that other button style handlers don't have similar bugs. There are
> > not that much of button styles to carfully review and fix if required,
>
> It doesn't, it also doesn't mean they do. What's important is that it
> doesn't break other style.

Wow, that's really nice to see such an "explanation". Please be prepared
to accept similar excuses for the things you are complaining about in your
(sometimes extremely picky) reviews.
> > and if for some reason you've decided to resubmit the staged patches
> > you should be prepared to spend time to improve the original patches,
> > in this particular case take care of the FIXME.
>
> I worked on this bug during code freeze and have a fix attached there,
> before your set was linked. So I consider mine as original, and yours as
> improved ones touching the same issue, whether it was based on mine, or
> developed separately. That "some reason" you mentioned is that I already
> spent time on it while working on the report.

I'll copy the relevant history from https://bugs.winehq.org/show_activity.cgi?id=41830
for your convenience:

Who 	When 	What 	Removed 	Added
[e-mail] 	2016-11-26 23:13:56 CST 	Component 	user32 	-unknown
[e-mail] 	2016-11-29 13:32:14 CST 	Component 	-unknown 	user32
[e-mail] 	2016-12-18 16:59:25 CST 	Ever confirmed 	0 	1
Assignee 	[e-mail] 	[e-mail]
Status 	UNCONFIRMED 	NEW
[e-mail] 	2016-12-18 16:59:38 CST 	Keywords 		download
[e-mail] 	2016-12-21 03:31:00 CST 	Staged patchset
Status 	NEW 	STAGED

and the dates from the staged patchset:
Date: Sun, 27 Nov 2016 14:00:12 +0800
Date: Sun, 27 Nov 2016 14:01:58 +0800
Date: Sun, 27 Nov 2016 14:07:12 +0800

so that everyone else can see the "progress" of your understading of that bug,
and see what is based on what. After all for some reason you've decided to
send the staged patches instead of your own work.

P.S.
Probably you should stop asking the authors of the patches you review
at wine-patches to add the tests or improve somehow their work if you
are not following your own development suggestions anyway.

[PATCH 3/3] user32: Fix groupbox rectangle calculation in the button's WM_SETTEXT handler.

[Sebastian Lackner]

On 06.02.2017 07:55, Dmitry Timoshkov wrote:
> Nikolay Sivov  wrote:
> > > > -
> > > >        /* FIXME: check other BS_* handlers */
> > > > -
> > > >        if (btn_type == BS_GROUPBOX)
> > > > -
> > > >            InflateRect(&rc, -7, 1); /* GB_Paint does this */
> > >
> > > How about doing what FIXME suggests?
> >
> > Sure, at some point. Original report is for group box, fix is limited to it.
>
> Yes, my original patch is only for BS_GROUPBOX buttons, but that doesn't
> mean that other button style handlers don't have similar bugs. There are
> not that much of button styles to carfully review and fix if required,
> and if for some reason you've decided to resubmit the staged patches
> you should be prepared to spend time to improve the original patches,
> in this particular case take care of the FIXME.
> > > Adding some tests wouldn't hurt either.
> >
> > Test what, filled rectangle size? I don't think it makes sense.
>
> This patchset fixes several things, and each of them deserves its own
> testing. For instance testing an original font selected into DC should
> be easy enough.
>
> There are multiple reasons why staged patches are not sent directly to
> wine-patches, and one of them is that they need more work. If you are
> not ready to do that part of the work (except of just copy/pasting an
> original patch) then you shouldn't touch the staged patches.

It is good that you are pointing out possible issues, however it doesn't
make much sense to be too picky. What Nikolay is doing here is absolutely
fine, and I appreciate the effort to help with upstreaming. Unless you
are planning to send an improved version yourself in the near future, I
do not see why such a partial fix should not be accepted - especially when
its not a hack (other paint handlers already restore the font, for example)
and reviewers are fine with it.

For what its worth, the handler for BS_DEFPUSHBUTTON might be affected
by a similar problem. No big deal to send this change separately though.

Best regards,
Sebastian