On Wed, Jul 10, 2019 at 10:28:15PM +0200 I heard the voice of
Rhialto, and lo! it spake thus:
>
> [...] None of the patches attempting to fix these bugs.
Hm. I read this as "none of the patches fixed these bugs", but now I
think it means "I didn't try it patched". Is that right?
> #4 OtpRestackWindow (twm_win=twm_win@entry=0x74ba18b40c00)
> at /mnt/vol1/rhialto/cvs/other/ctwm/bzr/trunk/otp.c:1626
> #5 0x000000000042f705 in SetFocus (tmp_win=0x74ba18b40800, tim=<optimized
> out>)
> at /mnt/vol1/rhialto/cvs/other/ctwm/bzr/trunk/win_ops.c:168
is definitely a non-patched backtrace, since SetFocus() doesn't call
Restack with the patch. I can reproduce that crash with xine and
roughly your steps (just mousing, not warp'ing, since that doesn't
quite work right in xnest/xephyr) with trunk, but with a patched tree
I can't.
Looking into it did turn up a few more cases; we need to make similar
changes in the FocusIn/FocusOut handlers. As far as I can tell from
reading and thinking, anyway. It's extremely hard to actually trigger
them, since with ClickToFocus we just call SetFocus(), and without we
get EnterNotify -> SetFocus. So, I don't have a good before/after
test on it, but it should be right.
So, updated patch attached; as far as I can tell, this (and the
previous variant) should fix that crash, and if that's also your
historic one, should cleanse the world.
> Now I use the window ring to bring focus to W which comes up
> partially on top of P. Now I move the mouse directly into P. (To my
> surprise, W did not disappear behind a rising F).
Correct; in this case, F doesn't rise, only P does. That's what I've
tentatively classified as a cosmetic-but-not-correctness bug. Should
fix that in trunk, but if we can't use it to crash things I'd like to
skip for backporting, for minimalism's sake.
> Sometimes I notice another weirdness: despite using f.warpring to go
> to W, it does not always come to the top as it should (since F isn't
> focused any more, it has no reason to stay on top of W).
When F loses focus, it will drop back down to its base OTP layer,
which would be the same layer as W. But it'll wind up as the _top_
thing on that layer, so it'll still be above W.
--
Matthew Fuller (MF4839) | [email protected]
Systems/Network Administrator | http://www.over-yonder.net/~fullermd/
On the Internet, nobody can hear you scream.
=== modified file 'event_handlers.c'
--- event_handlers.c 2018-11-19 06:15:52 +0000
+++ event_handlers.c 2019-07-11 02:13:12 +0000
@@ -367,35 +367,36 @@
static void
HandleFocusIn(void)
{
-#ifdef EWMH
- TwmWindow *old_focus = Scr->Focus;
-#endif
-
if(! Tmp_win->wmhints->input) {
return;
}
if(Scr->Focus == Tmp_win) {
return;
}
+
+#ifdef EWMH
+ // Handle focus-dependent re-stacking of what we're moving out of.
+ if(Scr->Focus && OtpIsFocusDependent(Scr->Focus)) {
+ OtpUnfocusWindow(Scr->Focus);
+ // NULL's Scr->Focus
+ }
+#endif
+
if(Tmp_win->AutoSqueeze && Tmp_win->squeezed) {
AutoSqueeze(Tmp_win);
}
SetFocusVisualAttributes(Tmp_win, true);
+#ifdef EWMH
+ // Handle focus-dependent re-stacking of what we're moving in to.
+ if(Tmp_win && OtpIsFocusDependent(Tmp_win)) {
+ OtpFocusWindow(Tmp_win);
+ // Sets Scr->Focus
+ }
+#endif
+
+ // Redundant in EWMH case
Scr->Focus = Tmp_win;
-
-#ifdef EWMH
- /*
- * Some EWMH flags may affect stacking, so after we change
- * Scr->Focus...
- */
- if(old_focus && OtpIsFocusDependent(old_focus)) {
- OtpRestackWindow(old_focus);
- }
- if(Tmp_win && OtpIsFocusDependent(Tmp_win)) {
- OtpRestackWindow(Tmp_win);
- }
-#endif
}
@@ -413,18 +414,20 @@
}
SetFocusVisualAttributes(Tmp_win, false);
- Scr->Focus = NULL;
-
#ifdef EWMH
/*
* X-ref HandleFocusIn() comment. FocusOut is only leaving a window,
* not entering a new one, so there's only one we may need to
* restack.
*/
- if(Tmp_win && OtpIsFocusDependent(Tmp_win)) {
- OtpRestackWindow(Tmp_win);
+ if(Scr->Focus && OtpIsFocusDependent(Scr->Focus)) {
+ OtpUnfocusWindow(Scr->Focus);
+ // NULL's Scr->Focus
}
#endif
+
+ // Redundant in EWMH case
+ Scr->Focus = NULL;
}
=== modified file 'otp.c'
--- otp.c 2018-08-19 22:50:13 +0000
+++ otp.c 2019-07-07 04:44:45 +0000
@@ -1579,6 +1579,99 @@
}
+
+/**
+ * Focus/unfocus backend. This is used on windows whose stacking is
+ * focus-dependent (e.g., EWMH fullscreen), to move them and their
+ * transients around. For these windows, getting/losing focus is
+ * practically the same as a f.setpriority, except it's on the calculated
+ * rather than the base parts. And it's hard to re-use our existing
+ * functions to do it because we have to move Scr->Focus before the main
+ * window changes, but then it's too late to see where all the transients
+ * were.
+ *
+ * There are a number of unpleasant assumptions in here relating to where
+ * the transients are, and IWBNI we could be smarter and quicker about
+ * dealing with them. But this gets us past the simple to cause
+ * assertion failures, anyway...
+ */
+static void
+OtpFocusWindowBE(TwmWindow *twm_win, int oldprio)
+{
+ OtpWinList *owl = twm_win->otp;
+
+ // This one comes off the list, and goes back in its new place.
+ RemoveOwl(owl);
+ InsertOwl(owl, Above);
+
+ // Now root around for any transients of it, and
+ // nudge them into the new location. The whole Above/Below thing is
+ // kinda a heavy-handed guess, but...
+ //
+ // This is nearly a reimplementation of TryToMoveTransientsOfTo(),
+ // but the assumption that we can find the transients by starting
+ // from where the old priority was in the list turns out to be deeply
+ // broken. So just walk the whole thing. Which isn't ideal, but...
+ //
+ // XXX It should not be this freakin' hard to find a window's
+ // transients. We should fix that more globally.
+ OtpWinList *trans = Scr->bottomOwl;
+ while((trans != NULL)) {
+ // Gotta pre-stash, since we're sometimes about to move trans.
+ OtpWinList *next = trans->above;
+
+ if((trans->type == WinWin)
+ && isTransientOf(trans->twm_win, twm_win)) {
+ // Bounce this one
+ RemoveOwl(trans);
+ InsertOwl(trans, Above);
+ }
+
+ trans = next;
+ }
+
+ OtpCheckConsistency();
+}
+
+/**
+ * Unfocus a window. This needs to know internals of OTP because of
+ * focus-dependent stacking of it and its transients.
+ */
+void
+OtpUnfocusWindow(TwmWindow *twm_win)
+{
+ // Stash where it currently appears to be. We assume all its
+ // transients currently have the same effective priority. See also
+ // TryToMoveTransientsOfTo() which makes the same assumption. I'm
+ // not sure that's entirely warranted...
+ int oldprio = PRI(twm_win->otp);
+
+ // Now tell ourselves it's unfocused
+ assert(Scr->Focus == twm_win);
+ Scr->Focus = NULL;
+
+ // And do the work
+ OtpFocusWindowBE(twm_win, oldprio);
+}
+
+/**
+ * Focus a window. This needs to know internals of OTP because of
+ * focus-dependent stacking of it and its transients.
+ */
+void
+OtpFocusWindow(TwmWindow *twm_win)
+{
+ // X-ref OtoUnfocusWindow() comments.
+ int oldprio = PRI(twm_win->otp);
+
+ assert(Scr->Focus != twm_win);
+ Scr->Focus = twm_win;
+
+ OtpFocusWindowBE(twm_win, oldprio);
+}
+
+
+
/*
* Calculating effective priority. Take the base priority (what gets
* set/altered by various OTP config and functions), and then tack on
=== modified file 'otp.h'
--- otp.h 2017-06-01 15:51:31 +0000
+++ otp.h 2019-07-07 04:42:48 +0000
@@ -61,6 +61,9 @@
void OtpStashAflagsFirstTime(TwmWindow *twm_win);
void OtpRestackWindow(TwmWindow *twm_win);
+void OtpUnfocusWindow(TwmWindow *twm_win);
+void OtpFocusWindow(TwmWindow *twm_win);
+
/* functions to manage the preferences. The second arg specifies icon prefs */
void OtpScrInitData(ScreenInfo *);
name_list **OtpScrSwitchingL(ScreenInfo *, WinType);
=== modified file 'win_ops.c'
--- win_ops.c 2018-11-18 21:09:02 +0000
+++ win_ops.c 2019-07-07 04:42:48 +0000
@@ -129,9 +129,6 @@
{
Window w = (tmp_win ? tmp_win->w : PointerRoot);
bool f_iconmgr = false;
-#ifdef EWMH
- TwmWindow *old_focus = Scr->Focus;
-#endif
if(Scr->Focus && (Scr->Focus->isiconmgr)) {
f_iconmgr = true;
@@ -153,24 +150,34 @@
AutoSqueeze(Scr->Focus);
}
SetFocusVisualAttributes(Scr->Focus, false);
+#ifdef EWMH
+ // Priority may change when focus does
+ if(OtpIsFocusDependent(Scr->Focus)) {
+ OtpUnfocusWindow(Scr->Focus);
+ // That Scr->Focus = NULL's internally for us, but we don't
+ // care, since we're about to reset it if we need to.
+ }
+#endif
}
- if(tmp_win) {
+
+ if(tmp_win) {
if(tmp_win->AutoSqueeze && tmp_win->squeezed) {
AutoSqueeze(tmp_win);
}
SetFocusVisualAttributes(tmp_win, true);
+#ifdef EWMH
+ // Priority may change when focus does
+ if(OtpIsFocusDependent(tmp_win)) {
+ OtpFocusWindow(tmp_win);
+ // Pre-sets Scr->Focus
+ }
+#endif
}
+
+ // in the EWMH cases, this was already done.
Scr->Focus = tmp_win;
-#ifdef EWMH
- /* Priority may change when focus does */
- if(old_focus && OtpIsFocusDependent(old_focus)) {
- OtpRestackWindow(old_focus);
- }
- if(tmp_win && OtpIsFocusDependent(tmp_win)) {
- OtpRestackWindow(tmp_win);
- }
-#endif
+ return;
}
