STR #3124: Cross-window take_focus not fully working

STR #3124

Application:	FLTK Library
Status:	1 - Closed w/Resolution
Priority:	3 - Moderate, e.g. unable to compile the software
Scope:	2 - Specific to an operating system
Subsystem:	X11
Summary:	Cross-window take_focus not fully working
Version:	1.3-current
Created By:	cand
Assigned To:	AlbrechtS
Fix Version:	1.3.3 (SVN: v10339)
Update Notification:	Receive EMails Don't Receive EMails

Trouble Report Files:

#1	cand 12:44 Sep 02, 2014

windowfocus.cxx
1k

#2	ianmacarthur 07:54 Sep 04, 2014

windowfocus2.cxx
1k

#3	ianmacarthur 08:59 Sep 04, 2014

windowfocus3.cxx
2k

#4	ianmacarthur 09:02 Sep 04, 2014

windowfocus4.cxx
1k

#5	cand 06:59 Sep 11, 2014

fltk-show-sync.patch
4k

#6	cand 00:18 Sep 12, 2014

fltk-show-sync-v2.patch
4k

#7	cand 23:46 Sep 16, 2014

fltk-show-sync-v3.patch
5k

#8	AlbrechtS 09:24 Sep 17, 2014

wait_for_expose.patch
2k

#9	AlbrechtS 09:37 Sep 25, 2014

wait_for_expose_v2.patch
2k

#10	cand 00:39 Sep 26, 2014

windowtest.patch
2k

Trouble Report Comments:

#1	cand 12:44 Sep 02, 2014

On Linux, take_focus across windows seems to work only partially. If you have existing windows, you can pass the focus between them fine, but if you need to pop a new window and restore focus to the spawning window, it doesn't work. Attaching sample code. I'd appreciate if someone could confirm it's working on other platforms (win/mac).

#2	ianmacarthur 07:54 Sep 04, 2014

I had initially thought this problem was *not* restricted to X11, but was in fact widespread. However, testing Lauri's example windowfocus.cxx on Win7 I *do not* see the failure, so the fault may be X11 specific after all... I attach an slightly extended windowfocus2.cxx example by way of seeing something of where the focus actually is, when it goes awry...

#3	AlbrechtS 08:36 Sep 04, 2014

FWIW, I don't know if this is related to the issue in this STR, however it seems, so I just want to let you know. There are indeed platform specific differences when showing windows, because X needs at least two or three messages (client-server-turnaround) to show a new window. This can be seen in callbacks when you show a window, or maybe in splash-screens when showing a window before Fl::run() is called. The effect is something like this: win->show(); // pop up window Fl::wait(); // wait for window to show [1] while (win->shown()) { do something; ... } This works on non-X platforms, but the call to Fl::wait() at [1] is not sufficient to show the window. Under Linux you may need two or more Fl::wait() calls to succeed. That or something similar... This *may* explain the different focus behavior (or not). Hope this helps, but I can't contribute more right now. Sorry.

#4	chris 08:37 Sep 04, 2014

Setting win2->set_menu_window() or win2->set_override() does the trick here.

#5	ianmacarthur 08:59 Sep 04, 2014

OK, some more testing, here's what I see. Basically, there's a race of sorts going on here. As Albrecht notes, actually showing a window under X takes more "server trips" than it does on OSX or WIN32. The net result, for this use case, is that if you show() a window then immediatley try to pass the focus *away* from that window to some other widget, that *may* not work, since under X the window is not even mapped yet... it gets mapped "later" when the server gets around to it - at which point it gains the foucs. But this is *after* we have executed the code to "give away" the focus to the input widget in Lauri's test code... See windowfocus3.cxx above, to which I have added some debug to see what's going on, including a timeout so I could see the focus moving from one widget to another "after the fact". However, windowfocus3.cxx "works correctly" as I've added a workaround to allow the callback to wait until the window is mapped before giving away the focus. This may be "good enough" for Lauri's desired use-case in the short term. The real fix is harder, as under X we are always at the mercy of the server in terms of *when* a window will actually be mapped... Thoughts?

#6	ianmacarthur 09:03 Sep 04, 2014

The file windowfocus4.cxx is just windowfocus3 with the debug stuff stripped out, so it is my proposed "working example" for the workaround...

#7	cand 11:37 Sep 04, 2014

I'd really like to get a proper fix. I tested adding this to the end of Fl_Window::show() in Fl_x.cxx: while (!Fl_X::i(this) || Fl_X::i(this)->wait_for_expose) { Fl::wait(0); } and it correctly blocks until we have a window. I would think show() blocking until the window is up is quite ok, even expected behavior, but I'm not sure if this wouldn't break a corner use case. The above code alone is not enough to fully fix the bug, though. While it prevents the initial focus steal, passing focus between windows doesn't correctly pass the focus to the new window, causing the focus to break when I move the mouse over either window. It would seem that a focus change should check if the window changed, and if so, call XSetInputFocus on the new window. I'll open a new STR for this part.

#8	ianmacarthur 13:35 Sep 04, 2014

I'm uneasy about show() *always* waiting for the window to be mapped, since in a networked environ that might be a long time; or never... The current asynchronous nature is part of the way X works, but it is a nuisance. This is why I think a "proper" fix is hard. The issue with not setting the focus correctly between windows once mapped does look like it may be amenable to a bit of tweaking. That said I spent some time staring at the code where we interact with Fl::focus(), widget::take_focus(), fl_fix_focus(), fl_throw_focus()... and so on, and it is a bit of a rabbit warren, so I lost my way a bit...!

#9	cand 00:03 Sep 05, 2014

How about making it a global option then, default to current behavior?

#10	cand 07:00 Sep 11, 2014

Attaching proposed patch. It also includes a modified version of windowfocus.cxx under test/.

#11	manolo 23:52 Sep 11, 2014

The proposed patch states "Note that this will block threads that use Fl::lock() for syncronization for a short time." In fact, the Fl::wait() call within show_sync() unlocks, if anything. But it may be desirable to have show_sync truely block threads until the window is mapped. This would be void Fl_Window::show_sync() { Fl::lock(); show(); while (!i || i->wait_for_expose) { Fl::wait(); } Fl::unlock(); } may be? Also, just use i instead of Fl_X::i(this) inside the Fl_Window class.

#12	cand 00:18 Sep 12, 2014

Posted v2 with those modifications.

#13	ianmacarthur 15:08 Sep 13, 2014

Apropos of nothing much - in the patch we say that OSX and Win32 are both "show synchronous" by default... I recall that Manolo said that OSX was, but do we know that Win32 is? I have a vague recollection of getting caught out in the past with windows showing (or rather not showing) on Win32, though I don't recall the circumstances...

#14	cand 04:37 Sep 14, 2014

I relied on comment #2 saying the test case works on Windows.

#15	cand 00:23 Sep 16, 2014

Checking Fl_win32.cxx, it calls ShowWindow for new windows and BringWindowToTop for old windows. Both of these are blocking calls according to documentation. If there are no objections, I'll commit tomorrow.

#16	AlbrechtS 17:47 Sep 16, 2014

Patch (v2) looks good so far, but I'd like to test the Windows case more thoroughly, probably tomorrow. WRT blocking threads: true, v1 didn't block threads (my fault), because Fl::wait() calls Fl::unlock() and Fl::lock(), patch v2 does. But do we really want this, or should we remove the comment about blocking threads. Maybe blocking the main thread (that calls show_sync()) with Fl::wait() until the window is shown would be enough. But there's another point: we should really also have [virtual] show_sync(int argc, char **argv) because that would be the typical call for using splash screens (the first window shown in such a scenario). Maybe not trivial to do however w/o a little code refactoring.

#17	cand 23:47 Sep 16, 2014

Posted v3 with show_sync(int argc, char **argv).

#18	AlbrechtS 05:21 Sep 17, 2014

Thanks for fltk-show-sync-v3.patch. It was late yesterday, and I was distracted by all that argc/argv stuff I saw. Of course it was simple to implement show_sync(argc,argv).

#19	AlbrechtS 05:39 Sep 17, 2014

I apologize for the confusion in advance, but I thought about it and maybe my own proposal of show_sync() was not the best way. We would have to add two overloaded virtual methods, and the questions about blocking threads are not resolved yet (I believe that both would be okay, but I'd tend to use a non-locking variant now). I want to come back to Ian's proposal somewhere in fltk.coredev. Maybe this is the better way to do it, because we need only one new and very simple method, and this could probably be non-virtual. Something like this would probably suffice: /** Waits for a window to be fully shown after calling show(). [ ... more from the docs as proposed earlier, but w/o the comment about blocking threads, because it's non-blocking when calling Fl::wait() as Manolo wrote earlier. ] */ void Fl_Window::wait_for_expose() { while (!i || i->wait_for_expose) { Fl::wait(); } } If we'd really want to block threads we could make that optional, as in: FL/Fl_Window.H: void Fl_Window::wait_for_expose(int blocking=0); Implementation, as proposed by Manolo with optional internal locking: void Fl_Window::wait_for_expose(int blocking) { if (blocking) Fl::lock(); while (!i || i->wait_for_expose) { Fl::wait(); } if (blocking) Fl::unlock(); } Note that this would utilize the recursive locking of Fl::lock() which is (AFAICT) not fully documented, but I've verified it in Windows and Unix (pthread) code versions. I don't know about the Mac code, unless we use pthreads on OS X as well. Do we? But that's probably moot, since wait_for_expose() would be a no-op on Windows and OS X anyway. [Note: I did not test Windows yet. Need more time, sorry.]

#20	AlbrechtS 09:24 Sep 17, 2014

See attached file wait_for_expose.patch for my proposal with Fl_Window::wait_for_expose(int blocking=0); Personally I would prefer not to have the blocking option at all, and this can be removed easily. I don't think that it is needed, since a programmer could easily wrap his code in Fl::lock() ... Fl::unlock() as well. The doxygen comment is somewhat long, maybe this is not all needed. Comments, please, which version to use: (a1) show_sync() or (a2) wait_for_expose() (b1) blocking version or (b2) non-blocking version (c) maybe a better name for "wait_for_expose()" ? My vote would be (a2) and (b2), i.e. blocking argument removed from wait_for_expose.patch.

#21	AlbrechtS 09:26 Sep 17, 2014

Note: wait_for_expose() is neither virtual nor platform-dependent in this patch. Tested on Windows and Linux.

#22	cand 11:49 Sep 17, 2014

a2 & b2 is ok by me. Note that the test app should still be committed, otherwise there is no test for this functionality included, and so it might break in the future.

#23	AlbrechtS 05:13 Sep 18, 2014

Yep, agreed (test app).

#24	AlbrechtS 09:37 Sep 25, 2014

Please review and test wait_for_expose_v2.patch. This has been compiled w/o problems on Windows (MinGW) and Linux. I intend to commit this patch tomorrow, if there are no objections. Changes: (1) No "blocking" options, i.e. no Fl::lock() and Fl::unlock(). (2) Documentation is in source file. Todo: Fix test program and test. Lauri, if you can adapt your test program and post it, feel free to do it. I won't have time to work on this until tomorrow afternoon (MET).

#25	cand 00:40 Sep 26, 2014

Posted updated test program as windowtest.patch. Working fine here.

#26	AlbrechtS 17:14 Sep 26, 2014

Fixed in Subversion repository. Note: there are some small additions, mainly svn attributes and $Id$ tags in following commits after 10339, but 10339 is the real fix version.

#27	AlbrechtS 17:15 Sep 26, 2014

PS: Lauri, thanks for the updated test program.

#28	cand 01:52 Sep 27, 2014

All good, closing.