Raise the window more reliably

[guijan]

Instead of polling for the event (with drift from calling nanosleep() in a loop...), this code:

scrot/src/scrot.c

Lines 373 to 379 in dc65e96

	struct timespec delay = {0, 10000000L}; // 10ms
	for (short i = 0; i < 30; ++i) {
	if (XCheckIfEvent(disp, &(XEvent){0}, &scrotXEventVisibility, (XPointer)&target))
	break;
	nanosleep(&delay, NULL);
	}

Should use select() or poll() to wait for the fd used to communicate with the X server to have some data, then use XCheckIfEvent() to check if the data is what we're watching for.

Edit: Is 300ms enough of a delay? 10ms is often the precision OSes offer to the userland when it comes to timers, there's probably significant drift in polling every 10ms, fixing this might expose an issue with the delay being too short, but I find it unlikely for local X servers.
Edit 2: fixed incorrect statement about poll()

[N-R-K]

Edit: poll() doesn't have subsecond precision.

poll's timeout is in miliseconds.

And there are much bigger issues with that code. I'm at the end of my awake hours, so I'll take a closer look tomorrow with fresh eyes before elaborating.

[guijan]

Other place with the same pattern:

scrot/src/selection_edge.c

Lines 152 to 158 in 3b281db

	struct timespec delay = {0, 80000000L}; // 80ms
	for (short i = 0; i < 30; ++i) {
	if (XCheckIfEvent(disp, &ev, &xeventUnmap, (XPointer) & (pe->wndDraw)))
	break;
	nanosleep(&delay, NULL);
	}

This one seems similar on the surface:

scrot/src/scrot_selection.c

Lines 248 to 256 in 5ef8a82

	ret = XGrabKeyboard(disp, root, False, GrabModeAsync, GrabModeAsync, CurrentTime);
	if (ret == AlreadyGrabbed) {
	int attempts = 20;
	struct timespec delay = { 0, 50 * 1000L * 1000L };
	do {
	nanosleep(&delay, NULL);
	ret = XGrabKeyboard(disp, root, False, GrabModeAsync, GrabModeAsync, CurrentTime);
	} while (--attempts > 0 && ret == AlreadyGrabbed);
	}

But I don't know if there's a way to sleep until the keyboard can be grabbed.

[N-R-K]

And there are much bigger issues with that code.

1. scrotXEventVisibility tries to check if the window becomes visible. However, that event will never come. Because we never subscribed to visibility events.

            XSelectInput(disp, target, FocusChangeMask);

This subscribes us to focus-change events, not visibility events. For visibility events, VisibilityChangeMask is needed. The window member on both visibility and focus-change event happens to line up, which is why that code happened to "work" (for some definition of work).

2. Just because we received a visibility event doesn't mean the window is visible. Visibility event can be generated due to a window becoming obscured or partially obscured as well.

3. XCheckIfEvent will only pull out the event for which scrotXEventVisibility returned true and leave the prior events in the queue. I don't really see why this would be needed or desired.

4. There might be a race since we call XSelectInput after raising the window. So we might end up missing the event even though it was generated. (This one I'm not 100% sure about since xlib buffers the output).

There's so much issues with this code that I'd rather not try and fix it, instead I'd much rather figure out what exact problem that code was trying to solve and then solve that from scratch.

[N-R-K]

I'd much rather figure out what exact problem that code was trying to solve

Related: #67

My wm doesn't seem to respect the raise request. So it'll be difficult for me to test.

[N-R-K]

One more bug, if the selected window was already raised, then there won't be any visibility event (as nothing changed) and so scrot will just sleep for that entire duration.

[N-R-K]

What we ideally want here is something like the following:

1. Send a raise request
2. Wait for the raise request to be either completed/accepted or rejected.

But I'm not sure if this can be done.

The only thing close to it in ICCCM/EWMH I can find is this. But it's not exactly what we're looking for, it's about the currently focused window - I'm not sure if there's anything that says that a focused window cannot be obscured.

And then there's this:

the Window Manager may decide to refuse the request (either completely ignore it, or e.g. use _NET_WM_STATE_DEMANDS_ATTENTION)

And in practice, there's plently of WMs that take this route. For example, dwm only sets the window to be "urgent" when it receives a _NET_ACTIVE_WINDOW request - it doesn't give it focus.

[N-R-K]

And in practice, there's plently of WMs that take this route. For example, dwm only sets the window to be "urgent" when it receives a _NET_ACTIVE_WINDOW request - it doesn't give it focus.

If a WM supports _NET_ACTIVE_WINDOW then we can avoid sleeping if the window already was raised. But I'm not sure if "active" window really means it's "raised" or not, the spec only says it's about which window has focus (and typically the window that has focus is raised, but I don't know if it's required to be).

[N-R-K]

But I don't know if there's a way to sleep until the keyboard can be grabbed.

This is a different issue altogether.

But yes you can wait for a focus event to come (since grabbing/ungrabbing the keyboard generates focus in/out events). It's what I do in sx4 - but there's no timeout involved.

In order to do a timeout as well, I think you'd need to either setup a sigalarm handler that _exit()s or mess with poll or mess with XSync alarm.

Unless there's some pitfall involved, setting up an alarm with a fatal handler seems to be the simplest solution. I'd rather not go XSync route, it's a lot of code, is inefficient and misses deadlines by larger amount.