[PATCH v2] winex11.drv: Handle Vulkan surface creation with NULL hwnd.

[Paul Gofman]

Signed-off-by: Paul Gofman 
---
    For Red Dead Redemption 2.
v2:
    - store hwnd in struct wine_vk_surface instead of a dedicated flag.

dlls/vulkan-1/tests/vulkan.c | 112 ++++++++++++++++++++++++++++++++++-
 dlls/winex11.drv/vulkan.c    |  38 +++++++++---
 dlls/winex11.drv/window.c    |  20 +++++++
 dlls/winex11.drv/x11drv.h    |   1 +
 4 files changed, 160 insertions(+), 11 deletions(-)

diff --git a/dlls/vulkan-1/tests/vulkan.c b/dlls/vulkan-1/tests/vulkan.c
index 9061b2b6db8..e516d0d61c8 100644
--- a/dlls/vulkan-1/tests/vulkan.c
+++ b/dlls/vulkan-1/tests/vulkan.c
@@ -437,7 +437,102 @@ static void test_private_data(VkPhysicalDevice vk_physical_device)
     vkDestroyDevice(vk_device, NULL);
 }

-static void for_each_device(void (*test_func)(VkPhysicalDevice))
+static const char *test_null_hwnd_extensions[] =
+{
+    "VK_KHR_surface",
+    "VK_KHR_win32_surface",
+};
+
+static void test_null_hwnd(VkInstance vk_instance, VkPhysicalDevice vk_physical_device)
+{
+    PFN_vkGetPhysicalDeviceSurfacePresentModesKHR pvkGetPhysicalDeviceSurfacePresentModesKHR;
+    VkDeviceGroupPresentModeFlagsKHR present_mode_flags;
+    VkWin32SurfaceCreateInfoKHR surface_create_info;
+    VkSurfaceCapabilitiesKHR surf_caps;
+    VkSurfaceFormatKHR *formats;
+    uint32_t queue_family_index;
+    VkPresentModeKHR *modes;
+    VkSurfaceKHR surface;
+    VkDevice vk_device;
+    uint32_t count;
+    VkRect2D rect;
+    VkBool32 bval;
+    VkResult vr;
+
+    pvkGetPhysicalDeviceSurfacePresentModesKHR = (void *)vkGetInstanceProcAddr(vk_instance,
+            "vkGetPhysicalDeviceSurfacePresentModesKHR");
+    surface_create_info.sType = VK_STRUCTURE_TYPE_WIN32_SURFACE_CREATE_INFO_KHR;
+    surface_create_info.pNext = NULL;
+    surface_create_info.flags = 0;
+    surface_create_info.hinstance = NULL;
+    surface_create_info.hwnd = NULL;
+
+    bval = find_queue_family(vk_physical_device, VK_QUEUE_GRAPHICS_BIT, &queue_family_index);
+    ok(bval, "Could not find presentation queue.\n");
+
+    surface = 0xdeadbeef;
+    vr = vkCreateWin32SurfaceKHR(vk_instance, &surface_create_info, NULL, &surface);
+    ok(vr == VK_SUCCESS, "Got unexpected vr %d.\n", vr);
+    ok(surface != 0xdeadbeef, "Surface not created.\n");
+
+    count = 0;
+    vr = vkGetPhysicalDeviceSurfaceFormatsKHR(vk_physical_device, surface, &count, NULL);
+    ok(vr == VK_SUCCESS, "Got unexpected vr %d.\n", vr);
+    ok(count, "Got zero count.\n");
+    formats = heap_alloc(sizeof(*formats) * count);
+    vr = vkGetPhysicalDeviceSurfaceFormatsKHR(vk_physical_device, surface, &count, formats);
+    ok(vr == VK_SUCCESS, "Got unexpected vr %d.\n", vr);
+    heap_free(formats);
+
+    vr = vkGetPhysicalDeviceSurfaceSupportKHR(vk_physical_device, queue_family_index, surface, &bval);
+    ok(vr == VK_SUCCESS, "Got unexpected vr %d.\n", vr);
+
+    vr = vkGetPhysicalDeviceSurfaceCapabilitiesKHR(vk_physical_device, surface, &surf_caps);
+    ok(vr, "vkGetPhysicalDeviceSurfaceCapabilitiesKHR succeeded.\n");
+
+    count = 0;
+    vr = pvkGetPhysicalDeviceSurfacePresentModesKHR(vk_physical_device, surface, &count, NULL);
+    ok(vr == VK_SUCCESS, "Got unexpected vr %d.\n", vr);
+    ok(count, "Got zero count.\n");
+    modes = heap_alloc(sizeof(*modes) * count);
+    vr = pvkGetPhysicalDeviceSurfacePresentModesKHR(vk_physical_device, surface, &count, modes);
+    ok(vr == VK_SUCCESS, "Got unexpected vr %d.\n", vr);
+    heap_free(modes);
+
+    count = 0;
+    vr = vkGetPhysicalDevicePresentRectanglesKHR(vk_physical_device, surface, &count, NULL);
+    ok(vr == VK_SUCCESS, "Got unexpected vr %d.\n", vr);
+    ok(count == 1, "Got unexpected count %u.\n", count);
+    memset(&rect, 0xcc, sizeof(rect));
+    vr = vkGetPhysicalDevicePresentRectanglesKHR(vk_physical_device, surface, &count, &rect);
+    if (vr == VK_SUCCESS) /* Fails on AMD, succeeds on Nvidia. */
+    {
+        ok(count == 1, "Got unexpected count %u.\n", count);
+        ok(!rect.offset.x && !rect.offset.y && !rect.extent.width && !rect.extent.height,
+                "Got unexpected rect %d, %d, %u, %u.\n",
+                rect.offset.x, rect.offset.y, rect.extent.width, rect.extent.height);
+    }
+
+    if ((vr = create_device(vk_physical_device, 0, NULL, NULL, &vk_device)) < 0)
+    {
+        skip("Failed to create device, vr %d.\n", vr);
+        vkDestroySurfaceKHR(vk_instance, surface, NULL);
+        return;
+    }
+
+    if (0)
+    {
+        /* Causes access violation on Windows. */
+        vr = vkGetDeviceGroupSurfacePresentModesKHR(vk_device, surface, &present_mode_flags);
+        ok(vr == VK_SUCCESS, "Got unexpected vr %d.\n", vr);
+    }
+
+    vkDestroyDevice(vk_device, NULL);
+    vkDestroySurfaceKHR(vk_instance, surface, NULL);
+}
+
+static void for_each_device_instance(uint32_t extension_count, const char * const *enabled_extensions,
+        void (*test_func_instance)(VkInstance, VkPhysicalDevice), void (*test_func)(VkPhysicalDevice))
 {
     VkPhysicalDevice *vk_physical_devices;
     VkInstance vk_instance;
@@ -445,7 +540,7 @@ static void for_each_device(void (*test_func)(VkPhysicalDevice))
     uint32_t count;
     VkResult vr;

-    if ((vr = create_instance_skip(0, NULL, &vk_instance)) < 0)
+    if ((vr = create_instance_skip(extension_count, enabled_extensions, &vk_instance)) < 0)
         return;
     ok(vr == VK_SUCCESS, "Got unexpected VkResult %d.\n", vr);

@@ -463,13 +558,23 @@ static void for_each_device(void (*test_func)(VkPhysicalDevice))
     ok(vr == VK_SUCCESS, "Got unexpected VkResult %d.\n", vr);

for (i = 0; i < count; ++i)
-        test_func(vk_physical_devices[i]);
+    {
+        if (test_func_instance)
+            test_func_instance(vk_instance, vk_physical_devices[i]);
+        else
+            test_func(vk_physical_devices[i]);
+    }

heap_free(vk_physical_devices);

vkDestroyInstance(vk_instance, NULL);
 }

+static void for_each_device(void (*test_func)(VkPhysicalDevice))
+{
+    for_each_device_instance(0, NULL, NULL, test_func);
+}
+
 START_TEST(vulkan)
 {
     test_instance_version();
@@ -481,4 +586,5 @@ START_TEST(vulkan)
     test_unsupported_instance_extensions();
     for_each_device(test_unsupported_device_extensions);
     for_each_device(test_private_data);
+    for_each_device_instance(ARRAY_SIZE(test_null_hwnd_extensions), test_null_hwnd_extensions, test_null_hwnd, NULL);
 }
diff --git a/dlls/winex11.drv/vulkan.c b/dlls/winex11.drv/vulkan.c
index 139faf6b407..4ce06f3e9ca 100644
--- a/dlls/winex11.drv/vulkan.c
+++ b/dlls/winex11.drv/vulkan.c
@@ -62,6 +62,7 @@ struct wine_vk_surface
     LONG ref;
     Window window;
     VkSurfaceKHR surface; /* native surface */
+    HWND hwnd;
 };

typedef struct VkXlibSurfaceCreateInfoKHR
@@ -255,14 +256,18 @@ static VkResult X11DRV_vkCreateSwapchainKHR(VkDevice device,
         const VkSwapchainCreateInfoKHR *create_info,
         const VkAllocationCallbacks *allocator, VkSwapchainKHR *swapchain)
 {
+    struct wine_vk_surface *x11_surface = surface_from_handle(create_info->surface);
     VkSwapchainCreateInfoKHR create_info_host;
     TRACE("%p %p %p %p\n", device, create_info, allocator, swapchain);

if (allocator)
         FIXME("Support for allocation callbacks not implemented yet\n");

+    if (!x11_surface->hwnd)
+        return VK_ERROR_SURFACE_LOST_KHR;
+
     create_info_host = *create_info;
-    create_info_host.surface = surface_from_handle(create_info->surface)->surface;
+    create_info_host.surface = x11_surface->surface;

return pvkCreateSwapchainKHR(device, &create_info_host, NULL /* allocator */, swapchain);
 }
@@ -281,7 +286,7 @@ static VkResult X11DRV_vkCreateWin32SurfaceKHR(VkInstance instance,
         FIXME("Support for allocation callbacks not implemented yet\n");

/* TODO: support child window rendering. */
-    if (GetAncestor(create_info->hwnd, GA_PARENT) != GetDesktopWindow())
+    if (create_info->hwnd && GetAncestor(create_info->hwnd, GA_PARENT) != GetDesktopWindow())
     {
         FIXME("Application requires child window rendering, which is not implemented yet!\n");
         return VK_ERROR_INCOMPATIBLE_DRIVER;
@@ -292,8 +297,10 @@ static VkResult X11DRV_vkCreateWin32SurfaceKHR(VkInstance instance,
         return VK_ERROR_OUT_OF_HOST_MEMORY;

x11_surface->ref = 1;
+    x11_surface->hwnd = create_info->hwnd;
+    x11_surface->window = x11_surface->hwnd ? create_client_window(create_info->hwnd, &default_visual)
+            : create_dummy_client_window();

-    x11_surface->window = create_client_window(create_info->hwnd, &default_visual);
     if (!x11_surface->window)
     {
         ERR("Failed to allocate client window for hwnd=%p\n", create_info->hwnd);
@@ -316,13 +323,16 @@ static VkResult X11DRV_vkCreateWin32SurfaceKHR(VkInstance instance,
         goto err;
     }

-    EnterCriticalSection(&context_section);
-    if (!XFindContext(gdi_display, (XID)create_info->hwnd, vulkan_hwnd_context, (char **)&prev))
+    if (x11_surface->hwnd)
     {
-        wine_vk_surface_release(prev);
+        EnterCriticalSection(&context_section);
+        if (!XFindContext(gdi_display, (XID)create_info->hwnd, vulkan_hwnd_context, (char **)&prev))
+        {
+            wine_vk_surface_release(prev);
+        }
+        XSaveContext(gdi_display, (XID)create_info->hwnd, vulkan_hwnd_context, (char *)wine_vk_surface_grab(x11_surface));
+        LeaveCriticalSection(&context_section);
     }
-    XSaveContext(gdi_display, (XID)create_info->hwnd, vulkan_hwnd_context, (char *)wine_vk_surface_grab(x11_surface));
-    LeaveCriticalSection(&context_section);

*surface = (uintptr_t)x11_surface;

@@ -456,6 +466,15 @@ static VkResult X11DRV_vkGetPhysicalDevicePresentRectanglesKHR(VkPhysicalDevice

TRACE("%p, 0x%s, %p, %p\n", phys_dev, wine_dbgstr_longlong(surface), count, rects);

+    if (!x11_surface->hwnd)
+    {
+        if (rects)
+            return VK_ERROR_SURFACE_LOST_KHR;
+
+        *count = 1;
+        return VK_SUCCESS;
+    }
+
     return pvkGetPhysicalDevicePresentRectanglesKHR(phys_dev, x11_surface->surface, count, rects);
 }

@@ -485,6 +504,9 @@ static VkResult X11DRV_vkGetPhysicalDeviceSurfaceCapabilitiesKHR(VkPhysicalDevic

TRACE("%p, 0x%s, %p\n", phys_dev, wine_dbgstr_longlong(surface), capabilities);

+    if (!x11_surface->hwnd)
+        return VK_ERROR_SURFACE_LOST_KHR;
+
     return pvkGetPhysicalDeviceSurfaceCapabilitiesKHR(phys_dev, x11_surface->surface, capabilities);
 }

diff --git a/dlls/winex11.drv/window.c b/dlls/winex11.drv/window.c
index baaa30d74e3..1f0d636a142 100644
--- a/dlls/winex11.drv/window.c
+++ b/dlls/winex11.drv/window.c
@@ -1466,6 +1466,26 @@ static Window get_dummy_parent(void)
 }

+/**********************************************************************
+ *		create_dummy_window
+ */
+Window create_dummy_client_window(void)
+{
+    XSetWindowAttributes attr;
+
+    attr.colormap = default_colormap;
+    attr.bit_gravity = NorthWestGravity;
+    attr.win_gravity = NorthWestGravity;
+    attr.backing_store = NotUseful;
+    attr.border_pixel = 0;
+
+    return XCreateWindow( gdi_display,
+                                       get_dummy_parent(),
+                                       0, 0, 1, 1, 0, default_visual.depth, InputOutput,
+                                       default_visual.visual, CWBitGravity | CWWinGravity |
+                                       CWBackingStore | CWColormap | CWBorderPixel, &attr );
+}
+
 /**********************************************************************
  *		create_client_window
  */
diff --git a/dlls/winex11.drv/x11drv.h b/dlls/winex11.drv/x11drv.h
index 45855976607..2362dfd9563 100644
--- a/dlls/winex11.drv/x11drv.h
+++ b/dlls/winex11.drv/x11drv.h
@@ -601,6 +601,7 @@ extern void update_user_time( Time time ) DECLSPEC_HIDDEN;
 extern void read_net_wm_states( Display *display, struct x11drv_win_data *data ) DECLSPEC_HIDDEN;
 extern void update_net_wm_states( struct x11drv_win_data *data ) DECLSPEC_HIDDEN;
 extern void make_window_embedded( struct x11drv_win_data *data ) DECLSPEC_HIDDEN;
+extern Window create_dummy_client_window(void) DECLSPEC_HIDDEN;
 extern Window create_client_window( HWND hwnd, const XVisualInfo *visual ) DECLSPEC_HIDDEN;
 extern void set_window_visual( struct x11drv_win_data *data, const XVisualInfo *vis, BOOL use_alpha ) DECLSPEC_HIDDEN;
 extern void change_systray_owner( Display *display, Window systray_window ) DECLSPEC_HIDDEN;

[Rémi Bernon]

A few style nitpicks, otherwise it looks good to me.

On 4/30/21 4:42 PM, Paul Gofman wrote:
> @@ -292,8 +297,10 @@ static VkResult X11DRV_vkCreateWin32SurfaceKHR(VkInstance instance,
>           return VK_ERROR_OUT_OF_HOST_MEMORY;
>   x11_surface->ref = 1;
> - x11_surface->hwnd = create_info->hwnd;
> - x11_surface->window = x11_surface->hwnd ? create_client_window(create_info->hwnd, &default_visual)
> -
>        : create_dummy_client_window();

Indentation feels weird here, I think it would be better if ':' was 
aligned with the '?' above.
> - x11_surface->window = create_client_window(create_info->hwnd, &default_visual);
>  if (!x11_surface->window)
>  {
>      ERR("Failed to allocate client window for hwnd=%p\n", create_info->hwnd);
>
> @@ -316,13 +323,16 @@ static VkResult X11DRV_vkCreateWin32SurfaceKHR(VkInstance instance,
>           goto err;
>       }
> - EnterCriticalSection(&context_section);
> - if (!XFindContext(gdi_display, (XID)create_info->hwnd, vulkan_hwnd_context, (char **)&prev))
> - if (x11_surface->hwnd)
>  {
> -
>    wine_vk_surface_release(prev);
> -
>    EnterCriticalSection(&context_section);
> -
>    if (!XFindContext(gdi_display, (XID)create_info->hwnd, vulkan_hwnd_context, (char **)&prev))
> -
>    {
> -
>        wine_vk_surface_release(prev);
> -
>    }
> -
>    XSaveContext(gdi_display, (XID)create_info->hwnd, vulkan_hwnd_context, (char *)wine_vk_surface_grab(x11_surface));
> -
>    LeaveCriticalSection(&context_section);
> }
> -
>
> XSaveContext(gdi_display, (XID)create_info->hwnd, vulkan_hwnd_context, (char *)wine_vk_surface_grab(x11_surface));
> -
>
> LeaveCriticalSection(&context_section);
>
> *surface = (uintptr_t)x11_surface;
>
> @@ -456,6 +466,15 @@ static VkResult X11DRV_vkGetPhysicalDevicePresentRectanglesKHR(VkPhysicalDevice
>   TRACE("%p, 0x%s, %p, %p\n", phys_dev, wine_dbgstr_longlong(surface), count, rects);
>
> - if (!x11_surface->hwnd)
> - {
> -
>    if (rects)
> -
>        return VK_ERROR_SURFACE_LOST_KHR;
> -
> -
>    *count = 1;
> -
>    return VK_SUCCESS;
> - }
> -
> return pvkGetPhysicalDevicePresentRectanglesKHR(phys_dev, x11_surface->surface, count, rects);
> }
>
> @@ -485,6 +504,9 @@ static VkResult X11DRV_vkGetPhysicalDeviceSurfaceCapabilitiesKHR(VkPhysicalDevic
>   TRACE("%p, 0x%s, %p\n", phys_dev, wine_dbgstr_longlong(surface), capabilities);
>
> - if (!x11_surface->hwnd)
> -
>    return VK_ERROR_SURFACE_LOST_KHR;
> -
> return pvkGetPhysicalDeviceSurfaceCapabilitiesKHR(phys_dev, x11_surface->surface, capabilities);
> }
>
> diff --git a/dlls/winex11.drv/window.c b/dlls/winex11.drv/window.c
> index baaa30d74e3..1f0d636a142 100644
> --- a/dlls/winex11.drv/window.c
> +++ b/dlls/winex11.drv/window.c
> @@ -1466,6 +1466,26 @@ static Window get_dummy_parent(void)
>   }
>
> +/**********************************************************************
> -
> -
> create_dummy_window

Should probably be create_dummy_client_window
> - */
>
> +Window create_dummy_client_window(void)
> +{
> - XSetWindowAttributes attr;
> -
> - attr.colormap = default_colormap;
> - attr.bit_gravity = NorthWestGravity;
> - attr.win_gravity = NorthWestGravity;
> - attr.backing_store = NotUseful;
> - attr.border_pixel = 0;
> -
> - return XCreateWindow( gdi_display,
> -
>                                   get_dummy_parent(),
> -
>                                   0, 0, 1, 1, 0, default_visual.depth, InputOutput,
> -
>                                   default_visual.visual, CWBitGravity | CWWinGravity |
> -
>                                   CWBackingStore | CWColormap | CWBorderPixel, &attr );

Here as well, indentation is pretty weird.

[Paul Gofman]

On 5/1/21 10:21, Rémi Bernon wrote:
> A few style nitpicks, otherwise it looks good to me.
>
> On 4/30/21 4:42 PM, Paul Gofman wrote:
> > @@ -292,8 +297,10 @@ static VkResult
> > X11DRV_vkCreateWin32SurfaceKHR(VkInstance instance,
> >           return VK_ERROR_OUT_OF_HOST_MEMORY;
> >         x11_surface->ref = 1;
> > +    x11_surface->hwnd = create_info->hwnd;
> > +    x11_surface->window = x11_surface->hwnd ?
> > create_client_window(create_info->hwnd, &default_visual)
> > +            : create_dummy_client_window();
>
> Indentation feels weird here, I think it would be better if ':' was
> aligned with the '?' above.

Yeah, ok, I will change this way.
> > diff --git a/dlls/winex11.drv/window.c b/dlls/winex11.drv/window.c
> > index baaa30d74e3..1f0d636a142 100644
> > --- a/dlls/winex11.drv/window.c
> > +++ b/dlls/winex11.drv/window.c
> > @@ -1466,6 +1466,26 @@ static Window get_dummy_parent(void)
> >   }
> >    
> > +/**********************************************************************
> > - *        create_dummy_window
>
> Should probably be create_dummy_client_window

Thanks, that's a copy-paste leftover.
> > - */
> >
> > +Window create_dummy_client_window(void)
> > +{
> > +    XSetWindowAttributes attr;
> > -
> >
> > +    attr.colormap = default_colormap;
> > +    attr.bit_gravity = NorthWestGravity;
> > +    attr.win_gravity = NorthWestGravity;
> > +    attr.backing_store = NotUseful;
> > +    attr.border_pixel = 0;
> > -
> >
> > +    return XCreateWindow( gdi_display,
> > +                                       get_dummy_parent(),
> > +                                       0, 0, 1, 1, 0,
> > default_visual.depth, InputOutput,
> > +                                       default_visual.visual,
> > CWBitGravity | CWWinGravity |
> > +                                       CWBackingStore | CWColormap |
> > CWBorderPixel, &attr );
>
> Here as well, indentation is pretty weird.

The indentation is actually a copy paste from XCrreateWindow in
create_client_window(). I agree its weird though. I realized I am
missing the general rule for multipline operator indentation if it is
not wined3d style used. Do you know if there is such a rule?

[Rémi Bernon]

On 5/3/21 11:45 AM, Paul Gofman wrote:
> On 5/1/21 10:21, Rémi Bernon wrote:
> > A few style nitpicks, otherwise it looks good to me.
> >
> > On 4/30/21 4:42 PM, Paul Gofman wrote:
> > > @@ -292,8 +297,10 @@ static VkResult
> > > X11DRV_vkCreateWin32SurfaceKHR(VkInstance instance,
> > >            return VK_ERROR_OUT_OF_HOST_MEMORY;
> > >          x11_surface->ref = 1;
> > > +    x11_surface->hwnd = create_info->hwnd;
> > > +    x11_surface->window = x11_surface->hwnd ?
> > > create_client_window(create_info->hwnd, &default_visual)
> > > +            : create_dummy_client_window();
> >
> > Indentation feels weird here, I think it would be better if ':' was
> > aligned with the '?' above.
>
> Yeah, ok, I will change this way.
> > > diff --git a/dlls/winex11.drv/window.c b/dlls/winex11.drv/window.c
> > > index baaa30d74e3..1f0d636a142 100644
> > > --- a/dlls/winex11.drv/window.c
> > > +++ b/dlls/winex11.drv/window.c
> > > @@ -1466,6 +1466,26 @@ static Window get_dummy_parent(void)
> > >    }
> > >
> > > +/**********************************************************************
> > > - *        create_dummy_window
> >
> > Should probably be create_dummy_client_window
>
> Thanks, that's a copy-paste leftover.
> > > - */
> > >
> > > +Window create_dummy_client_window(void)
> > > +{
> > > +    XSetWindowAttributes attr;
> > > -
> > >
> > > +    attr.colormap = default_colormap;
> > > +    attr.bit_gravity = NorthWestGravity;
> > > +    attr.win_gravity = NorthWestGravity;
> > > +    attr.backing_store = NotUseful;
> > > +    attr.border_pixel = 0;
> > > -
> > >
> > > +    return XCreateWindow( gdi_display,
> > > +                                       get_dummy_parent(),
> > > +                                       0, 0, 1, 1, 0,
> > > default_visual.depth, InputOutput,
> > > +                                       default_visual.visual,
> > > CWBitGravity | CWWinGravity |
> > > +                                       CWBackingStore | CWColormap |
> > > CWBorderPixel, &attr );
> >
> > Here as well, indentation is pretty weird.
>
> The indentation is actually a copy paste from XCrreateWindow in
> create_client_window(). I agree its weird though. I realized I am
> missing the general rule for multipline operator indentation if it is
> not wined3d style used. Do you know if there is such a rule?

No idea, I think winex11 is a bit messy but except for some parts I 
think it tends to follow Julliard style (as ntdll for instance), with 
line continuations aligned with the corresponding element on the 
previous line.

This probably could be something like IMHO:
> return XCreateWindow( gdi_display, get_dummy_parent(), 0, 0, 1, 1, 0,
>                       default_visual.depth, InputOutput, default_visual.visual,
>                       CWBitGravity | CWWinGravity | CWBackingStore | CWColormap | CWBorderPixel, &attr );

Cheers,

[Paul Gofman]

On 5/3/21 12:52, Rémi Bernon wrote:
> No idea, I think winex11 is a bit messy but except for some parts I
> think it tends to follow Julliard style (as ntdll for instance), with
> line continuations aligned with the corresponding element on the
> previous line.

If there is actually no consistent rule for indentation I'd suggest that
maybe its ok to just follow wined3d style (two indentation positions for
continued lines), given it also used often now throughout all the Wine
code. Which has an advantage that you don't have to think of that and
always have it consistent. But for now I will just change the way you
suggest.

[Rémi Bernon]

On 5/3/21 11:56 AM, Paul Gofman wrote:
> On 5/3/21 12:52, Rémi Bernon wrote:
> > No idea, I think winex11 is a bit messy but except for some parts I
> > think it tends to follow Julliard style (as ntdll for instance), with
> > line continuations aligned with the corresponding element on the
> > previous line.
>
> If there is actually no consistent rule for indentation I'd suggest that
> maybe its ok to just follow wined3d style (two indentation positions for
> continued lines), given it also used often now throughout all the Wine
> code. Which has an advantage that you don't have to think of that and
> always have it consistent. But for now I will just change the way you
> suggest.

Is it really? I'm sure it depends on the maintainer. Also winex11 code 
is not completely consistent but still, most of its code is not 
following this rule, and new code that has been recently added to it 
don't either.

Although I agree the said rule is convenient to think less about the 
indentation, in this specific case (the ternary operator) I really find 
it less readable.

IMHO if we really want to stop thinking about indentation we should use 
automatic indentation tools, not rules that makes things less readable.

Maybe the best fix is then not to use a ternary operator.

[Paul Gofman]

On 5/3/21 13:03, Rémi Bernon wrote:
> Is it really? I'm sure it depends on the maintainer. Also winex11 code
> is not completely consistent but still, most of its code is not
> following this rule, and new code that has been recently added to it
> don't either.
>
> Although I agree the said rule is convenient to think less about the
> indentation, in this specific case (the ternary operator) I really
> find it less readable.
>
> IMHO if we really want to stop thinking about indentation we should
> use automatic indentation tools, not rules that makes things less
> readable.

I am not sure how automatic indentation tool can manage if we can't
formulate the rule for indentation?

[Rémi Bernon]

On 5/3/21 12:08 PM, Paul Gofman wrote:
> On 5/3/21 13:03, Rémi Bernon wrote:
> > Is it really? I'm sure it depends on the maintainer. Also winex11 code
> > is not completely consistent but still, most of its code is not
> > following this rule, and new code that has been recently added to it
> > don't either.
> >
> > Although I agree the said rule is convenient to think less about the
> > indentation, in this specific case (the ternary operator) I really
> > find it less readable.
> >
> > IMHO if we really want to stop thinking about indentation we should
> > use automatic indentation tools, not rules that makes things less
> > readable.
>
> I am not sure how automatic indentation tool can manage if we can't
> formulate the rule for indentation?

I believe aligning operators (or line continuations) with the previous 
line is a pretty common style, generally considered as readable [*]. And 
although it's not written anywhere I think it's how most winex11.drv 
code is written.

[*] Of course it's still a matter of personal taste but there's enough 
examples of code using such style out there to assume it's the case.

[Zebediah Figura (she/her)]

On 5/1/21 2:21 AM, Rémi Bernon wrote:
> A few style nitpicks, otherwise it looks good to me.
>
> On 4/30/21 4:42 PM, Paul Gofman wrote:
> > @@ -292,8 +297,10 @@ static VkResult X11DRV_vkCreateWin32SurfaceKHR(VkInstance instance,
> >            return VK_ERROR_OUT_OF_HOST_MEMORY;
> >    x11_surface->ref = 1;
> > - x11_surface->hwnd = create_info->hwnd;
> > - x11_surface->window = x11_surface->hwnd ? create_client_window(create_info->hwnd, &default_visual)
> > -
> >        : create_dummy_client_window();
>
> Indentation feels weird here, I think it would be better if ':' was
> aligned with the '?' above.

Personally, if I find myself having to wrap a ternary operator, I take 
it as a sign that I should use an if/else statement instead. Just my two 
cents...