On Sat, Jun 29, 2019 at 03:02:30PM +0200 I heard the voice of
Rhialto, and lo! it spake thus:
>
> Looking at my test case (Xine), it has indeed a small window with
> playback controls, and it is also a transient of the main window
> (fullscreenable).
Ah, well, see? If you just stuck with mpv or mplayer like me, you
wouldn't have a transient, and wouldn't have these problems :p
> _NET_WM_WINDOW_TYPE(ATOM) = _NET_WM_WINDOW_TYPE_TOOLBAR
>
> which sounds weird to be, but it definitely complicates window
> stacking!
Well, luckily, we don't know or care anything about TOOLBAR, so it
doesn't hurt anything itself.
> I didn't get a reliable way to get the crash. I just noticed it mostly
> seemed to occur if I was switching to another workspace while the
> main window fullscreened, or when switching away from fullscreen mode.
> I didn't take notice of any other windows. I guess I'll experiment a bit
> more.
Interesting. In both cases, that sounds like you're finding the crash
when the window is on its way _down_, whereas my method hits it on the
way _up_. Interesting. Now, apart from the fullscreen thing, I don't
think I ever have any windows with OTP != 0; maybe if you've got
something at e.g. +2, that the FS window and its transient have to
pass in both directions...?
> My first impression, after reading your description, was that we have
> found different problems. But now that my Xine Panel window turns out to
> be transient as well, maybe it's the same one, or at least more related
> than I thought. It is a fairly small window. But when taking focus away
> from the main window, it also changes the calculated proper position of
> the panel window.
Yeah. The trickiness apparently is that a window and its transients
really need to move around as a group. I mean, conceptually they
don't, but with our OTP implementation, I think it's probably way more
troublesome if they don't. And that winds up mostly being the case,
but there are a few edge cases where I think you can probably wind up
with them in different places (f.setpriority on a transient itself,
frex, or _ABOVE/_BELOW flags, etc). All the OTP code dealing with
transients seems to assume they're always on the same effective level
as the main window, but that's not actually enforced anywhere. I'll
bet you'll cause a _lot_ of trouble, if you arrange those
situations...
The focus handling has been calling OtpRestackWindows(), which calls
InsertOwl(). Which seemingly assumes that all of the window's
transients are on the top of the current OTP layer, copies in the
current base priority, and then... raises all the small transients
above all the windows below the last one that loop found? And then
puts this window right below them. The logic is kinda odd, but
apparently that's purely handling TransientOnTop stuff (which is why
cranking TOT lets me hack around my crash).
But for our case, we want to act more like f.setpriority, in that we
need to assume all our transients are shifting to the new layer with
us. I did a little noodling around the tree below
OtpRestackWindows(), but I couldn't come up with any good ideas that
didn't have obviously wacky side effects. So I think we just need to
adjust what the focus handlers call.
I came up with the attached patch, which does fix my crash. The major
side effect is that the window's transients all always wind up back on
top of it when it moves up/down. That's not ideal, but it's better
than a crash, and putting them all below it doesn't sound any better.
Really, the whole interaction of the transients + fullscreen/focus is
wonky and needs some rethinking. Actually, one reason I haven't
already added a StopDoingOTPCrapOnFullscreenFocused setting is 'cuz
I'd probably just set it and never think about the things that really
need fixing in it...
So, give the attached a whirl. Especially if you can find a way to
trigger the crash at will, so we can actually know whether it's
addressing the matter. If it succeeds, I'd like to go back to just
before the RLayout/RANDR changes, apply it there, and cut a 4.0.3 with
that and the few other little things that happened early enough; 'd be
good to get that fix out in a point release.
--
Matthew Fuller (MF4839) | [email protected]
Systems/Network Administrator | http://www.over-yonder.net/~fullermd/
On the Internet, nobody can hear you scream.
=== modified file 'otp.c'
--- otp.c 2018-08-19 22:50:13 +0000
+++ otp.c 2019-06-30 22:47:26 +0000
@@ -1579,6 +1579,104 @@
}
+
+/**
+ * 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 in its old layer 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 roughly a reimplementation of TryToMoveTransientsOfTo(),
+ // but we can't use that func itself because we already moved the
+ // focus, so PRI(transients) won't match PRI(thiswin), and we won't
+ // find them. See above for uncertainty about this guess as to where
+ // to find them... we should have a better way of finding a
+ // window's transients than we currently do :|
+ OtpWinList *trans = OwlRightBelow(oldprio);
+ trans = (trans == NULL) ? Scr->bottomOwl : trans->above;
+ while((trans != NULL)) {
+ OtpWinList *next = trans->above;
+
+ if((trans->type == WinWin)
+ && isTransientOf(trans->twm_win, twm_win)) {
+ // Bounce this one
+ RemoveOwl(trans);
+ InsertOwl(trans, Above);
+ }
+ else {
+ // Have we left the layer we started in?
+ if(PRI(trans) != oldprio) {
+ break;
+ }
+ }
+
+ 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-06-30 21:49:05 +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-06-30 22:39:40 +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;
}
