PATCH: 3.0.6-pre-4: COLUMNS/LINES environment handling

PATCH: 3.0.6-pre-4: COLUMNS/LINES environment handling

[Bart Schaefer]

This took far longer than it should have, but I think this is right.  If PWS
agrees that it looks OK, I can produce an equivalent patch for 3.1.5-pws-21.
Here's what's going on:

The problem with the before-this-patch adjustwinsize() is mostly that it's
trying to do too much at once.  When the user sets LINES and COLUMNS, they
must be set one at a time, but adjustwinsize() behaves as if both always
change together -- which led to to the `userlines' and `usercols' statics,
trying to keep track of whether the user explicitly set one or the other
on a previous call.  This also makes it problematic to allow a recursive
call to the function, as would happen if setiparam() were used to update
the LINES and COLUMNS parameters on a SIGWINCH.

So after mucking about for far too long trying to keep the changes inside
adjustwinsize(), I finally concluded that the only right thing was to rip
management of LINES and COLUMNS out into their own functions that could be
called independently from adjustwinsize().  Hence were born adjustlines()
and adjustcolumns(), which encapsulate synchnronizing LINES and COLUMNS
with the tty driver window size structure, and return true if the values
changed or false if they remained unchanged.

Now adjustwinsize() calls both of the other adjust*() functions when its
parameter is 0 or 1, and only one of them when 2 or 3.  Thus setting the
value of LINES can't poke a new columns value into the tty driver, and
vice versa.  Happily, this also makes recursive calls to adjustwinsize()
into no-ops, so it's possible to call setiparam() to export any changes in
the 0 or 1 case.  This is done only if zgetenv() finds the parameter name
in the environment, so it can't accidentaly reverse an "unset LINES" (or
COLUMNS).

As an optimization, I used a static local to avoid doing the ioctl() again
on a recursive call; the behavior is unchanged without the static, but see
the comment in the code regarding possible race conditions.

The behavior should be as discussed in 6562 and its predecessors, to wit:
1. At startup, LINES/COLUMNS are determined by trying in this order:
   - import from the environment;
   - read from the tty driver (TIOCGWINSZ);
   - read from the termcap/terminfo entry;
   - assume 24 lines and 80 columns.
   Note that LINES/COLUMNS are not implicitly exported, so if they were
   not imported from the environment in the first place you must export
   them yourself later.  If you don't like the value imported from the
   environment, see (5) below.
2. On SIGWINCH, the tty driver values are used unless they're zeroes.
   I put in some debugging statements to warn if driver values are bad.
3. After running an external command or subshell, the tty driver values
   are used if they've changed; otherwise see (1).
4. When LINES or COLUMNS is assigned a nonzero value, that value is
   written to the tty driver if possible (TIOCSWINSZ); but only the one
   that changed is written.  E.g. even if the tty driver and the internal
   value of COLUMNS disagree, assigning to LINES will change only the
   tty driver rows, not both the rows and the columns.  (It also won't
   cause the tty driver's columns value to be assigned to COLUMNS!)
5. When LINES or COLUMNS is assigned a zero value, a nonzero value is
   computed as in (1) [skipping the environment] and that becomes the
   actual value of the parameter.  Note that without (4), assigning a
   zero to either of LINES or COLUMNS would have clobbered the tty driver
   value of the other one, making this feature a lot less useful.

Now, somebody who has a copy of the actual POSIX shell spec should tell
us if the above is reasonable.  Zoltan, are you out there?

How much of all that needs to go in the manual?  Lots of it should go in
the FAQ, including remarks about how it is broken in various versions if
we can figure that out.

Another thing I tossed in was forcing a refresh on SIGWINCH even if the
OS appears not to support TIOCGWINSZ.  I don't know whether it's even
possible for that case to come up, but ...

Finally, I noticed that `winchanged' was declared only if TIOCGWINSZ but
used without such a condition.  Apparently zsh doesn't get built on any
systems that don't have TIOCGWINSZ?

Index: Src/utils.c
===================================================================
@@ -832,61 +832,131 @@
 extern winchanged;
 #endif
 
-/* check the size of the window and adjust if necessary. *
- * The value of from:					 *
- *   0: called from update_job or setupvals		 *
- *   1: called from the SIGWINCH handler		 *
- *   2: the user have just changed LINES manually	 *
- *   3: the user have just changed COLUMNS manually      */
-
-/**/
-void
-adjustwinsize(int from)
+static int
+adjustlines(int signalled)
 {
-    int oldcols = columns, oldrows = lines;
+    int oldlines = lines;
 
 #ifdef TIOCGWINSZ
-    static int userlines, usercols;
-
-    if (SHTTY == -1)
-	return;
-
-    if (from == 2)
-	userlines = lines > 0;
-    if (from == 3)
-	usercols = columns > 0;
-
-    if (!ioctl(SHTTY, TIOCGWINSZ, (char *)&shttyinfo.winsize)) {
-	if (!userlines || from == 1)
-	    lines = shttyinfo.winsize.ws_row;
-	if (!usercols || from == 1)
-	    columns = shttyinfo.winsize.ws_col;
+    if (signalled || lines <= 0)
+	lines = shttyinfo.winsize.ws_row;
+    else
+	shttyinfo.winsize.ws_row = lines;
+#endif /* TIOCGWINSZ */
+    if (lines <= 0) {
+	DPUTS(signalled, "BUG: Impossible TIOCGWINSZ rows");
+	lines = tclines > 0 ? tclines : 24;
     }
-#endif   /* TIOCGWINSZ */
 
-    if (lines <= 0)
-	lines = tclines > 0 ? tclines : 24;
-    if (columns <= 0)
-	columns = tccolumns > 0 ? tccolumns : 80;
     if (lines > 2)
 	termflags &= ~TERM_SHORT;
     else
 	termflags |= TERM_SHORT;
+
+    return (lines != oldlines);
+}
+
+static int
+adjustcolumns(int signalled)
+{
+    int oldcolumns = columns;
+
+#ifdef TIOCGWINSZ
+    if (signalled || columns <= 0)
+	columns = shttyinfo.winsize.ws_col;
+    else
+	shttyinfo.winsize.ws_col = columns;
+#endif /* TIOCGWINSZ */
+    if (columns <= 0) {
+	DPUTS(signalled, "BUG: Impossible TIOCGWINSZ cols");
+	columns = tccolumns > 0 ? tccolumns : 80;
+    }
+
     if (columns > 2)
 	termflags &= ~TERM_NARROW;
     else
 	termflags |= TERM_NARROW;
 
+    return (columns != oldcolumns);
+}
+
+/* check the size of the window and adjust if necessary. *
+ * The value of from:					 *
+ *   0: called from update_job or setupvals		 *
+ *   1: called from the SIGWINCH handler		 *
+ *   2: called from the LINES parameter callback	 *
+ *   3: called from the COLUMNS parameter callback	 */
+
+/**/
+void
+adjustwinsize(int from)
+{
+    static int getwinsz = 1;
+    int ttyrows = shttyinfo.winsize.ws_row;
+    int ttycols = shttyinfo.winsize.ws_col;
+    int resetzle = 0;
+
+    if (getwinsz || from == 1) {
 #ifdef TIOCGWINSZ
-    if (interact && from >= 2) {
-	shttyinfo.winsize.ws_row = lines;
-	shttyinfo.winsize.ws_col = columns;
+	if (SHTTY == -1)
+	    return;
+	if (ioctl(SHTTY, TIOCGWINSZ, (char *)&shttyinfo.winsize) == 0) {
+	    resetzle = (ttyrows != shttyinfo.winsize.ws_row ||
+			ttycols != shttyinfo.winsize.ws_col);
+	    if (from == 0 && resetzle && ttyrows && ttycols)
+		from = 1; /* Signal missed while a job owned the tty? */
+	    ttyrows = shttyinfo.winsize.ws_row;
+	    ttycols = shttyinfo.winsize.ws_col;
+	} else {
+	    /* Set to unknown on failure */
+	    shttyinfo.winsize.ws_row = 0;
+	    shttyinfo.winsize.ws_col = 0;
+	    resetzle = 1;
+	}
+#else
+	resetzle = from == 1;
+#endif /* TIOCGWINSZ */
+    } /* else
+	 return; */
+
+    switch (from) {
+    case 0:
+    case 1:
+	getwinsz = 0;
+	/* Calling setiparam() here calls this function recursively, but  *
+	 * because we've already called adjustlines() and adjustcolumns() *
+	 * here, recursive calls are no-ops unless a signal intervenes.   *
+	 * The commented "else return;" above might be a safe shortcut,   *
+	 * but I'm concerned about what happens on race conditions; e.g., *
+	 * suppose the user resizes his xterm during `eval $(resize)'?    */
+	if (adjustlines(from) && zgetenv("LINES"))
+	    setiparam("LINES", lines);
+	if (adjustcolumns(from) && zgetenv("COLUMNS"))
+	    setiparam("COLUMNS", columns);
+	getwinsz = 1;
+	break;
+    case 2:
+	resetzle = adjustlines(0);
+	break;
+    case 3:
+	resetzle = adjustcolumns(0);
+	break;
+    }
+
+#ifdef TIOCGWINSZ
+    if (interact && from >= 2 &&
+	(shttyinfo.winsize.ws_row != ttyrows ||
+	 shttyinfo.winsize.ws_col != ttycols)) {
+	/* shttyinfo.winsize is already set up correctly */
 	ioctl(SHTTY, TIOCSWINSZ, (char *)&shttyinfo.winsize);
     }
-#endif
+#endif /* TIOCGWINSZ */
 
-    if (zleactive && (from >= 2 || oldcols != columns || oldrows != lines)) {
-	resetneeded = winchanged = 1;
+    if (zleactive && resetzle) {
+#ifdef TIOCGWINSZ
+	winchanged =
+#endif /* TIOCGWINSZ */
+	    resetneeded = 1;
 	refresh();
     }
 }


-- 
Bart Schaefer                                 Brass Lantern Enterprises
http://www.well.com/user/barts              http://www.brasslantern.com

Re: PATCH: 3.0.6-pre-4: COLUMNS/LINES environment handling

[Tatsuo Furukawa]

Hello.

Bart> This took far longer than it should have, but I think this is right.

This is what I really want.  I tested this patch, and NO problem! 

----------------

I}     4. If above is void, use 80*25.
Bart> I'm sticking with 80x24, ....

This is my misunderstanding...  "80x24" is the best value.


I} "After some command",
I} 
I}     1. If COLUMNS/LINES are different from internal columns/lines
I}        value, COLUMNS/LINES are used.

Bert> No, that can't be right.

Sorry, I was wrong.

------------------

I hope this will be merged into new zsh.

-- 
Tatsuo Furukawa (frkwtto@osk3.3web.ne.jp)

Re: PATCH: 3.0.6-pre-4: COLUMNS/LINES environment handling

[Peter Stephenson]

"Bart Schaefer" wrote:
> This took far longer than it should have, but I think this is right.  If PWS
> agrees that it looks OK, I can produce an equivalent patch for 3.1.5-pws-21.

It applied with 1 fuzz after changing refresh to zrefresh.

I am prepared to collect any suggestions of what to put in the FAQ.

-- 
Peter Stephenson <pws@ibmth.df.unipi.it>       Tel: +39 050 844536
WWW:  http://www.ifh.de/~pws/
Dipartimento di Fisica, Via Buonarroti 2, 56127 Pisa, Italy

RE: PATCH: 3.0.6-pre-4: COLUMNS/LINES environment handling

[Andrej Borsenkow]

>
> Now, somebody who has a copy of the actual POSIX shell spec should tell
> us if the above is reasonable.  Zoltan, are you out there?
>

This is available online: http://www.opengroup.org/onlinepubs/7908799/toc.htm
(Single Unix V2)

 Other Environment Variables
COLUMNS
A decimal integer > 0 used to indicate the user's preferred width in column
positions for the terminal screen or window. (See column position .) If this
variable is unset or null, the implementation determines the number of columns,
appropriate for the terminal or window, in an unspecified manner. When COLUMNS
is set, any terminal-width information implied by TERM will be overridden. Users
and portable applications should not set COLUMNS unless they wish to override
the system selection and produce output unrelated to the terminal
characteristics. The default value for the number of column positions is
unspecified because historical implementations use different methods to
determine values corresponding to the size of the screen in which the utility is
run. This size is typically known to the implementation through the value of
TERM or by more elaborate methods such as extensions to the stty utility, or
knowledge of how the user is dynamically resizing windows on a bit-mapped
display terminal. Users should not need to set this variable in the environment
unless there is a specific reason to override the implementation's default
behaviour, such as to display data in an area arbitrarily smaller than the
terminal or window.

Note, that neither stty nor "General Terminal Inteface" description say any word
about the whole stuff :-)

In any case, I think the proposed order is correct and patch for 3.1.5 is
welcome.

/andrej

> How much of all that needs to go in the manual?  Lots of it should go in
> the FAQ, including remarks about how it is broken in various versions if
> we can figure that out.
>

We definitely have to describe, how COLUMNS/LINES are used/set by shell.

/andrej

Re: PATCH: 3.0.6-pre-4: COLUMNS/LINES environment handling

[Peter Stephenson]

"Bart Schaefer" wrote:

> Now, somebody who has a copy of the actual POSIX shell spec should tell
> us if the above is reasonable.  Zoltan, are you out there?

I had a look at the single UNIX spec (see http://www.opengroup.org/) and it
doesn't say anything about this, at least in the Shell chapter (2).

> How much of all that needs to go in the manual?  Lots of it should go in
> the FAQ, including remarks about how it is broken in various versions if
> we can figure that out.

As it now behaves logically, the manual entry can be shortish, but it might
be nice to say a bit more about LINES and COLUMNS, e.g. that they're set
automatically if possible but may be wrong.

As for the FAQ entry, maybe inspiration will strike, but these things are
unpredictable, n'est-ce pas?

> Finally, I noticed that `winchanged' was declared only if TIOCGWINSZ but
> used without such a condition.  Apparently zsh doesn't get built on any
> systems that don't have TIOCGWINSZ?

Presumably it hasn't been (because it can't be), but this must be wrong.


This is a non-sequitur, but while I was looking at the UNIX spec I noticed
that LINENO, unlike LINES, is specified, and it says that functions as well
as scripts should have it set.  You'll find out that with autoloaded
functions you always get 0 from `print $LINENO' (the question of why is not
entirely trivial).  I've known about this for ages, but the ways I thought
about fixing it at the time were rather gross.  But maybe someone's little
grey cells...

-- 
Peter Stephenson <pws@ibmth.df.unipi.it>       Tel: +39 050 844536
WWW:  http://www.ifh.de/~pws/
Dipartimento di Fisica, Via Buonarroti 2, 56127 Pisa, Italy

RE: PATCH: 3.0.6-pre-4: COLUMNS/LINES environment handling

[Andrej Borsenkow]

>
> How much of all that needs to go in the manual?  Lots of it should go in
> the FAQ, including remarks about how it is broken in various versions if
> we can figure that out.
>

Currently both LINES and COLUMNS are in section "Parameters used by shell".
After this patch this is no more true (and presumably never was). So, at least,
they should be moved to the section "Parameters set by shell" with explanation,
when they are set.

/andrej

Re: PATCH: 3.0.6-pre-4: COLUMNS/LINES environment handling

[Bart Schaefer]

On Jun 17,  3:39pm, Andrej Borsenkow wrote:
} Subject: RE: PATCH: 3.0.6-pre-4: COLUMNS/LINES environment handling
}
} > How much of all that needs to go in the manual? Lots of it should
} > go in the FAQ, including remarks about how it is broken in various
} > versions if we can figure that out.
} 
} Currently both LINES and COLUMNS are in section "Parameters used by
} shell". After this patch this is no more true (and presumably never
} was). So, at least, they should be moved to the section "Parameters
} set by shell" with explanation, when they are set.

They most certainly are used by the shell!  LINES is now used by the code
that displays `select name in ...' listings; LINES also determines whether
SINGLE_LINE_ZLE is asserted; COLUMNS determines placement of RPS1; and
both obviously affect the way multiline zle is used.

If you override LINES and COLUMNS and never cause a SIGWINCH to be sent to
the shell, they'll never change, even if they don't actually match the
size of your terminal, and zsh will behave as if the terminal is whatever
size you claimed.  This can be a bit strange if you try to make it use
a bigger window than you really have, but you get what you ask for.

-- 
Bart Schaefer                                 Brass Lantern Enterprises
http://www.well.com/user/barts              http://www.brasslantern.com

Re: PATCH: 3.0.6-pre-4: COLUMNS/LINES environment handling

[Geoff Wing]

Bart Schaefer <schaefer@candle.brasslantern.com> typed:
:On Jun 17,  3:39pm, Andrej Borsenkow wrote:
:They most certainly are used by the shell!  LINES is now used by the code
:that displays `select name in ...' listings; LINES also determines whether
:SINGLE_LINE_ZLE is asserted; COLUMNS determines placement of RPS1; and
:both obviously affect the way multiline zle is used.

I've got some recollection of making zsh width restricted by COLUMNS.
i.e. you can have an 80 column window and say COLUMNS=20.  And back then
everything would have been fine (on every system but one Irix one, I think)
as I did some magic automargin thingy, which displayed the correct stuff
on both automargin and non-automargin terms.  Except, xterm's couldn't 
do proper cut'n'paste using that trick so that got taken out.  Nowadays
if you do COLUMNS=20 on an 80 column window and your 80 column window
has automargin, then you'll lose out.
Anyway, just injecting a bit of programming history into this.

Regards,
-- 
Geoff Wing : <gcw@pobox.com>     Work URL: http://www.primenet.com.au/
Rxvt Stuff : <gcw@rxvt.org>      Ego URL : http://pobox.com/~gcw/
Zsh Stuff  : <gcw@zsh.org>       Phone   : (Australia) 0413 431 874