Subject: Re: stacking chain

From: Michael Han <[email-masked]>
Date: Sun, 14 May 2000 13:44:58 -0700

Previously from Michael Han
>Hi, I've been trying to track down the problem I occassionally see
>with the stacking chain getting out of order, allowing transients to
>be raised above windows in layers higher than themselves. I could use
>some help understanding what's supposed to happen in
>RaiseOrLowerWindow().

I think I understand better...

>Basically, add_window.c's AddWindow() calls
>postition_new_window_in_stack_ring() after pushing the window on top
>of the stack. This results in a RaiseOrLowerWindow(), which seems to
>be where something's not working properly. Following the code in
>RaiseOrLowerWindow, I'm having trouble figuring out why transients
>should do anything *but* sit on top of the stack regardless of layers,
>assuming that no_movement gets set. Or am I saying the wrong thing
>here? Should a transient always cause no_movement to be False?

It took me a while to figure out that the recursive
RaiseOrLowerWindow() kicked off on the transient window's transientfor
is where the magic supposed to happen. But the trouble seems to be
that must_move_transients() somehow gets fooled into thinking it
needn't move the transient, while RaiseOrLowerWindow() is executing
for the transientfor parent. I think this is what happens:

Three windows: xterm(layer 5); FvwmConsole(layer 4); editres(layer 4);
creating a transient for editres named xt(layer 4):

If the chain is: xt; xterm; editres; FvwmConsole; upon entry into
RaiseOrLowerWindow() you'll recurse into RaiseOrLowerWindow(editres)
and when must_move_transients() gets called:

1) the first window is the transient, which sets *found_transient;
2) the next window is xterm, in a higher layer, giving continue;
3) the next window is editres, which unsets scanning_above_window
4) the next window is FvwmConsole, which triggers no action
5) the loop breaks, returning False

But if the chain is: xt; xterm; FvwmConsole; editres; this is the
procedure:

1) the first window is the transient, which sets *found_transient;
2) the next window is xterm, in a higher layer, giving continue;
3) the next window is FvwmConsole, which triggers the final condition,
returning True

The stacking chain logic is fairly opaque to me. I'm not sure how best
to fix the problem. I'm guessing that maybe some sort of sanity check
that actually keeps track of whether transients have been found above
the higher-layer windows is necessary.

Here's a patch to do that. It seems minimally correct, though I'm not
positive that layers must be unsigned values, since the data type is
int... Still, if that's the case (and I've always understood things
this way), it's safe to temporarily leave the layer set to -1 as a
guarantee that no real layer has been set yet.
-- 
[email-masked]                         http://www.best.com/~mikehan/
coffee achiever                              San Francisco, California
"I will not be a snickerpuss." - Lisa

Index: fvwm/stack.c
===================================================================
RCS file: /home/cvs/fvwm/fvwm/fvwm/stack.c,v
retrieving revision 1.32
diff -u -r1.32 stack.c
--- fvwm/stack.c	2000/01/25 14:38:59	1.32
+++ fvwm/stack.c	2000/05/14 20:36:37
@@ -216,6 +216,8 @@
 static Bool must_move_transients(FvwmWindow *t, Bool do_lower,
 				 Bool *found_transient)
 {
+  int last_layer = -1;
+  int out_of_order = 0;
   *found_transient = False;
 
   /* raise */
@@ -227,6 +229,10 @@
 
     for (q = Scr.FvwmRoot.stack_next; q != &Scr.FvwmRoot; q = q->stack_next)
     {
+      if ((last_layer > -1) && (last_layer < q->layer))
+      {
+	out_of_order++;
+      }
       if (t->layer < q->layer)
       {
 	/* We're not interested in higher layers. */
@@ -259,9 +265,13 @@
 	 * lower. */
 	return True;
       }
+      last_layer = q->layer;
     }
   }
-  return False;
+  if (out_of_order)
+    return True;
+  else
+    return False;
 }
 
 static void RaiseOrLowerWindow(

---

From: Dominik Vogt <[email-masked]>
Date: Mon, 15 May 2000 15:33:13 +0200

> the higher-layer windows is necessary.

Okay, if I understand this correctly, RaiseOrLowerWindow ends up with

  do_move_transients = False
  found_transient = True

==> no_movement = True

I think the reason is that the RaiseOrLowerWindow() function is called
by the recursion although the stack ring is currently out of order
(because of the transient being placed at the beginning of the stack
ring although there are windows in higher layers behind it).  Then,
moust_move_transients wrongly assumes that transients are placed
properly in the stack ring.  If this theory is correct, it should be
possible to fix the problem by giving the RaiseOrLowerWindow function
a 'currently_adding_a_new_window' flag and disabling recursion in this
case.

Mike, I've committed an experimental 'fix'.  Can you test it, please?

Bye

Dominik ^_^

--
Dominik Vogt, Agilent Technologies, Dept. BVS
Herrenberger Str.130, 71034 Boeblingen, Germany
phone: 07031/464-4596, fax: 07031/464-3883, [email-masked]
--
Visit the official FVWM web page at <URL:http://www.fvwm.org/>.
To unsubscribe from the list, send "unsubscribe fvwm-workers" in the
body of a message to [email-masked].
To report problems, send mail to [email-masked]

---

From: Dominik Vogt <[email-masked]>
Date: Mon, 15 May 2000 22:25:22 +0200

> Mike, I've committed an experimental 'fix'.  Can you test it, please?

Finally I can reproduce the problem too.  It only happens all of the
following conditions are met:

 1) The style StackTransientParent is set for the new transient.
 2) Either RaiseTransient or LowerTransient is set for the parent
    window.
 3) There is a window on a higher layer.

E.g.

  Style * RaiseTransient, StackTransientParent
  Style xterm layer 5

will do.  Your analysis was quite correct, Mike - good work.  Now
let's see if my patch works...

--- three minutes later ---

Yup, it does (I guess yours does too, Mike).  Now let's see if it
does the right thing...

--- ten minutes later ---

Nope, it doesn't.  Now it fails to raise the parent window when
a new transient is mapped.

--- thirty minutes later ---

Okay, I got it straigt now.  Basically the fix works like this:
RaiseOrLowerWindow() has a new parameter 'is_new_window'.  New
windows are simply raised/lowered without doing all the fancy
stuff in must_move_transients() or in the recursion.  Then,
further down, if the new window is transient and has set
STACK_TRANSIENT_PARENT, RaiseOrLowerWindow() calls itself, now
with is_new_window unset.  This duplicate call makes sure that
the parent is positioned properly below the new transient.

Thanks for the excellent analysis of the problem, Mike.  It
made fixing the problem child's play.

Bye

Dominik ^_^

--
Dominik Vogt, [email-masked]
Reply-To: [email-masked]
--
Visit the official FVWM web page at <URL:http://www.fvwm.org/>.
To unsubscribe from the list, send "unsubscribe fvwm-workers" in the
body of a message to [email-masked].
To report problems, send mail to [email-masked]

---

From: Michael Han <[email-masked]>
Date: Mon, 15 May 2000 19:44:13 -0700

Previously from Dominik Vogt
>>   do_move_transients = False

Precisely.

> 3) There is a window on a higher layer.

Yes, these are requirements, but they are also default settings (well,
1 and 2 are). At least, they seem to be on my machine.

>made fixing the problem child's play.

I'm still seeing problems if I create transients for a window already
at the top of the layer. Looking over your patches, I don't see why
this isn't working on my machine. But it's been a long day of
struggling with crappy software, so I'll wait a while before thinking
about this again.

Anyway, I'm glad you can reproduce the problem now. I was beginning to
think that I made a mistake upgrading to XFree86 4.0.
-- 
[email-masked]                         http://www.best.com/~mikehan/
coffee achiever                              San Francisco, California
"I will not be a snickerpuss." - Lisa
--
Visit the official FVWM web page at <URL:http://www.fvwm.org/>.
To unsubscribe from the list, send "unsubscribe fvwm-workers" in the
body of a message to [email-masked].
To report problems, send mail to [email-masked]

---

From: Dominik Vogt <[email-masked]>
Date: Wed, 17 May 2000 08:45:31 +0200

On Mon, May 15, 2000 at 07:44:13PM -0700, Michael Han wrote:
> this isn't working on my machine.

When I wrote the patch I was a bit worried that the double raise
of new transients might cause them to be buried under their
transientfor window if the FlipTransient style is activated.

[testing...]

Um, and that's exactly what happens :(  Try using

  style * fliptransient, raisetransient, stacktransientparent
  style * lowertransient, clicktofocus

Then open netscape and e.g. the preferences transient.  I'll
make another patch.

> But it's been a long day of struggling with crappy software,
> so I'll wait a while before thinking about this again.

I hope you don't means fvwm when you say 'crappy software' 8-)

Bye

Dominik ^_^

---

From: Dominik Vogt <[email-masked]>
Date: Wed, 17 May 2000 10:25:44 +0200

> Then open netscape and e.g. the preferences transient.  I'll

:-(  No way to fix that.  The FlipTransient style really does
not work acceptably, and I don't see a way how to make it work.
It works only in a very limited set of configurations.  I think
it might be best to simply remove it before people start using
it.

Bye

Dominik ^_^

---

From: Michael Han <[email-masked]>
Date: Wed, 17 May 2000 07:22:18 -0700

>it might be best to simply remove it before people start using

I'm assuming there's a case that my proposed patch didn't adequately
cover? For the default configurations, it was getting things correct.
Incidentally, after sending it to the list, I rewrote it because what
I sent to the list was stupid: instead, it stores only a Bool
out_of_order, and in the comparison to see if the chain got broken, it
does a:

      if ((q->stack_next != &Scr.FvwmRoot) &&
          (q->layer < q->stack_next->layer))

Then it just returns the out_of_order flag if it manages to make its
way to the end of the function's block.

Well, I don't know what to do about FlipTransient.
-- 
[email-masked]                         http://www.best.com/~mikehan/
coffee achiever                              San Francisco, California
"What was your username?" - BOFH
--
Visit the official FVWM web page at <URL:http://www.fvwm.org/>.
To unsubscribe from the list, send "unsubscribe fvwm-workers" in the
body of a message to [email-masked].
To report problems, send mail to [email-masked]

---

From: Dominik Vogt <[email-masked]>
Date: Wed, 17 May 2000 18:26:29 +0200

On Wed, May 17, 2000 at 07:22:18AM -0700, Michael Han wrote:
> cover?

Sorry, I didn't even try out your patch.  I tried to find out
why the broken call of must_move_transients() was made in the
first place and came to the conslusion that the code failed to
handle the case when new windows trigger the recursion.  I tried
to fix the root cause rather than the symptom.

> For the default configurations, it was getting things correct.

Well, in the end your patch is much simpler.

> Well, I don't know what to do about FlipTransient.

I guess the FlipTransient bug is not a new one.  I'm sure it was
there before.  The problem here is *not* that the new window triggers
a double raise.  What happens is that a second raise is triggered
when you release the mouse button in netscape's 'Edit' menu over
'Preferences'.  Well, I can't describe it properly, but

 1) FlipTransient does not work as expected anyway (e.g. if you
    pree on the title to move a window)
 2) I have no idea how to code FlipTransient correctly.
 3) It causes other strange effects in some setups.
 
==> We should remove it before 2.4, i.e. before it can make
    its way into any stable release.

Bye

Dominik ^_^

---

From: Michael Han <[email-masked]>
Date: Thu, 18 May 2000 18:44:08 -0700

>to fix the root cause rather than the symptom.

Yes, this is the sensible approach, when you have a sense of what's
*supposed* to happen ;> Seriously though, I haven't given this problem
a whole lot of thought, but it seems to me that in essence,
RaiseOrLowerWindow() wants to raise top level windows, which seems a
valid approach to handle stacking.

>    its way into any stable release.

Having observed above that I think RaiseOrLowerWindow() wants to raise
toplevels, not transients, I don't find it terribly surprising that
FlipTransient causes a lot of trouble. It would be very difficult for
new windows not to get caught in FlipTransient logic.

I have no real comment on jettisoning FlipTransient. I don't use it.
-- 
[email-masked]                         http://www.best.com/~mikehan/
coffee achiever                              San Francisco, California
There is too much blood in my caffeine system
--
Visit the official FVWM web page at <URL:http://www.fvwm.org/>.
To unsubscribe from the list, send "unsubscribe fvwm-workers" in the
body of a message to [email-masked].
To report problems, send mail to [email-masked]

---

From: Dominik Vogt <[email-masked]>
Date: Fri, 19 May 2000 10:57:08 +0200

> I have no real comment on jettisoning FlipTransient. I don't use it.

I wrote it half a year ago because it thought it was a good
idea.  But I never came up with a decent implementation and
now it causes more trouble than it helps.

Bye

Dominik ^_^