On 23-04-15 08:12PM, Christian Weisgerber wrote:
> Mark Jamsek:
> 
> > The below diff implements the framework for tog automated tests.
> 
> I have some comments on the tog.c changes:
> 
> > +#define TOG_SCREEN_DUMP		"SCREENDUMP"
> > +#define TOG_SCREEN_DUMP_LEN	(sizeof(TOG_SCREEN_DUMP) - 1)
> 
> Why is this worth separate defines, when you use "KEY_ENTER" etc
> verbatim in tog_read_script_key()?

tbh, I've forgotten why I defined the screendump instruction now. I let
this diff sit for about a month while busy with uni, and before that
I hadn't touched it since the week after g2k22! I had a bunch of
different instructions when I was working on the vi command-modesque
implementation of the test harness and may have just left this around
ever since.

But, yes, it can go.

> > +/*
> > + * Extract visible substring of line y from the curses screen
> > + * and strip trailing whitespace. If vline is set and locale is
> > + * UTF-8, overwrite line[vline] with '|' because the ACS_VLINE
> > + * character is written out as 'x'. Write the line to file f.
> > + */
> 
> What does the locale have to do with it?  If anything, it would
> suggest the use of UTF-8 line-drawing characters.  The line drawing
> characters chosen by ncurses primarily depend on $TERM and the
> termcap/terinfo entry.  For VT100-compatible terminals, you'd expect
> a switch to the Alternate Character Set, and 'x' which is then
> rendered as a vertical line.  I don't think we want to try to predict
> the entire interaction between termcap/terminfo, ncurses, the
> terminal, and for all I know, yes, even the locale.
> 
> Maybe overwrite the character unconditionally?

The locale is only relevant in this case because we only use the ACS_*
characters when in a utf8 locale--otherwise we already draw the | and
- characters so I felt like overwriting with the same character was
redundant.

But I also agree with you insofar as it'll simplify and remove code to
just overwrite the character unconditionally.

> >  	/*
> > +	 * XXX Perhaps we should define "xterm" as the terminal
> > +	 * type for standardised testing instead of using $TERM?
> > +	 */
> 
> Yes, you should use a standardized $TERM.  I mean what happens for
> TERM=dumb?  I'm not sure what a good choice is.  "xterm" feels like
> a moving target.

I was going back and forth on this and ultimately went the other way!
But I'm happy to defer to your better judgement on this; "dumb" seems to
work so maybe run with that?

diff /home/mark/src/got
commit - 0f62ede7fdacafe2923dcbb41ba897be89dd44ba
path + /home/mark/src/got
blob - 9a96a68020526f3cc89eef7dcb2a5da2e100087a
file + tog/tog.c
--- tog/tog.c
+++ tog/tog.c
@@ -622,9 +622,7 @@ static int using_mock_io;
 } tog_io;
 static int using_mock_io;
 
-#define TOG_SCREEN_DUMP		"SCREENDUMP"
-#define TOG_SCREEN_DUMP_LEN	(sizeof(TOG_SCREEN_DUMP) - 1)
-#define TOG_KEY_SCRDUMP		SHRT_MIN
+#define TOG_KEY_SCRDUMP	SHRT_MIN
 
 /*
  * We implement two types of views: parent views and child views.
@@ -1450,9 +1448,9 @@ strip_trailing_ws(char *str, int *n)
 
 /*
  * Extract visible substring of line y from the curses screen
- * and strip trailing whitespace. If vline is set and locale is
- * UTF-8, overwrite line[vline] with '|' because the ACS_VLINE
- * character is written out as 'x'. Write the line to file f.
+ * and strip trailing whitespace. If vline is set, overwrite
+ * line[vline] with '|' because the ACS_VLINE character is
+ * written out as 'x'. Write the line to file f.
  */
 static const struct got_error *
 view_write_line(FILE *f, int y, int vline)
@@ -1471,7 +1469,7 @@ view_write_line(FILE *f, int y, int vline)
 	 */
 	strip_trailing_ws(line, &r);
 
-	if (vline > 0 && got_locale_is_utf8())
+	if (vline > 0)
 		line[vline] = '|';
 
 	w = fprintf(f, "%s\n", line);
@@ -1521,7 +1519,7 @@ screendump(struct tog_view *view)
 			    view->child->begin_y : view->begin_y;
 
 		for (i = 0; i < view->lines; i++) {
-			if (hline && got_locale_is_utf8() && i == hline - 1) {
+			if (hline && i == hline - 1) {
 				int c;
 
 				/* ACS_HLINE writes out as 'q', overwrite it */
@@ -1655,7 +1653,7 @@ tog_read_script_key(FILE *script, int *ch, int *done)
 		*ch = KEY_DOWN;
 	else if (strncasecmp(line, "KEY_UP", 6) == 0)
 		*ch = KEY_UP;
-	else if (strncasecmp(line, TOG_SCREEN_DUMP, TOG_SCREEN_DUMP_LEN) == 0)
+	else if (strncasecmp(line, "SCREENDUMP", 10) == 0)
 		*ch = TOG_KEY_SCRDUMP;
 	else
 		*ch = *line;
@@ -4211,8 +4209,7 @@ init_mock_term(const char *test_script_path)
 		goto done;
 	}
 
-	/* use local TERM so we test in different environments */
-	if (newterm(NULL, tog_io.cout, tog_io.cin) == NULL)
+	if (newterm("dumb", tog_io.cout, tog_io.cin) == NULL)
 		err = got_error_msg(GOT_ERR_IO,
 		    "newterm: failed to initialise curses");
 

-- 
Mark Jamsek <fnc.bsdbox.org|got.bsdbox.org>
GPG: F2FF 13DE 6A06 C471 CA80  E6E2 2930 DC66 86EE CF68