May 2016 - Wine-Devel - list.winehq.org

Re: comdlg32: Use SetRect() instead of open coding it.

by Frédéric Delanoy

On Wed, May 4, 2016 at 10:55 AM, Dmitry Timoshkov <dmitry(a)baikal.ru> wrote: > Frédéric Delanoy <frederic.delanoy(a)gmail.com> wrote: > >> - cancel_rc.left = dialog_rc.right - cancel_width - hspacing; >> - cancel_rc.top = dialog_rc.bottom - cancel_height - vspacing; >> - cancel_rc.right = cancel_rc.left + cancel_width; >> - cancel_rc.bottom = cancel_rc.top + cancel_height; >> + SetRect(&cancel_rc, >> + dialog_rc.right - cancel_width - hspacing, dialog_rc.bottom - cancel_height - vspacing, >> + dialog_rc.right - hspacing , dialog_rc.bottom - vspacing); > > This is at least very strange and weird formatting of an API call. > I don't see anything like that in the changed file. > > -- > Dmitry. It's for increased readability. Of course, it doesn't show well in proportional fonts like those used in emails. -- Frédéric

May 4, 2016

Re: msctf: Improved ITfKeystrokeMgr::TestKeyUp and TestKeyDown stubs.

by Marvin

Hi, While running your changed tests on Windows, I think I found new failures. Being a bot and all I'm not very good at pattern recognition, so I might be wrong, but could you please double-check? Full results can be found at https://testbot.winehq.org/JobDetails.pl?Key=22836 Your paranoid android. === wvistau64 (32 bit inputprocessor) === inputprocessor.c:254: Test failed: Unexpected TextStoreACP_RequestLock sink inputprocessor.c:275: Test failed: Unexpected TextStoreACP_GetSelection sink inputprocessor.c:1241: Test failed: eaten = 1 === w2008s64 (32 bit inputprocessor) === inputprocessor.c:254: Test failed: Unexpected TextStoreACP_RequestLock sink inputprocessor.c:275: Test failed: Unexpected TextStoreACP_GetSelection sink inputprocessor.c:1241: Test failed: eaten = 1 === w864 (32 bit inputprocessor) === inputprocessor.c:793: Test failed: Deactivation came unexpectedly === wvistau64 (64 bit inputprocessor) === inputprocessor.c:254: Test failed: Unexpected TextStoreACP_RequestLock sink inputprocessor.c:275: Test failed: Unexpected TextStoreACP_GetSelection sink inputprocessor.c:1241: Test failed: eaten = 1 === w2008s64 (64 bit inputprocessor) === inputprocessor.c:254: Test failed: Unexpected TextStoreACP_RequestLock sink inputprocessor.c:275: Test failed: Unexpected TextStoreACP_GetSelection sink inputprocessor.c:1241: Test failed: eaten = 1

May 4, 2016

Re: winex11.drv: Use IsRectEmpty() instead of open coding it.

by Michael Stefaniuc

On 05/04/2016 10:28 AM, Michael Stefaniuc wrote: > On 05/04/2016 10:15 AM, Alexandre Julliard wrote: >> Ken Thomases <ken(a)codeweavers.com> writes: >> >>> On May 4, 2016, at 2:58 AM, Alexandre Julliard <julliard(a)winehq.org> wrote: >>>> >>>> Michael Stefaniuc <mstefani(a)redhat.de> writes: >>>> >>>>> @@ -134,7 +134,7 @@ static inline void reset_bounds( RECT *bounds ) >>>>> >>>>> static inline void add_bounds_rect( RECT *bounds, const RECT *rect ) >>>>> { >>>>> - if (rect->left >= rect->right || rect->top >= rect->bottom) return; >>>>> + if (IsRectEmpty(rect)) return; >>>> >>>> I'm not sure it's an improvement to add a function call in such a simple >>>> inline function. >>> >>> All of these sorts of patches have made me wonder if these >>> RECT-related utility functions shouldn't themselves be defined static >>> inline in the headers (as well as having real versions for export). >> >> If we are making heavy use of them, then yes, that would probably be >> worth it. > Yes, I have thought of that already. > But the amount of patches and other developers requesting it makes for > the better "more convincing arguments" ;-P And here are the hard numbers for wine-1.9.9-67-g9eaa372, give or take: wine$ git grep -w SetRect | wc -l 566 wine$ git grep -w SetRectEmpty | wc -l 123 wine$ git grep -w IsRectEmpty | wc -l 89 I still have some more SetRect() calls to add, should be around 200-300. SetRectEmpty() I see only 3 more in the generated diff. And for IsRectEmpty() I have submitted everything. bye michael

May 4, 2016

Re: comdlg32: Use SetRect() instead of open coding it.

by Dmitry Timoshkov

Frédéric Delanoy <frederic.delanoy(a)gmail.com> wrote: > - cancel_rc.left = dialog_rc.right - cancel_width - hspacing; > - cancel_rc.top = dialog_rc.bottom - cancel_height - vspacing; > - cancel_rc.right = cancel_rc.left + cancel_width; > - cancel_rc.bottom = cancel_rc.top + cancel_height; > + SetRect(&cancel_rc, > + dialog_rc.right - cancel_width - hspacing, dialog_rc.bottom - cancel_height - vspacing, > + dialog_rc.right - hspacing , dialog_rc.bottom - vspacing); This is at least very strange and weird formatting of an API call. I don't see anything like that in the changed file. -- Dmitry.

May 4, 2016

Re: winex11.drv: Use IsRectEmpty() instead of open coding it.

by Michael Stefaniuc

On 05/04/2016 10:15 AM, Alexandre Julliard wrote: > Ken Thomases <ken(a)codeweavers.com> writes: > >> On May 4, 2016, at 2:58 AM, Alexandre Julliard <julliard(a)winehq.org> wrote: >>> >>> Michael Stefaniuc <mstefani(a)redhat.de> writes: >>> >>>> @@ -134,7 +134,7 @@ static inline void reset_bounds( RECT *bounds ) >>>> >>>> static inline void add_bounds_rect( RECT *bounds, const RECT *rect ) >>>> { >>>> - if (rect->left >= rect->right || rect->top >= rect->bottom) return; >>>> + if (IsRectEmpty(rect)) return; >>> >>> I'm not sure it's an improvement to add a function call in such a simple >>> inline function. >> >> All of these sorts of patches have made me wonder if these >> RECT-related utility functions shouldn't themselves be defined static >> inline in the headers (as well as having real versions for export). > > If we are making heavy use of them, then yes, that would probably be > worth it. Yes, I have thought of that already. But the amount of patches and other developers requesting it makes for the better "more convincing arguments" ;-P bye michael

May 4, 2016

Re: winex11.drv: Use IsRectEmpty() instead of open coding it.

by Alexandre Julliard

Ken Thomases <ken(a)codeweavers.com> writes: > On May 4, 2016, at 2:58 AM, Alexandre Julliard <julliard(a)winehq.org> wrote: >> >> Michael Stefaniuc <mstefani(a)redhat.de> writes: >> >>> @@ -134,7 +134,7 @@ static inline void reset_bounds( RECT *bounds ) >>> >>> static inline void add_bounds_rect( RECT *bounds, const RECT *rect ) >>> { >>> - if (rect->left >= rect->right || rect->top >= rect->bottom) return; >>> + if (IsRectEmpty(rect)) return; >> >> I'm not sure it's an improvement to add a function call in such a simple >> inline function. > > All of these sorts of patches have made me wonder if these > RECT-related utility functions shouldn't themselves be defined static > inline in the headers (as well as having real versions for export). If we are making heavy use of them, then yes, that would probably be worth it. -- Alexandre Julliard julliard(a)winehq.org

May 4, 2016

Re: [PATCH 2/4] winemac.drv: Move create_bitmap_from_dib() above the import/export functions.

by Ken Thomases

Signed-off-by: Ken Thomases <ken(a)codeweavers.com>

May 4, 2016

Re: winex11.drv: Use IsRectEmpty() instead of open coding it.

by Ken Thomases

On May 4, 2016, at 2:58 AM, Alexandre Julliard <julliard(a)winehq.org> wrote: > > Michael Stefaniuc <mstefani(a)redhat.de> writes: > >> @@ -134,7 +134,7 @@ static inline void reset_bounds( RECT *bounds ) >> >> static inline void add_bounds_rect( RECT *bounds, const RECT *rect ) >> { >> - if (rect->left >= rect->right || rect->top >= rect->bottom) return; >> + if (IsRectEmpty(rect)) return; > > I'm not sure it's an improvement to add a function call in such a simple > inline function. All of these sorts of patches have made me wonder if these RECT-related utility functions shouldn't themselves be defined static inline in the headers (as well as having real versions for export). -Ken

May 4, 2016

Re: winex11.drv: Use IsRectEmpty() instead of open coding it.

by Alexandre Julliard

Michael Stefaniuc <mstefani(a)redhat.de> writes: > @@ -134,7 +134,7 @@ static inline void reset_bounds( RECT *bounds ) > > static inline void add_bounds_rect( RECT *bounds, const RECT *rect ) > { > - if (rect->left >= rect->right || rect->top >= rect->bottom) return; > + if (IsRectEmpty(rect)) return; I'm not sure it's an improvement to add a function call in such a simple inline function. -- Alexandre Julliard julliard(a)winehq.org

May 4, 2016

Re: winemac.drv: Use IsRectEmpty() instead of open coding it.

by Ken Thomases

On May 3, 2016, at 2:47 PM, Michael Stefaniuc <mstefani(a)redhat.de> wrote: > > Signed-off-by: Michael Stefaniuc <mstefani(a)redhat.de> > --- > Not even compile tested. But as this is a purely generated patch there > was no opportunity for me to screw it up... In fact, it doesn't compile. :-/ You've added a call to IsRectEmpty() to macdrv.h, which doesn't #include winuser.h. That alone only causes an implicit declaration warning. However, clipboard.c includes winuser.h after macdrv.h, which causes an error about the conflict between the real and implicit declarations. You can move the include of winuser.h from all of the modules to macdrv.h, but make sure to #define OEMRESOURCE before it, as required by mouse.c. It also seems to need to come after the include of wingdi.h. -Ken > > > > dlls/winemac.drv/macdrv.h | 2 +- > dlls/winemac.drv/window.c | 2 +- > 2 files changed, 2 insertions(+), 2 deletions(-) > > diff --git a/dlls/winemac.drv/macdrv.h b/dlls/winemac.drv/macdrv.h > index f3b5e65..81b642c 100644 > --- a/dlls/winemac.drv/macdrv.h > +++ b/dlls/winemac.drv/macdrv.h > @@ -47,7 +47,7 @@ > > static inline CGRect cgrect_from_rect(RECT rect) > { > - if (rect.left >= rect.right || rect.top >= rect.bottom) > + if (IsRectEmpty(&rect)) > return CGRectMake(rect.left, rect.top, 0, 0); > return CGRectMake(rect.left, rect.top, rect.right - rect.left, rect.bottom - rect.top); > } > diff --git a/dlls/winemac.drv/window.c b/dlls/winemac.drv/window.c > index a48ac17..fbea49f 100644 > --- a/dlls/winemac.drv/window.c > +++ b/dlls/winemac.drv/window.c > @@ -401,7 +401,7 @@ static void sync_window_region(struct macdrv_win_data *data, HRGN win_region) > */ > static inline void add_bounds_rect(RECT *bounds, const RECT *rect) > { > - if (rect->left >= rect->right || rect->top >= rect->bottom) return; > + if (IsRectEmpty(rect)) return; > bounds->left = min(bounds->left, rect->left); > bounds->top = min(bounds->top, rect->top); > bounds->right = max(bounds->right, rect->right); > -- > 2.4.11 > >

May 4, 2016