Subject: Consistency of toggling functions

From: Matthias Clasen <[email-masked]>
Date: Tue, 12 Jan 1999 21:18:35 +0100

While playing with my window-group stuff, I wanted to have
a non-toggling Stick function. This made me think about
toggling functions in fvwm in general. I came up with the following
list:

Iconify [value]
Maximize [h v]
RaiseLower 
Stick
WindowShade [opt]

Among these five, two (Iconify, WindowShade) offer non-toggling variants 
through optional arguments, but they use different values to fix 
directions (Iconify: > 0 / < 0; WindowShade: = 1 / = 2), 
and one has directional variants (RaiseLower: Raise, Lower).
The remaining two do not offer non-toggling variants.

The following patch attempts to improve the consistency of this 
by adding an optional parameter to Maximize and Stick and change
the semantics of the WindowShade parameter to be like the others. 

For Maximize, this means that it can now be called with 0, 1, 2 or 3
parameters. If the number of parameters is odd, the first one is the
toggling parameter.

For WindowShade, this patch is an incompatible change, but I think
the improved consistency is worth it. 

Matthias

--- builtins.c2	Tue Jan 12 20:37:42 1999
+++ builtins.c	Tue Jan 12 20:32:42 1999
@@ -330,6 +330,8 @@
 {
   int new_width, new_height,new_x,new_y, page_x, page_y;
   int val1, val2, val1_unit,val2_unit,n;
+  int args, val = 0;
+  char *junk, *tmp;
 
   if (DeferExecution(eventp,&w,&tmp_win,&context, SELECT,ButtonRelease))
     return;
@@ -346,6 +348,26 @@
     FunctionNotAllowed();
     return;
   }
+
+  /* count arguments */
+  args = 0;
+  tmp = GetNextToken (action, &junk);
+  while (junk != NULL) {
+    args++;
+    tmp = GetNextToken (tmp, &junk);
+  } 
+  
+  if (args & 1) {
+     /* odd number of arguments, first is toggle,
+        read it and get rid of it. */
+     GetIntegerArguments(action, NULL, &val, 1);
+     action = GetNextToken (action, &junk);
+  
+     if (((val > 0) && (tmp_win->flags & MAXIMIZED)) ||
+         ((val < 0) && !(tmp_win->flags & MAXIMIZED))) 
+       return; 
+  }
+
   n = GetTwoArguments(action, &val1, &val2, &val1_unit, &val2_unit);
   if(n != 2)
   {
@@ -404,26 +426,29 @@
  *
  *  WindowShade -- shades or unshades a window ([email-masked])
  *
- *  Args: 1 -- force shade, 2 -- force unshade  No Arg: toggle
+ *  Args: > 0 -- force shade, < 0 -- force unshade  No Arg: toggle
  *
  ***********************************************************************/
 void WindowShade(XEvent *eventp,Window w,FvwmWindow *tmp_win,
 		 unsigned long context, char *action, int *Module)
 {
-    int n = 0;
+    int val = 0;
 
     if (DeferExecution(eventp,&w,&tmp_win,&context, SELECT,ButtonRelease))
 	return;
 
+    GetIntegerArguments(action, NULL, &val, 1);
+
     if (!(tmp_win->flags & TITLE) || (tmp_win->flags & MAXIMIZED)) {
       FunctionNotAllowed();
       return;
     }
-    while (isspace(*action))++action;
-    if (isdigit(*action))
-	sscanf(action,"%d",&n);
 
-    if (((tmp_win->buttons & WSHADE)||(n==2))&&(n!=1))
+    if (((val > 0) && (tmp_win->buttons & WSHADE)) ||
+        ((val < 0) && !(tmp_win->buttons & WSHADE)))
+      return;
+
+    if (tmp_win->buttons & WSHADE)
     {
 	tmp_win->buttons &= ~WSHADE;
 	SetupFrame(tmp_win,
@@ -989,7 +1014,15 @@
 void stick_function(XEvent *eventp,Window w,FvwmWindow *tmp_win,
 		    unsigned long context, char *action, int *Module)
 {
+  int val = 0;
+
   if (DeferExecution(eventp,&w,&tmp_win,&context,SELECT,ButtonRelease))
+    return;
+
+  GetIntegerArguments(action, NULL, &val, 1);
+  
+  if (((val > 0) && (tmp_win->flags & STICKY)) ||
+      ((val < 0) && !(tmp_win->flags & STICKY)))
     return;
 
   if(tmp_win->flags & STICKY)

-- 
Matthias Clasen, 
Tel. 0761/203-5606
Email: [email-masked]
Mathematisches Institut, Albert-Ludwigs-Universitaet Freiburg
--
Visit the official FVWM web page at <URL:http://www.hpc.uh.edu/fvwm/>.
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: Jason L Tibbitts III <[email-masked]>
Date: 12 Jan 1999 14:45:22 -0600

>>>>> "MC" == Matthias Clasen <[email-masked]> writes:

MC> For WindowShade, this patch is an incompatible change, but I think the
MC> improved consistency is worth it.

When did windowshade make it into the source?  If it came in as a 2.1
feature then we should be free to change its behavior up until 2.2 (with
good reason, of course, but I think this is a pretty good reason).

 - J<

---

From: Dan Espen <[email-masked]>
Date: Tue, 12 Jan 1999 15:49:57 -0500

> good reason, of course, but I think this is a pretty good reason).

Window shade first appears in the NEWS file in 2.0.44.

--
Dan Espen
444 Hoes Lane  Room RRC 1C-214           E-mail: [email-masked]
Piscataway, NJ 08854                     Phone: (732) 699-5570
--
Visit the official FVWM web page at <URL:http://www.hpc.uh.edu/fvwm/>.
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, 13 Jan 1999 00:13:01 +0100

> the improved consistency is worth it. 

If we change that we should do it right. When I see a call like
'RaiseLower 27', what am I supposed to think what it does? Or
'WindowShade -1'? If we start working on the syntax we should
do something that is easy to understand, e.g. give all of these
builtins an optional (and unique) first argument:

  Iconify [on | off | toggle]
  Stick [on | off | toggle]
  Maximize [on | off | toggle]
  WindowShade [on | off | toggle]

and if it's really needed (I don't think so):

  RaiseLower [raise | lower | toggle]

This can even be merged with the old syntax.

> +    tmp = GetNextToken (tmp, &junk);

That's a huge memory leak. Every time you call on of these
functions a leak the size of the whole command line will
occur. You have to use 'free(junk);'.

> +         ((val < 0) && !(tmp_win->flags & MAXIMIZED))) 

I don't think parsing should be done this way. First we allocate
memory for every token on the line, then throw it away and begin
parsing the whole line again. With explicit non-integer arguments
you can easily avoid this.

> +      ((val < 0) && !(tmp_win->flags & STICKY)))

This isn't any easier to implement than comparing to strings.
Have a look at the examples in builtins.c that use
GetTokenIndex or preferrably GetNextTokenIndex. Costs only
a few bytes (for the strings) but is much easier to
understand.

Bye

Dominik

--
Dominik Vogt, [email-masked]
Reply-To: [email-masked]

---

From: Matthias Clasen <[email-masked]>
Date: Thu, 14 Jan 1999 01:15:06 +0100

>   WindowShade [on | off | toggle]

If you really want to avoid the numeric argument, I'd say that
simply splitting these functions into three like
 
   Iconify, DeIconify, ToggleIconify 

would be more natural. Anyway, I haven't done anything except
fix the stupid memory leak in my first version. Also, the argument
parsing code in Maximize now is a bit less wasteful. 

Matthias

PS Sorry, I don't know how to instruct emacs rmail-mode to
produce a proper attachment, thus I'm still including the
patch below. If this continues to give you problems, I can
uuencode them

--- builtins.c2	Tue Jan 12 20:37:42 1999
+++ builtins.c	Thu Jan 14 00:24:34 1999
@@ -330,6 +330,8 @@
 {
   int new_width, new_height,new_x,new_y, page_x, page_y;
   int val1, val2, val1_unit,val2_unit,n;
+  int val;
+  char *tmp;
 
   if (DeferExecution(eventp,&w,&tmp_win,&context, SELECT,ButtonRelease))
     return;
@@ -346,7 +348,37 @@
     FunctionNotAllowed();
     return;
   }
-  n = GetTwoArguments(action, &val1, &val2, &val1_unit, &val2_unit);
+
+  /* The following code parses the argument list which may be
+     nothing
+     val
+     val1 val2
+     val val1 val2 
+   */
+  val = 0;
+  n = 0;
+  if (GetIntegerArguments(action, &tmp, &val, 1)) {
+    /* first arg was an integer, assume it is val */
+    while (tmp && *tmp && isspace(*tmp))
+        tmp++;
+    if (tmp && *tmp != 0) {
+      /* remaining args must be val1 val2 */
+      n = GetTwoArguments(tmp, &val1, &val2, &val1_unit, &val2_unit);
+      if (n != 2) {
+        /* no they aren't. Initial assumption must have been wrong */
+	val = 0;
+	n = GetTwoArguments(action, &val1, &val2, &val1_unit, &val2_unit);
+      }
+    } 
+  } else {
+    /* either no arg or first arg is not an integer */
+    n = GetTwoArguments(action, &val1, &val2, &val1_unit, &val2_unit);
+  }
+   
+  if (((val > 0) && (tmp_win->flags & MAXIMIZED)) ||
+      ((val < 0) && !(tmp_win->flags & MAXIMIZED))) 
+    return; 
+  
   if(n != 2)
   {
     val1 = 100;
@@ -404,26 +436,29 @@
  *
  *  WindowShade -- shades or unshades a window ([email-masked])
  *
- *  Args: 1 -- force shade, 2 -- force unshade  No Arg: toggle
+ *  Args: > 0 -- force shade, < 0 -- force unshade  No Arg: toggle
  *
  ***********************************************************************/
 void WindowShade(XEvent *eventp,Window w,FvwmWindow *tmp_win,
 		 unsigned long context, char *action, int *Module)
 {
-    int n = 0;
+    int val = 0;

 
@@ -989,7 +1024,15 @@

---

From: Dominik Vogt <[email-masked]>
Date: Thu, 14 Jan 1999 01:33:23 +0100

>   WindowShade [on | off | toggle]

Here is my version of this patch (actually I was thinking about
this the day before you posted your patch). All of the four
functions will accept any of these values: yes/no, true/false,
y/n, t/f, on/off and toggle (effectively doing nothing). The
old syntax still works for backwards compatibility.

Bye

Dominik

--
Dominik Vogt, [email-masked]
Reply-To: [email-masked]
--- fvwm/builtins.c.orig	Wed Jan 13 22:40:33 1999
+++ fvwm/builtins.c	Wed Jan 13 23:42:32 1999
@@ -319,6 +319,7 @@
 {
   int new_width, new_height,new_x,new_y;
   int val1, val2, val1_unit,val2_unit,n;
+  int toggle;
 
   if (DeferExecution(eventp,&w,&tmp_win,&context, SELECT,ButtonRelease))
     return;
@@ -335,6 +336,10 @@
     XBell(dpy, 0);
     return;
   }
+  toggle = ParseToggleArgument(action, &action, -1, 0);
+  if (((toggle == 1) && (tmp_win->flags & MAXIMIZED)) ||
+      ((toggle == 0) && !(tmp_win->flags & MAXIMIZED)))
+    return;
   n = GetTwoArguments(action, &val1, &val2, &val1_unit, &val2_unit);
   if(n != 2)
   {
@@ -392,43 +397,55 @@
 void WindowShade(XEvent *eventp,Window w,FvwmWindow *tmp_win,
 		 unsigned long context, char *action, int *Module)
 {
-    int n = 0;
+  int toggle;
 
-    if (DeferExecution(eventp,&w,&tmp_win,&context, SELECT,ButtonRelease))
-	return;
+  if (DeferExecution(eventp,&w,&tmp_win,&context, SELECT,ButtonRelease))
+    return;
 
-    if (!(tmp_win->flags & TITLE) || (tmp_win->flags & MAXIMIZED)) {
-	XBell(dpy, 0);
-	return;
-    }
-    while (isspace(*action))++action;
-    if (isdigit(*action))
-	sscanf(action,"%d",&n);
+  if (!(tmp_win->flags & TITLE) || (tmp_win->flags & MAXIMIZED)) {
+    XBell(dpy, 0);
+    return;
+  }
+  toggle = ParseToggleArgument(action, NULL, -1, 0);
+  if (toggle == -1)
+  {
+    if (GetIntegerArguments(action, NULL, &toggle, 1) > 0)
+      {
+	if (toggle == 1)
+	  toggle = 1;
+	else if (toggle == 2)
+	  toggle = 0;
+	else
+	  toggle = -1;
+      }
+  }
+  if (toggle == -1)
+    toggle = (tmp_win->buttons & WSHADE) ? 0 : 1;
 
-    if (((tmp_win->buttons & WSHADE)||(n==2))&&(n!=1))
-    {
-	tmp_win->buttons &= ~WSHADE;
-	SetupFrame(tmp_win,
-		   tmp_win->frame_x,
-		   tmp_win->frame_y,
-		   tmp_win->orig_wd,
-		   tmp_win->orig_ht,
-		   True);
-        BroadcastPacket(M_DEWINDOWSHADE, 3,
-                        tmp_win->w, tmp_win->frame, (unsigned long)tmp_win);
-    }
-    else
-    {
-	tmp_win->buttons |= WSHADE;
-	SetupFrame(tmp_win,
-		   tmp_win->frame_x,
-		   tmp_win->frame_y,
-		   tmp_win->frame_width,
-		   tmp_win->title_height + tmp_win->boundary_width,
-		   False);
-        BroadcastPacket(M_WINDOWSHADE, 3,
-                        tmp_win->w, tmp_win->frame, (unsigned long)tmp_win);
-    }
+  if ((tmp_win->buttons & WSHADE) && toggle == 0)
+  {
+    tmp_win->buttons &= ~WSHADE;
+    SetupFrame(tmp_win,
+	       tmp_win->frame_x,
+	       tmp_win->frame_y,
+	       tmp_win->orig_wd,
+	       tmp_win->orig_ht,
+	       True);
+    BroadcastPacket(M_DEWINDOWSHADE, 3,
+		    tmp_win->w, tmp_win->frame, (unsigned long)tmp_win);
+  }
+  else if (!(tmp_win->buttons & WSHADE) && toggle == 1)
+  {
+    tmp_win->buttons |= WSHADE;
+    SetupFrame(tmp_win,
+	       tmp_win->frame_x,
+	       tmp_win->frame_y,
+	       tmp_win->frame_width,
+	       tmp_win->title_height + tmp_win->boundary_width,
+	       False);
+    BroadcastPacket(M_WINDOWSHADE, 3,
+		    tmp_win->w, tmp_win->frame, (unsigned long)tmp_win);
+  }
 }
 #endif /* WINDOWSHADE */
 
@@ -684,27 +701,43 @@
 		      unsigned long context,char *action, int *Module)
 
 {
-  int val = 0;
+  int toggle;
 
   if (DeferExecution(eventp,&w,&tmp_win,&context, SELECT, ButtonRelease))
     return;
 
-  GetIntegerArguments(action, NULL, &val, 1);
+  toggle = ParseToggleArgument(action, NULL, -1, 0);
+  if (toggle == -1)
+  {
+    if (GetIntegerArguments(action, NULL, &toggle, 1) > 0)
+      {
+	if (toggle > 0)
+	  toggle = 1;
+	else if (toggle < 0)
+	  toggle = 0;
+	else
+	  toggle = -1;
+      }
+  }
+  if (toggle == -1)
+    toggle = (tmp_win->flags & ICONIFIED) ? 0 : 1;
 
   if (tmp_win->flags & ICONIFIED)
   {
-    if(val <=0)
+    if (toggle == 0)
       DeIconify(tmp_win);
   }
   else
   {
-    if(check_allowed_function2(F_ICONIFY,tmp_win) == 0)
+    if (toggle == 1)
     {
-      XBell(dpy, 0);
-      return;
-    }
-    if(val >=0)
+      if(check_allowed_function2(F_ICONIFY,tmp_win) == 0)
+      {
+	XBell(dpy, 0);
+	return;
+      }
       Iconify(tmp_win,eventp->xbutton.x_root-5,eventp->xbutton.y_root-5);
+    }
   }
 }
 
@@ -937,7 +970,14 @@
 void stick_function(XEvent *eventp,Window w,FvwmWindow *tmp_win,
 		    unsigned long context, char *action, int *Module)
 {
+  int toggle;
+
   if (DeferExecution(eventp,&w,&tmp_win,&context,SELECT,ButtonRelease))
+    return;
+
+  toggle = ParseToggleArgument(action, &action, -1, 0);
+  if ((toggle == 1 && (tmp_win->flags & STICKY)) ||
+      (toggle == 0 && !(tmp_win->flags & STICKY)))
     return;
 
   if(tmp_win->flags & STICKY)
--- libs/fvwmlib.h.orig	Wed Jan 13 23:02:27 1999
+++ libs/fvwmlib.h	Wed Jan 13 23:14:17 1999
@@ -89,6 +89,8 @@
 int GetOnePercentArgument(char *action, int *value, int *unit_io);
 int GetTwoPercentArguments(char *action, int *val1, int *val2, int *val1_unit,
 			   int *val2_unit);
+int ParseToggleArgument(char *action, char **ret_action, int default_ret,
+			char no_toggle);

 /***********************************************************************
--- libs/Parse.c.orig	Wed Jan 13 22:40:39 1999
+++ libs/Parse.c	Wed Jan 13 23:14:12 1999
@@ -367,6 +367,12 @@
   return tmp;
 }
 
+/*
+ *
+ * convenience functions
+ *
+ */
+
 /****************************************************************************
  *
  * Works like GetNextToken, but with the following differences:
@@ -551,7 +557,7 @@

 int GetTwoPercentArguments(char *action, int *val1, int *val2, int *val1_unit,
-		    int *val2_unit)
+			   int *val2_unit)
 {
   char *tok1, *tok2;
   int n = 0;
@@ -577,4 +583,54 @@
   if (tok2)
     free(tok2);
   return n;
+}
+
+/* Parses the next token in action and returns 1 if it is "yes", "true", "y",
+ * "t" or "on", zero if it is "no", "false", "n", "f" or "off" and -1 if it is
+ * "toggle". A pointer to the first character in action behind the token is
+ * returned through ret_action in this case. ret_action may be NULL. If the
+ * token matches none of these strings the default_ret value is returned and
+ * the action itself is passed back in ret_action. If the no_toggle flag is
+ * non-zero, the "toggle" string is handled as no match. */
+int ParseToggleArgument(char *action, char **ret_action, int default_ret,
+			char no_toggle)
+{
+  int index;
+  int rc;
+  char *token;
+  char *next;
+  char *optlist[] = {
+    "toggle",
+    "yes", "no",
+    "true", "false",
+    "on", "off",
+    "t", "f",
+    "y", "n",
+    NULL
+  };
+
+  next = GetNextToken(action, &token);
+  index = GetTokenIndex(token, optlist, 0, NULL);
+  if (index == 0 && no_toggle == 0)
+  {
+    /* toggle requested explicitly */
+    rc = -1;
+  }
+  else if (index == -1 || (index == 0 && no_toggle))
+  {
+    /* nothing selected, use default and don't modify action */
+    rc = default_ret;
+    next = action;
+  }
+  else
+  {
+    /* odd numbers denote True, even numbers denote False */
+    rc = (index & 1);
+  }
+
+  if (ret_action)
+    *ret_action = next;
+  if (token)
+    free(token);
+  return rc;
 }