April 2017 - Wine-Devel - list.winehq.org

Re: [PATCH 2/4] winex11: move XDestroyWindow() after free_gl_drawable() in set_win_format()

by Alexandre Julliard

Miklós Máté <mtmkls(a)gmail.com> writes: > On 04/04/17 16:06, Alexandre Julliard wrote: >> Miklós Máté <mtmkls(a)gmail.com> writes: >> >>> On 04/04/17 12:11, Alexandre Julliard wrote: >>>> Miklós Máté <mtmkls(a)gmail.com> writes: >>>> >>>>> @@ -1334,21 +1334,21 @@ static void free_gl_drawable( struct gl_drawable *gl ) >>>>> /*********************************************************************** >>>>> * create_gl_drawable >>>>> */ >>>>> -static BOOL create_gl_drawable( HWND hwnd, struct gl_drawable *gl ) >>>>> +static BOOL create_gl_drawable( HWND hwnd, struct gl_drawable *gl, struct x11drv_win_data *data ) >>>>> { >>>>> gl->drawable = 0; >>>>> if (GetAncestor( hwnd, GA_PARENT ) == GetDesktopWindow()) >>>>> /* top-level window */ >>>>> { >>>>> - struct x11drv_win_data *data = get_win_data( hwnd ); >>>>> - >>>>> if (data) >>>>> { >>>>> gl->type = DC_GL_WINDOW; >>>>> gl->window = create_client_window( data, gl->visual ); >>>>> if (gl->window) >>>>> + { >>>>> gl->drawable = pglXCreateWindow( gdi_display, gl->format->fbconfig, gl->window, NULL ); >>>>> - release_win_data( data ); >>>>> + XSync( gdi_display, False ); >>>> Do you really need all the extra XSync calls? We try to avoid server >>>> round-trips as much as possible. >>>> >>> Yes. We must ensure that glXCreateWindow() and glXDestroyWindow() >>> refer to a valid X window, so the command queue of data->display and >>> gdi_display have to be in sync around those calls. >> If the goal is that the X window is valid, I'd have expected an XSync on >> the thread display. Why is the XSync on gdi_display needed here? What is >> the scenario that fails? >> >> I'm not questioning that some XSyncs are necessary (and we already have >> a few), but I want to make sure we don't add more than needed. >> > This one prevents crash in the following scenario: > > 1. create_gl_drawable() > > 2. X11DRV_ThreadDetach() calls XCloseDisplay( data->display ); > > 3. the Xserver processes the glXCreateWindow in the command queue of > gdi_display -> X window is not valid > > This happens sometimes during the ddraw and d3d9 tests. Shouldn't this be happening even without your other changes then? -- Alexandre Julliard julliard(a)winehq.org

April 5, 2017

Re: gdi32: Change default interface font to Gothic typeface for CJK.

by Huw Davies

> On 5 Apr 2017, at 15:03, Akihiro Sagawa <sagawa.aki(a)gmail.com> wrote: > >> On Tue, Apr 04, 2017 at 11:25:18PM +0800, Jactry Zeng wrote: >>> This patch changes default interface typeface of CJK from Mincho font to >>> Gothic font. >>> Windows had made this change since Windows Vista. macOS and most of Linux >>> distros are using Gothic typeface by default for CJK, too. >> >> This seems to be confirmed by the great oracle of truth that is Wikipedia. >> >> Signed-off-by: Huw Davies <huw(a)codeweavers.com> > > I'm not sure about this. On my Japanese Windows 7, MS Shell Dlg points > to MS UI Gothic, not Meiryo. If I remember correctly, it also points to > legacy Mincho font in Chinese/Korean environment. > > I'm afraid that this change also affects dialog base units (e.g. > GetDialogBaseUnits). A lot of application dialogs depend on default font's > dialog units. Though Windows 7 introduces new type faces, we should use > legacy font face in GDI for that reason. > > Instead of MS Shell Dlg, how about using "MS Shell Dlg 2" for system > dialog windows? Ok, so it sounds like this patch may not be correct. Jactry, could you tell us what MS Shell Dlg and MS Shell Dlg 2 map to on your system? Look under HKLM\\Software\\Microsoft\\Windows NT\\CurrentVersion\\FontSubstitutes Thanks, Huw.

April 5, 2017

Re: [PATCH 1/4] regedit: Concatenate the next line when a comment follows the backslash

by Alexandre Julliard

Hugh McMaster <hugh.mcmaster(a)outlook.com> writes: > @@ -715,7 +715,7 @@ static char *get_lineA(FILE *fp) > > while (next) > { > - char *p = strpbrk(line, "\r\n"); > + char *p = strpbrk(line, "\r\n"), *s; > if (!p) > { > size_t len, count; > @@ -741,10 +741,11 @@ static char *get_lineA(FILE *fp) > next = p + 1; > if (*p == '\r' && *(p + 1) == '\n') next++; > *p = 0; > - if (p > buf && *(p - 1) == '\\') > + if ((s = strchr(line, ':')) && (*(s - 1) == 'x' || *(s - 1) == ')')) > { > - while (*next == ' ' || *next == '\t') next++; > - memmove(p - 1, next, strlen(next) + 1); > + s = strchr(s, '\\'); > + if (!s) return line; > + memmove(s, next, strlen(next) + 1); This looks wrong. I assume it's meant to check for hex() and the like, but this would have to be done in the parser, not in the line reading code. There could just as well be a "hex:" inside a value string for instance. -- Alexandre Julliard julliard(a)winehq.org

April 5, 2017

Re: [PATCH 5/5] wined3d: Store render target info instead of surfaces in "blit_targets".

by Henri Verbeet

On 5 April 2017 at 15:38, Józef Kucia <jkucia(a)codeweavers.com> wrote: > @@ -2678,17 +2713,17 @@ BOOL context_apply_clear_state(struct wined3d_context *context, const struct win > > if (!rt_count || wined3d_resource_is_offscreen(rts[0]->resource)) > { > + memset(context->blit_targets, 0, gl_info->limits.buffers * sizeof(context->blit_targets)); Is that sizeof missing a '*'?

April 5, 2017

Re: [PATCH v2 1/5] wined3d: Save pixel and vertex shader resource views in stateblocks.

by Henri Verbeet

On 5 April 2017 at 15:38, Józef Kucia <jkucia(a)codeweavers.com> wrote: > @@ -3129,6 +3129,8 @@ struct wined3d_saved_states > WORD vertexShaderConstantsB; /* WINED3D_MAX_CONSTS_B, 16 */ > WORD vertexShaderConstantsI; /* WINED3D_MAX_CONSTS_I, 16 */ > BOOL vs_consts_f[WINED3D_MAX_VS_CONSTS_F]; > + DWORD pixel_shader_resource_views : 16; /* MAX_FRAGMENT_SAMPLERS, 16 */ > + DWORD vertex_shader_resource_views : 4; /* MAX_VERTEX_SAMPLERS, 4 */ > DWORD textures : 20; /* MAX_COMBINED_SAMPLERS, 20 */ > DWORD indices : 1; > DWORD material : 1; > @@ -3137,17 +3139,18 @@ struct wined3d_saved_states > DWORD pixelShader : 1; > DWORD vertexShader : 1; > DWORD scissorRect : 1; > - DWORD padding : 5; > + DWORD padding : 17; That doesn't do the right thing, unfortunately: struct wined3d_saved_states { DWORD transform[16]; /* 0 64 */ /* --- cacheline 1 boundary (64 bytes) --- */ WORD streamSource; /* 64 2 */ WORD streamFreq; /* 66 2 */ DWORD renderState[7]; /* 68 28 */ DWORD textureState[8]; /* 96 32 */ /* --- cacheline 2 boundary (128 bytes) --- */ WORD samplerState[20]; /* 128 40 */ DWORD clipplane; /* 168 4 */ WORD pixelShaderConstantsB; /* 172 2 */ WORD pixelShaderConstantsI; /* 174 2 */ BOOL ps_consts_f[224]; /* 176 896 */ /* --- cacheline 16 boundary (1024 bytes) was 48 bytes ago --- */ WORD vertexShaderConstantsB; /* 1072 2 */ WORD vertexShaderConstantsI; /* 1074 2 */ BOOL vs_consts_f[256]; /* 1076 1024 */ /* --- cacheline 32 boundary (2048 bytes) was 52 bytes ago --- */ DWORD pixel_shader_resource_views:16; /* 2100:16 4 */ DWORD vertex_shader_resource_views:4; /* 2100:12 4 */ /* XXX 12 bits hole, try to pack */ DWORD textures:20; /* 2104:12 4 */ DWORD indices:1; /* 2104:11 4 */ DWORD material:1; /* 2104:10 4 */ DWORD viewport:1; /* 2104: 9 4 */ DWORD vertexDecl:1; /* 2104: 8 4 */ DWORD pixelShader:1; /* 2104: 7 4 */ DWORD vertexShader:1; /* 2104: 6 4 */ DWORD scissorRect:1; /* 2104: 5 4 */ /* XXX 5 bits hole, try to pack */ DWORD padding:17; /* 2108:15 4 */ /* --- cacheline 33 boundary (2112 bytes) --- */ /* size: 2112, cachelines: 33, members: 24 */ /* bit holes: 2, sum bit holes: 17 bits */ /* bit_padding: 15 bits */ }; Incidentally, this also illustrates the issue of the ps_consts_f and vs_consts_f fields using up way more space than needed.

April 5, 2017

Re: [PATCH 1/6] comctl32/tests: Don't dump successful sequences on Windows.

by Francois Gouget

On Wed, 5 Apr 2017, Nikolay Sivov wrote: [...] > This patch is marked is failed by testbot, but it looks like bot > inability to deal with header file change in tests directory, as it > still tries to run some tests. Yes, that's bug 31789 which should be fixed now (I'll update Bugzilla once that's confirmed). https://bugs.winehq.org/show_bug.cgi?id=31789 I committed the fix a bit before the patchset but part of the fix applies to code that's checked out on the build VM and that is not updated automatically :-( So I updated the VM and things would work fine after the next 'Update Wine to latest git' job is run. And I'll update the TestBot so the checkout on the build VM is updated automatically. -- Francois Gouget <fgouget(a)codeweavers.com>

April 5, 2017

Re: gdi32: Change default interface font to Gothic typeface for CJK.

by Akihiro Sagawa

> On Tue, Apr 04, 2017 at 11:25:18PM +0800, Jactry Zeng wrote: > > This patch changes default interface typeface of CJK from Mincho font to > > Gothic font. > > Windows had made this change since Windows Vista. macOS and most of Linux > > distros are using Gothic typeface by default for CJK, too. > > This seems to be confirmed by the great oracle of truth that is Wikipedia. > > Signed-off-by: Huw Davies <huw(a)codeweavers.com> I'm not sure about this. On my Japanese Windows 7, MS Shell Dlg points to MS UI Gothic, not Meiryo. If I remember correctly, it also points to legacy Mincho font in Chinese/Korean environment. I'm afraid that this change also affects dialog base units (e.g. GetDialogBaseUnits). A lot of application dialogs depend on default font's dialog units. Though Windows 7 introduces new type faces, we should use legacy font face in GDI for that reason. Instead of MS Shell Dlg, how about using "MS Shell Dlg 2" for system dialog windows? Regards, Akihiro Sagawa

April 5, 2017

Re: [PATCH 2/6] comctl32/tests: Test messages sent to the pager child.

by Stefan Dösinger

Am 2017-04-05 um 14:53 schrieb Nikolay Sivov: > Maybe I don't see a full picture yet, but doesn't this duplicate 'id' > flag? We use it for ListView/Header combined sequences for example. It's not exactly the same, "id" is more a generic extra payload field. The pager test e.g. uses it to communicate a message id in a WM_NOTIFY message. In the tests I currently have no extra payload, so I could use the id mechanism instead of extending the parent / control flag. I could also use it in patch 6 to deal with the WINDOWPOS structures. But I'll need both child1/child2 info and WINDOWPOS in the same lines, so I can't do both.

April 5, 2017

Re: [PATCH 1/6] comctl32/tests: Don't dump successful sequences on Windows.

by Stefan Dösinger

Am 2017-04-05 um 14:36 schrieb Nikolay Sivov: > This patch is marked is failed by testbot, but it looks like bot > inability to deal with header file change in tests directory, as it > still tries to run some tests. Yeah, I think so, especially since the follow-up patches run ok. > Anyway, could you make it look similar to user32/tests/msg.c? It's using: > > --- > if( todo && !failcount) /* succeeded yet marked todo */ > todo_wine { > if (!strcmp(winetest_platform, "wine")) dump++; > --- > > Platform is tested directly. It's not the only place where it's done > like that, and it's more readable than broken(1), I think. Sure, I'll change it :-)

April 5, 2017

Re: [PATCH 2/6] comctl32/tests: Test messages sent to the pager child.

by Nikolay Sivov

On 05.04.2017 10:15, Stefan Dösinger wrote: > --- > dlls/comctl32/tests/msg.h | 19 ++++++++++-- > dlls/comctl32/tests/pager.c | 70 ++++++++++++++++++++++++++++++++++++++++++--- > 2 files changed, 83 insertions(+), 6 deletions(-) > > diff --git a/dlls/comctl32/tests/msg.h b/dlls/comctl32/tests/msg.h > index b30b5d8..a3c789b 100644 > --- a/dlls/comctl32/tests/msg.h > +++ b/dlls/comctl32/tests/msg.h > @@ -39,7 +39,8 @@ typedef enum > hook = 0x100, > winevent_hook =0x200, > id = 0x400, > - custdraw = 0x800 > + custdraw = 0x800, > + child1 = 0x1000, > } msg_flags_t; > Maybe I don't see a full picture yet, but doesn't this duplicate 'id' flag? We use it for ListView/Header combined sequences for example. > struct message > @@ -182,6 +183,8 @@ static void ok_sequence_(struct msg_sequence **seq, int sequence_index, > { > if (expected->message == actual->message) > { > + const char *expected_win = NULL; > + > if (expected->flags & wparam) > { > if (expected->wParam != actual->wParam && todo) > @@ -304,11 +307,23 @@ static void ok_sequence_(struct msg_sequence **seq, int sequence_index, > context, expected->message, (expected->flags & posted) ? "posted" : "sent"); > if ((expected->flags & (sent|posted)) != (actual->flags & (sent|posted))) dump++; > > + if (expected->flags & parent) > + expected_win = "parent"; > + else if (expected->flags & child1) > + expected_win = "child 1"; > + else > + expected_win = "pager"; This has to be generic, something neutral like "control" instead of "pager" is better, if we'll end up using new flag.

April 5, 2017