Fix X11_SetWindowPosition() edgecase

[rokups]

Currently it is not possible to move window with SDL_WINDOW_BORDERLESS flag outside of screen bounds using SDL_SetWindowPosition() (#3813). Suppose we move such window at the bottom of the screen, and it's bottom border is stops at bottom border of the screen. If we proceed to move such window further out of the screen bounds (still using SDL_SetWindowPosition()), window position keeps getting updated in SDL_SetWindowPosition(), but position we requested is outside of screen bounds and therefore is not valid. WM forces our window to stay in the same position. When X11_SetWindowPosition() executes following happens:

1. X11_XTranslateCoordinates(&orig_x, &orig_y); - return position clamped within screen bounds. This is not what we requested in X11_SetWindowPosition() and not what window->x/window->y have currently set.
2. X11_XMoveWindow(); - move to requested coordinates. However nothing happens because WM prevents moving outside of screen bounds. Window remains in the same location.
3. X11_XTranslateCoordinates(&x, &y); - checking if window moved. This call returns same coordinates as in step 1.
4. if (SDL_TICKS_PASSED(SDL_GetTicks(), timeout)) - eventually check passes and we break out of the loop.

So SDL_SetWindowPosition() updated window->x/window->y, then we failed to move window and kept our requested window position set in SDL_Window, making subsequent SDL_GetWindowPosition() calls return incorrect results.

Solution: unconditionally update window struct with real window position, because why not.

---

To reproduce this issue do the following:

git clone https://github.com/ocornut/imgui.git
cd imgui
git checkout docking
cd examples/example_sdl_opengl3
make
./example_sdl_opengl3

• Drag any window outside of main vieport
• Keep dragging this window to the edge of the screen, trying to move it outside
  Result: It becomes impossible to click widgets because SDL_GetWindowPos() returns position that does not match actual window position.

bug.mp4

[flibitijibibo]

The else if right below this will break if it's set here, so while we should probably update this more aggressively it'll have to be at a different spot so the stuff below will be reachable.

[rokups]

Thank you for taking a look! I force-pushed a fix. Should be good now.

[flibitijibibo]

This makes sense to me - previous commits for this loop:

d1df343
367a8b9
e731522

CC @icculus, this may interact with #3269.

[rokups]

There is something else. I am not sure if we can do anything about it though. This 10ms waiting is really visible when window is being dragged sideways while brushing against bottom edge of the screen. Under what conditions issue worked around by loop triggers? For comparison GLFW does not do any of this, it just tries to set window position and calls it a day. WM is free to deny movement and that is also fine, since _glfwPlatformGetWindowPos() merely calls XTranslateCoordinates() obtaining actual position from X11. I get an impression that caching window position in SDL causes more trouble than it is worth.

sdl-bug.mp4

[flibitijibibo]

It's possible that X specifically will have to get window position via events exclusively (since a lot of what makes this not work is all the roundtrips needed for X calls to stick), but keep in mind that Wayland's probably going to become the default within the next year or so, so either this will all get blown apart by Wayland anyway (but with no window positions at all, get or set!) or it'll be running through Xwayland, which is probably going to behave even worse than this.

[rokups]

Hah we keep saying mainstream wayland next year for last five years, so i am not holding my breath..

I have been tinkering with the code more. 100ms hicups every time WM prevents window from moving is a bit much, especially for applications that use SDL_SetWindowPosition() for window dragging.

void
X11_SetWindowPosition(_THIS, SDL_Window * window)
{
    SDL_WindowData *data = (SDL_WindowData *) window->driverdata;
    Display *display = data->videodata->display;
    unsigned int childCount;
    Window childReturn, root, parent;
    Window* children;
    XWindowAttributes attrs;
    int x, y, orig_x, orig_y;
    Uint32 timeout;

    X11_XSync(display, False);
    X11_XQueryTree(display, data->xwindow, &root, &parent, &children, &childCount);
    X11_XGetWindowAttributes(display, data->xwindow, &attrs);
    X11_XTranslateCoordinates(display, parent, DefaultRootWindow(display),
                              attrs.x, attrs.y, &orig_x, &orig_y, &childReturn);

    /*Attempt to move the window*/
    X11_XMoveWindow(display, data->xwindow, window->x - data->border_left, window->y - data->border_top);

    /* Wait a brief time to see if the window manager decided to let this move happen.
       If the window changes at all, even to an unexpected value, we break out. */
    //timeout = SDL_GetTicks() + 100;
    //while (SDL_TRUE) {
        X11_XSync(display, False);
        X11_XGetWindowAttributes(display, data->xwindow, &attrs);
        X11_XTranslateCoordinates(display, parent, DefaultRootWindow(display),
                                  attrs.x, attrs.y, &x, &y, &childReturn);
    
    //    if ((x != orig_x) || (y != orig_y)) {
    //        break;  /* window moved, time to go. */
    //    } else if ((x == window->x) && (y == window->y)) {
    //        break;  /* we're at the place we wanted to be anyhow, drop out. */
    //    }
    //
    //    if (SDL_TICKS_PASSED(SDL_GetTicks(), timeout)) {
    //        break;
    //    }
    //
    //    SDL_Delay(10);
    //}
    window->x = x;
    window->y = y;
}

This seems to work much better in my case. I am sure this code was added for a good reason and i would love to find out exact reason so i could replicate issue this loop is fixing. I read through #3337 and #3269 and pulled out old issue reproduction case and played with it. Since i we are dealing with position issues here as opposed to size issues that #3337 describes - i repurposed it a bit.

Testcase:

#include <SDL.h>
#include <stdio.h>

using namespace std;

int main (int argc, char** argv)
{
    if (SDL_Init(SDL_INIT_EVERYTHING) < 0) return -1;

    SDL_Window* window = SDL_CreateWindow("",0, 0,640, 480,0);
    if (!window) return -1;

    SDL_Renderer* renderer = SDL_CreateRenderer(window, -1, 0);
    if (!renderer) return -1;

    //SDL_Event e; while (SDL_PollEvent(&e));//This line fixes SDL_GetWindowSize,
    //but not the drawing problem
    int w, h;
    SDL_SetWindowPosition(window, 600, 2300);
    printf("SDL_SetWindowPosition(600, 2300);   // Request window position way out of bounds\n");
    SDL_GetWindowPosition(window, &w,&h);
    printf("SDL_GetWindowPosition(%d, %d);      // Initial window position response\n", w, h);

    SDL_PollEvent(nullptr); //This line causes next SDL_GetWindowSize to be wrong

    //Uncomment the next 3 lines for a (weird, possibly unreliable) fix
    // for the drawing problem
    //Replacing the loop with a single SDL_Delay(60); seems less reliable
    //SDL_RenderPresent(renderer);
    //for (int i = 0; i < 6; ++i) //Must be 6 or more to be reliable
    //    SDL_Delay(10);          //10 works; 1 doesn't

    SDL_GetWindowPosition(window, &w, &h);
    printf("SDL_GetWindowPosition(%d, %d);  // Final window position response\n", w, h);

    //Now let's draw cross hairs centered on middle
    SDL_SetRenderDrawColor(renderer, 255, 255, 255, 255);

    SDL_RenderDrawLine (renderer, 0,  h/2,   w,  h/2);
    SDL_RenderDrawLine (renderer, w/2,  0,  w/2, h);

    SDL_RenderPresent (renderer);

    SDL_Event sdlEvent; //Wait for key hit, so we can see rect
    do
        SDL_WaitEvent (&sdlEvent);
    while (sdlEvent.type != SDL_KEYDOWN);


    return 0;
}

Result:

SDL_SetWindowPosition(600, 2300);   // Request window position way out of bounds
SDL_GetWindowPosition(66, 58);      // Initial window position response
window 0x563a13d0b700: ConfigureNotify! (position: 66,58, size: 640x480)
window 0x563a13d0b700: ConfigureNotify! (position: 600,960, size: 640x480)
window 0x563a13d0b700: ConfigureNotify! (position: 600,960, size: 640x480)
SDL_GetWindowPosition(600, 960);  // Final window position response

So yes, you are right. We have a race condition here. Move request is not processed immediately and we get (66, 58) position initially. I do not believe that is an issue however. Yes, SDL_GetWindowPosition() does not return requested position immediately, but we expect such behavior by default - WM may prevent us positioning a window at position we requested. However no matter what WM does - in the end we always get ConfigureNotify with a new position (or size for that matter). Therefore i think solution initially proposed in #3269 is more correct as we eventually get actual window position wherever it ended up being, without introducing 100ms stalls in main loop.

[flibitijibibo]

You can update this patch to do this instead, if you want. Both changes seem okay to me, the other way makes sense too provided the sync does what it's supposed to do.

For Wayland-by-default, see #4306. Once libdecor is frozen (expected this year) and NV 470.xx is out (expected this month) this flip will happen really soon after for both SDL and desktop environments, possibly as soon as 2.0.18 should testers not find anything incorrect with our implementation. I strongly suggest testing ImGui_SDL against both drivers.

[rokups]

You can update this patch to do this instead, if you want. Both changes seem okay to me, the other way makes sense too provided the sync does what it's supposed to do.

Great! I will do that! I wasnt sure it will be accepted. It looks ok on the surface, but everything always is a deep pit.. Good to have a second opinion that agrees 👍🏻

I strongly suggest testing ImGui_SDL against both drivers.

Yeah.. This is bit complicated. Dear ImGui currently expects to know global window position to work and wayland threw us under the bus in this regard. I am pushing for a path to support wayland nevertheless, but its a tough way ahead. Hopefully we will get there 👍🏻

Edit:
I also took a look at removing same loop from X11_SetWindowSize(). Removing this loop and making behavior identical to X11_SetWindowPosition() actually made things worse in case of Dear ImGui. Lack of immediate response created a discrepancy between window size and graphics canvas imgui renders into, causing double-vision-like effect. Even though i think it is more technically correct, I could not replicate any visible resize stalls like in case of window dragging. Since there are no visible stalls - there is no point in changing this function, especially if it creates further issues.

[icculus]

I'm going to bump this from 2.26.0, since there's a lot of chatter in this thread and the current patch needs conflicts resolved, so let's not risk this right at the end of the milestone.

[icculus]

Having looked at this, I'm going to decline to change this; SDL takes a lot of pains to deal with X11's async nature but also present a consistent API, and removing that code is going to cause problems that we've tackled several times now.

We can revisit this in SDL3 if there's a specific problem with borderless windows, but this patch isn't the way we want to handle it.