Re: [PATCH 3/3] user32: Fix groupbox rectangle calculation in the button's WM_SETTEXT handler.
Nikolay Sivov <nsivov(a)codeweavers.com> 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. -- Dmitry.
On 06.02.2017 7:34, Dmitry Timoshkov wrote:
Nikolay Sivov <nsivov(a)codeweavers.com> 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.
Nikolay Sivov <bunglehead(a)gmail.com> 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. -- Dmitry.
On 06.02.2017 9:55, Dmitry Timoshkov wrote:
Nikolay Sivov <bunglehead(a)gmail.com> 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.
Nikolay Sivov <bunglehead(a)gmail.com> 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 bunglehead(a)gmail.com 2016-11-26 23:13:56 CST Component user32 -unknown james.tabor(a)reactos.org 2016-11-29 13:32:14 CST Component -unknown user32 bunglehead(a)gmail.com 2016-12-18 16:59:25 CST Ever confirmed 0 1 Assignee wine-bugs(a)winehq.org bunglehead(a)gmail.com Status UNCONFIRMED NEW bunglehead(a)gmail.com 2016-12-18 16:59:38 CST Keywords download sebastian(a)fds-team.de 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. -- Dmitry.
On 06.02.2017 07:55, Dmitry Timoshkov wrote:
Nikolay Sivov <bunglehead(a)gmail.com> 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
participants (3)
-
Dmitry Timoshkov -
Nikolay Sivov -
Sebastian Lackner