Subject: Re: FVWM: gkrellm and FVWM v2.5.8

From: Olivier Chapuis <[email-masked]>
Date: Fri, 7 Nov 2003 16:16:04 +0100

On Fri, Nov 07, 2003 at 02:36:55PM +0100, Dominik Vogt wrote:
> On Fri, Nov 07, 2003 at 01:10:43PM +0000, Mikhael Goikhman wrote:
> > On 07 Nov 2003 10:28:16 +0100, Dominik Vogt wrote:
> > > 
> > > On Thu, Nov 06, 2003 at 12:14:18PM -0500, Ben Winslow wrote:
> > > > On Thu, 2003-11-06 at 03:33, Dominik Vogt wrote:
> > > > > On Wed, Nov 05, 2003 at 11:17:21PM -0500, Ryan Daly wrote:
> > > > > > I'm noticing something strange with 2.5.8 and gkrellm (2.1.21).
> > > > > > 
> > > > > > I click in the top area to move the window and go to move the mouse, but 
> > > > > > the window stays put.  After about 2 or 3 seconds, I hear a beep and the 
> > > > > > window all of a sudden appears under my pointer ready for me to place it 
> > > > > > anywhere.
> > > > > > 
> > > > > > If I use my hotkey to move it, I don't get the delay.
> > > > > > 
> > > > > > Is this a bug or maybe a compile option I missed?
> > > > > 
> > > > > gkrellm and fvwm are competing for a grab on the pointer.  The
> > > > > beep indicates that fvwm has finally given up.  Make sure you don
> > > > > not have any mouse bindings like
> > > > > 
> > > > >   Mouse 1 w ... Function ...
> > > > > 
> > > > > in your fvwm config.
> > > > 
> > > > n.b. This won't matter--Not only is a complex function needed to
> > > > properly move a window by clicking and dragging, but
> > > > EWMHActivateWindowFunc is called when you try to start dragging the
> > > > gkrellm window.
> > > 
> > > That sounds somewhat familiar.  I think you are right.  So the
> > > problem can be circumvented by either destroying the corresponding
> > > fvwm function:
> > > 
> > >   DestroyFunc EWMHActivateWindowFunc
> > > 
> > > (which affects all windows, not just gkrellm).  Unfortunately, it
> > > is not yet possible to disable certain features of the EWMH spec
> > > for individual windows.
> > 
> > Destroying this function would be undesirable for modern applications
> > asking the window manager to activate (bring to the user attention) their
> > windows.
> > 
> > A better solution would be to introduce Grab command and add "Grab off"
> > to EWMHActivateWindowFunc.
> 
> Deja Vu?  No, it wouldn't.
>

I switch to the workers list.

Really I do not see why we should do nothing about this.  There is no
reason for forcing grabbing for _all_ functions. I already describe
others natural bugs caused by this. I already proposed various
solutions.

Now, attached to this message one of these solutions. It is _not_ the
best solution, however it is a good solution for testing: when the
function is totally immediate fvwm try to grab _fast_ and if grabbing
fail the function is executed anyway.

I use this patch for a long time now without problems (note that I
_need_ such a behavior for my fvwm config).  I will be very interested
to see which type of problems this patch can cause.

Again, I do not see why we should not add a way to force this 
behavior, e.g., with a dummy command
    "grab_fast_and_execute_anyway_if_fail"
as it is totally clear that some functions do not need to grab
the pointer!

Regards, Olivier
Index: fvwm/functions.c
===================================================================
RCS file: /home/cvs/fvwm/fvwm/fvwm/functions.c,v
retrieving revision 1.260
diff -u -r1.260 functions.c
--- fvwm/functions.c	5 Jul 2003 01:37:47 -0000	1.260
+++ fvwm/functions.c	7 Nov 2003 04:38:41 -0000
@@ -79,6 +79,7 @@
 	FunctionItem *last_item;         /* last item in function */
 	char *name;                      /* function name */
 	unsigned int use_depth;
+	Bool already_grab_warn;
 } FvwmFunction;
 
 /* Types of events for the FUNCTION builtin */
@@ -120,7 +121,7 @@
 }
 
 /*
- * Defer the execution of a function to the next button press if the context is
+ * Defer the execution of a function to the next button press if the context
  * C_ROOT
  *
  *  Inputs:
@@ -153,6 +154,11 @@
 	if (wcontext != C_ROOT && wcontext != C_NO_CONTEXT && fw != NULL &&
 	    wcontext != C_EWMH_DESKTOP)
 	{
+		if (Scr.flags.is_executing_complex_function &&
+		    trigger_evtype == ButtonPress)
+		{
+			trigger_evtype = ButtonRelease;
+		}
 		if (FinishEvent == ButtonPress ||
 		    (FinishEvent == ButtonRelease &&
 		     trigger_evtype != ButtonPress))
@@ -875,6 +881,8 @@
 	exec_context_change_mask_t mask;
 	int trigger_evtype;
 	XEvent *te;
+	Bool need_ungrab = False;
+	Bool grab_fast = True;
 
 	if (cond_rc == NULL)
 	{
@@ -960,6 +968,17 @@
 			{
 				do_allow_unmanaged_immediate &= fi->flags;
 				ImmediateNeedsTarget = True;
+				if (!grab_fast)
+				{
+					break;
+				}
+			}
+		}
+		if (fi->condition != CF_IMMEDIATE)
+		{
+			grab_fast = False;
+			if (ImmediateNeedsTarget)
+			{
 				break;
 			}
 		}
@@ -986,12 +1005,46 @@
 	/* we have to grab buttons before executing immediate actions because
 	 * these actions can move the window away from the pointer so that a
 	 * button release would go to the application below. */
-	if (!GrabEm(CRS_NONE, GRAB_NORMAL))
+	if (grab_fast)
 	{
-		func->use_depth--;
-		XBell(dpy, 0);
-		__cf_cleanup(&depth, arguments, cond_rc);
-		return;
+		if (!GrabEmWithAttempts(CRS_NONE, GRAB_NORMAL, 1))
+		{
+			/* execute the function anyway */
+			if (1 || !func->already_grab_warn)
+			{
+				func->already_grab_warn = True;
+				fvwm_msg(
+					WARN, "ComplexFunction",
+					"Cannot grab the cursor when "
+					"executing %s", action);
+			}
+		}
+		else
+		{
+			need_ungrab = True;
+		}
+	}
+	else
+	{
+		if (!GrabEm(CRS_NONE, GRAB_NORMAL))
+		{
+			if (1 || !func->already_grab_warn)
+			{
+				func->already_grab_warn = True;
+				fvwm_msg(
+					WARN, "ComplexFunction",
+					"Cannot grab the cursor, abort "
+					"%s execution", action);
+			}
+			func->use_depth--;
+			XBell(dpy, 0);
+			__cf_cleanup(&depth, arguments, cond_rc);
+			return;
+		}
+		else
+		{
+			need_ungrab = True;
+		}
 	}
 	exc2 = exc_clone_context(exc, &ecc, mask);
 	__run_complex_function_items(
@@ -1025,7 +1078,10 @@
 	{
 		func->use_depth--;
 		__cf_cleanup(&depth, arguments, cond_rc);
-		UngrabEm(GRAB_NORMAL);
+		if (need_ungrab)
+		{
+			UngrabEm(GRAB_NORMAL);
+		}
 		return;
 	}
 
@@ -1039,7 +1095,10 @@
 		{
 			func->use_depth--;
 			__cf_cleanup(&depth, arguments, cond_rc);
-			UngrabEm(GRAB_NORMAL);
+			if (need_ungrab)
+			{
+				UngrabEm(GRAB_NORMAL);
+			}
 			return;
 		}
 	}
@@ -1110,7 +1169,10 @@
 	 * function will receive MotionNotify events with a button held down!
 	 * The results are unpredictable.  E.g. rxvt interprets the
 	 * ButtonMotion as user input to select text. */
-	UngrabEm(GRAB_NORMAL);
+	if (need_ungrab)
+	{
+		UngrabEm(GRAB_NORMAL);
+	}
 #endif
 	fev_set_evpos(&d, x, y);
 	fev_fake_event(&d);
@@ -1125,8 +1187,10 @@
 	 */
 	func->use_depth--;
 	__cf_cleanup(&depth, arguments, cond_rc);
-	UngrabEm(GRAB_NORMAL);
-
+	if (need_ungrab)
+	{
+		UngrabEm(GRAB_NORMAL);
+	}
 	return;
 }
 
@@ -1143,6 +1207,7 @@
 	tmp->last_item = NULL;
 	tmp->name = stripcpy(name);
 	tmp->use_depth = 0;
+	tmp->already_grab_warn = False;
 	Scr.functions = tmp;
 
 	return tmp;
@@ -1277,7 +1342,7 @@
 	return;
 }
 
-void find_func_t(char *action, short *func_t, unsigned char *flags)
+void find_func_t(char *action, short *func_t, FUNC_FLAGS_TYPE *flags)
 {
 	int j, len = 0;
 	char *endtok = action;
Index: fvwm/functions.h
===================================================================
RCS file: /home/cvs/fvwm/fvwm/fvwm/functions.h,v
retrieving revision 1.74
diff -u -r1.74 functions.h
--- fvwm/functions.h	29 Jun 2003 19:53:23 -0000	1.74
+++ fvwm/functions.h	7 Nov 2003 04:38:41 -0000
@@ -46,7 +46,7 @@
 /* ---------------------------- interface functions ------------------------ */
 
 void find_func_t(
-	char *action, short *func_t, unsigned char *flags);
+	char *action, short *func_t, FUNC_FLAGS_TYPE *flags);
 Bool functions_is_complex_function(
 	const char *function_name);
 void execute_function(
@@ -58,6 +58,5 @@
 void execute_function_override_window(
 	cond_rc_t *cond_rc, const exec_context_t *exc, char *action,
 	FUNC_FLAGS_TYPE exec_flags, FvwmWindow *fw);
-
 #endif /* FUNCTIONS_H */
Index: fvwm/misc.c
===================================================================
RCS file: /home/cvs/fvwm/fvwm/fvwm/misc.c,v
retrieving revision 1.145
diff -u -r1.145 misc.c
--- fvwm/misc.c	25 Aug 2003 06:47:08 -0000	1.145
+++ fvwm/misc.c	7 Nov 2003 04:39:05 -0000
@@ -80,17 +80,6 @@
 	return;
 }
 
-/* ---------------------------- interface functions ------------------------ */
-
-int GetTwoArguments(
-	char *action, int *val1, int *val2, int *val1_unit, int *val2_unit)
-{
-	*val1_unit = Scr.MyDisplayWidth;
-	*val2_unit = Scr.MyDisplayHeight;
-
-	return GetTwoPercentArguments(action, val1, val2, val1_unit, val2_unit);
-}
-
 /*
  * Grab the pointer.
  * grab_context: GRAB_NORMAL, GRAB_BUSY, GRAB_MENU, GRAB_BUSYMENU,
@@ -102,7 +91,7 @@
  */
 #define DEBUG_GRAB 0
 #if DEBUG_GRAB
-void print_grab_stats(char *text)
+static void print_grab_stats(char *text)
 {
 	int i;
 
@@ -117,7 +106,7 @@
 }
 #endif
 
-Bool GrabEm(int cursor, int grab_context)
+Bool __GrabEm(int cursor, int grab_context, int attempt)
 {
 	int i = 0;
 	int val = 0;
@@ -180,18 +169,18 @@
 			grab_win = Scr.Root;
 		}
 		/* retry to grab the busy cursor only once */
-		rep = 2;
+		rep = (attempt)? attempt:2;
 		break;
 	case GRAB_PASSIVE:
 		/* cannot happen */
 		return False;
 	case GRAB_FREEZE_CURSOR:
 		grab_win = (PressedW != None) ? PressedW : Scr.NoFocusWin;
-		rep = 2;
+		rep = (attempt)? attempt:2;
 		break;
 	default:
 		grab_win = Scr.Root;
-		rep = NUMBER_OF_GRAB_ATTEMPTS;
+		rep = (attempt)? attempt:NUMBER_OF_GRAB_ATTEMPTS;
 		break;
 	}
 
@@ -218,11 +207,12 @@
 		default:
 			/* If you go too fast, other windows may not get a
 			 * chance to release any grab that they have. */
-			i++;
 			if (grab_context == GRAB_FREEZE_CURSOR)
 			{
+				i++;
 				break;
 			}
+			i++;
 			if (i < rep)
 			{
 				usleep(1000 * TIME_BETWEEN_GRAB_ATTEMPTS);
@@ -246,6 +236,28 @@
 	return True;
 }
 
+/* ---------------------------- interface functions ------------------------ */
+
+int GetTwoArguments(
+	char *action, int *val1, int *val2, int *val1_unit, int *val2_unit)
+{
+	*val1_unit = Scr.MyDisplayWidth;
+	*val2_unit = Scr.MyDisplayHeight;
+
+	return GetTwoPercentArguments(action, val1, val2, val1_unit, val2_unit);
+}
+
+Bool GrabEm(int cursor, int grab_context)
+{
+
+	return __GrabEm(cursor, grab_context, 0);
+
+}
+
+Bool GrabEmWithAttempts(int cursor, int grab_context, int attempts)
+{
+	return __GrabEm(cursor, grab_context, attempts);
+}
 
 /*
  *
Index: fvwm/misc.h
===================================================================
RCS file: /home/cvs/fvwm/fvwm/fvwm/misc.h,v
retrieving revision 1.129
diff -u -r1.129 misc.h
--- fvwm/misc.h	29 Jun 2003 19:53:23 -0000	1.129
+++ fvwm/misc.h	7 Nov 2003 04:39:05 -0000
@@ -61,7 +61,9 @@
 
 Bool GrabEm(
 	int cursor, int grab_context);
+Bool GrabEmWithAttempts(
+	int cursor, int grab_context, int attempts);
 Bool UngrabEm(
 	int ungrab_context);
 int GetTwoArguments(

---

From: Mikhael Goikhman <[email-masked]>
Date: Fri, 7 Nov 2003 15:43:20 +0000

On 07 Nov 2003 16:02:49 +0100, Dominik Vogt wrote:
> 
> On Fri, Nov 07, 2003 at 02:45:30PM +0000, Mikhael Goikhman wrote:
> > On 07 Nov 2003 14:36:55 +0100, Dominik Vogt wrote:
> > > 
> > > > A better solution would be to introduce Grab command and add
> > > > "Grab off" to EWMHActivateWindowFunc.
> > > 
> > I.e. you say fvwm should know better than the user what to do and
> > disallow some legitimate functionality. I hope you have a better solution
> > in mind for this kind of problems regarding fvwm functions.
> 
> Like it or not, fvwm functions don't work reliably without the
> grab.  Furthermore, they can not work reliably *because of the way
> X11 is designed*.  Theoretically, one could improve predictability
> of whether a particular function needs the grab or not, but
> practically the code is so obscure that it it virtually impossible
> to do so (partially since it depends on the behaviour of third
> party software).
> 
> In the specific case the "Focus" command in EWMHActivateWindowFunc
> is one of those that requires the grab to prevent a race condition
> between the pointer being moved while the focus is changed.  If
> the grab is not held in such a context, releasing the mouse button
> leads to more or less random windows being focused (experience
> from the past, not something I'm making up).
> 
> The situation can be improved by using the Schedule command to
> trigger EWMHActivateWindowFunc or UrgencyFunc (plus any others?)
> as soon as possible if the grab can not be established
> immediately.  THe code handling scheduled functions should have
> a similar check too and keep the functions due for execution in
> the queue until a grab can be established.  However, it would be
> necessary to always make the grab in the scheduler code since it's
> impossible to predict whether the grab is necessary or not.

Sorry to require you to post the same explanation again.

But my comment was intended to say: Why can't the user explicitelly ask
fvwm to be less reliable (because in fact it will be more relable or more
functional this way, just throwing away all function commands may be more
unreliable in some situations).

It is not critical if another window under the pointer is focused,
although I don't see how this may happen. I would like you to explain me
how a random window may be focused and not the original intended window
_or_ if SloppyFocus is used the window under the cursor, i.e. again the
intended window. But even if this happens it may be ok for the user.

The worst thing I may see is the function has both Focus and Next
commands, then the Next theoretically is not guaranteed to work without
grab (practically I think it will always work, since fvwm executes only
one flow of commands at one time). Still the user knows there is no Next
in his function and he may want to mark the function as grab-less.

Regards,
Mikhael.
--
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, 7 Nov 2003 17:14:18 +0100

> to see which type of problems this patch can cause.

Focus corruption in scripts.

> as it is totally clear that some functions do not need to grab

Because the side effects are totally unpredictable.

Ciao

Dominik ^_^  ^_^

---

From: Olivier Chapuis <[email-masked]>
Date: Fri, 7 Nov 2003 23:50:11 +0100

> Focus corruption in scripts.

So, apply the attached patch and explain me how I can reproduce a
problem. We have a list of 100% reproducible real life problems
with the current code.
 
> Because the side effects are totally unpredictable.

????

So you say that all fvwm functions need to grab the pointer??
 
By default function execution will not change. We (I) need
to execute certain (totally immediate) function which should not
be aborted because pointer grab fail. What we suggest is something
like:

AddToFunc Foo
+ I NoGrabFailAbort
+ I ...
...

I see no reasons to do not add such a feature as it allows to fixes
various bugs.

Regards, Olivier

---

From: Dominik Vogt <[email-masked]>
Date: Wed, 12 Nov 2003 15:56:33 +0100

> problem. We have a list of 100% reproducible real life problems

  AddToFunc func_may_need_pointer_grab
  + I Next (foo) Move $0
  + I All (currentpage, foo) destroy

This function demonstrates a class of functions that always need
to grab the pointer.  Without the grab, CMD_Move is called without
the grab.  Depending on the argument $0 it may or may not enter
__move_loop and establish a grab on the pointer with GRAB_NORMAL
if that fails, CMD_Move is abandoned but function execution
continues.  The next line may now destroy the window refered to by
"Next (foo)" because it was not moved.

With the pointer not being grabbed, any single command in a
function may fail at random and is then skipped with potentially
catastrophic effects (just consider what damage PipeRead may
cause if called improperly).

For this function, it is impossible to predict whether a grab is
needed or not:

  AddToFunc foo
  + I PipeRead some_script

Ciao

Dominik ^_^  ^_^

---

From: Mikhael Goikhman <[email-masked]>
Date: Wed, 12 Nov 2003 16:56:24 +0000

> "Next (foo)" because it was not moved.

Just to see whether I understand this example.
For this example to show any problem, the following should happen:

  * some app other than fvwm grabs the pointer when the function is
    invoked (pretty unusual situation, I didn't see it often)
  * $0 should be empty, so interactive Move is invoked, since
    non-interactive Move does not requere grab
  * A user intends to move the foo window interactivelly outside
    of the current page (how is it possible?), but he misses this chance

Then I think this example is not very good, because AFAIU
All (currentpage, foo) is almost guaranteed to match foo after
interactive Move.

This shows how difficult (if possible) to create an example that produces
non-requested results. The user requested All destroy and he got it.

I would say the user can't rely on Move to always work, it may fail just
because the parameter is incorrect, or a user canceled the Move using
Escape, or a grab was not acquired. The user may write his function
correctly if he wants to handle all situations, i.e. add Next (foo)
checks to see whether Move worked or not.

For me, it would be better if only one Move fails rather then the entire
function. Although I understand there are potential bad cases. It is just
that we throw away much more good cases than the bad ones.

It would be nice to see a more real life example that would be affected.
Just for my education.

> For this function, it is impossible to predict whether a grab is

If someone types "PipeRead some_script" command in FvwmConsole or sends
it from another module, then fvwm does not add additional grab, right?
I see no problem if there is a way to ask "foo" to do the same.

Anyway, both of these functions will continue to work just like they do
now with this patch applied, so it still does not explain why the patch
should not be accepted.

> > I see no reasons to do not add such a feature as it allows to fixes

I think noone argues that most of the user functions do not require grab.
A typical fvwm-themes configuration has more than 200 functions.

  % fvwm-themes-config -e | grep ^AddToFunc | wc -l
  235

I am not sure about maybe 5 functions, whether they require grab or not
(probably not), but all other 230 functions clearly do not need it. There
are some non-trivial functions with almost all existing fvwm commands
participating overall, still I know for sure they do not require grab.

I see no reason not to allow the user to request ContinueOnGrabFailure.

Just, to avoid additional unrelevant discussion, I mentioned fvwm-themes
as a non-trivial configuration only, I don't want to add
ContinueOnGrabFailure to these 230 functions, although this would
probably be a good idea. The intention is to allow users to add it only
to functions that would be terminated by fvwm completely otherwise.

Regards,
Mikhael.

---

From: Dominik Vogt <[email-masked]>
Date: Wed, 12 Nov 2003 18:30:37 +0100

>     of the current page (how is it possible?), but he misses this chance

Sure.  I knew you would start to argue about whether what this
example does is usefull or not.

> non-requested results. The user requested All destroy and he got it.

Receiving an UrgencyHint message while the pointer is grabbed is
another example that does not even involve user interaction.

> that we throw away much more good cases than the bad ones.

Disagreed.  There are better approaches to circumvent the
UrgencyHint/Ewmh events/Schedule queue problems.

> It would be nice to see a more real life example that would be affected.

Later.

> I see no problem if there is a way to ask "foo" to do the same.

It's just an example that explains why it is impossible to know
beforehand whether the grab is necessary or not.  In the "Move"
example you could have argued that we could add a bit in the
function table that tells whether a function needs a grab or not.

> now with this patch applied, so it still does not explain why the patch

Hm?  According to the patch, if a function with "I" actions only
fails grabbing the pointer, it is simply started without the grab
which opens the floodgates for the described problems.

> (probably not), but all other 230 functions clearly do not need it.

I don't know about the fvwm-themes functions, but you probably
forget the relation between mouse position and the focus.  The
way the EnterNotify/LeaveNotify handlers work changes drastically
when the pointer is grabbed (i.e. all such events are ignored
while the grab is held).  Without the grab, a random stream of
these events may be read from xlib once the function is done and
cause focus problems or even weird symptoms in combination with
FvwmAuto or FvwmEvent (for example with the autohide setups).
These are mostly random because they depend on what the user does
with the pointer while it is grabbed.

> There

Because in all but the most simple cases, it is extremely hard to
foresee the side effects.

Ciao

Dominik ^_^  ^_^

---

From: Mikhael Goikhman <[email-masked]>
Date: Wed, 12 Nov 2003 19:24:17 +0000

> which opens the floodgates for the described problems.

I spoke about the final patch, not the one that was posted (although
the posted one is probably ok too).

> > > > I see no reasons to do not add such a feature as it allows to fixes

I suggested to call this "NoGrabFailAbort" as "ContinueOnGrabFailure".

Of course a more correct syntax would be:

  SetFuncProperty Foo ContinueOnGrabFailure

but doing this using a pseudo command is more convenient.

> with the pointer while it is grabbed.

I didn't forget about this, thanks for remainding anyway.

Still most of the functions are simple and executed quickly, they are
pretty much immune against EnterNotify/LeaveNotify events. And the ones
that would be potentially affected usually contain only one command
requiring grab, and this individual command already cares about itself to
receive this grab. Its failure does not add any disaster unless the
function author planned this disaster.

> foresee the side effects.

My experience is different. In all but the most unreal cases it is
possible not to grab at all. And we speak about explicit no-grab only.

A function is just a set of commands, if these individual commands are
executed from several bindings or from one module there is no grab, but
still this set of commands is guaranteed to work, because every
individual command is guaranteed to work. It is good we have a kind of
transaction to ensure different commands will work better together.
Now, it would be nice if user is explicitely able to ask to return to the
separate command execution, by this he explicitely asks not to worry
about individual command failure, his function allows this.

Regards,
Mikhael.