Fix prediction unicode bug. Make all Cell members private.

Fixes #702.

src/frontend/terminaloverlay.cc

@@ -61,7 +61,7 @@ void ConditionalOverlayCell::apply( Framebuffer &fb, uint64_t confirmed_epoch, i
 
   if ( unknown ) {
     if ( flag && ( col != fb.ds.get_width() - 1 ) ) {
-      fb.get_mutable_cell( row, col )->renditions.set_attribute(Renditions::underlined, true);
+      fb.get_mutable_cell( row, col )->get_renditions().set_attribute(Renditions::underlined, true);
     }
     return;
   }
@@ -69,7 +69,7 @@ void ConditionalOverlayCell::apply( Framebuffer &fb, uint64_t confirmed_epoch, i
   if ( *fb.get_cell( row, col ) != replacement ) {
     *(fb.get_mutable_cell( row, col )) = replacement;
     if ( flag ) {
-      fb.get_mutable_cell( row, col )->renditions.set_attribute( Renditions::underlined, true );
+      fb.get_mutable_cell( row, col )->get_renditions().set_attribute( Renditions::underlined, true );
     }
   }
 }
@@ -206,9 +206,9 @@ void NotificationEngine::apply( Framebuffer &fb ) const
 
   /* draw bar across top of screen */
   Cell notification_bar( 0 );
-  notification_bar.renditions.foreground_color = 37;
-  notification_bar.renditions.background_color = 44;
-  notification_bar.contents.push_back( 0x20 );
+  notification_bar.get_renditions().foreground_color = 37;
+  notification_bar.get_renditions().background_color = 44;
+  notification_bar.append( 0x20 );
 
   for ( int i = 0; i < fb.ds.get_width(); i++ ) {
     *(fb.get_mutable_cell( 0, i )) = notification_bar;
@@ -275,12 +275,12 @@ void NotificationEngine::apply( Framebuffer &fb ) const
     case 2: /* wide character */
       this_cell = fb.get_mutable_cell( 0, overlay_col );
       fb.reset_cell( this_cell );
-      this_cell->renditions.set_attribute(Renditions::bold, true);
-      this_cell->renditions.foreground_color = 37;
-      this_cell->renditions.background_color = 44;
+      this_cell->get_renditions().set_attribute(Renditions::bold, true);
+      this_cell->get_renditions().foreground_color = 37;
+      this_cell->get_renditions().background_color = 44;
       
-      this_cell->contents.push_back( ch );
-      this_cell->width = chwidth;
+      this_cell->append( ch );
+      this_cell->set_width( chwidth );
       combining_cell = this_cell;
 
       overlay_col += chwidth;
@@ -290,14 +290,14 @@ void NotificationEngine::apply( Framebuffer &fb ) const
 	break;
       }
 
-      if ( combining_cell->contents.size() == 0 ) {
-	assert( combining_cell->width == 1 );
-	combining_cell->fallback = true;
+      if ( combining_cell->empty() ) {
+	assert( combining_cell->get_width() == 1 );
+	combining_cell->set_fallback( true );
 	overlay_col++;
       }
 
-      if ( combining_cell->contents.size() < 32 ) {
-	combining_cell->contents.push_back( ch );
+      if ( !combining_cell->full() ) {
+	combining_cell->append( ch );
       }
       break;
     case -1: /* unprintable character */
@@ -560,11 +560,11 @@ void PredictionEngine::cull( const Framebuffer &fb )
 
 	/* match rest of row to the actual renditions */
 	{
-	  const Renditions &actual_renditions = fb.get_cell( i->row_num, j->col )->renditions;
+	  const Renditions &actual_renditions = fb.get_cell( i->row_num, j->col )->get_renditions();
 	  for ( overlay_cells_type::iterator k = j;
 		k != i->overlay_cells.end();
 		k++ ) {
-	    k->replacement.renditions = actual_renditions;
+	    k->replacement.get_renditions() = actual_renditions;
 	  }
 	}
 
@@ -777,21 +777,21 @@ void PredictionEngine::new_user_byte( char the_byte, const Framebuffer &fb )
 	cell.active = true;
 	cell.tentative_until_epoch = prediction_epoch;
 	cell.expire( local_frame_sent + 1, now );
-	cell.replacement.renditions = fb.ds.get_renditions();
+	cell.replacement.get_renditions() = fb.ds.get_renditions();
 
 	/* heuristic: match renditions of character to the left */
 	if ( cursor().col > 0 ) {
 	  ConditionalOverlayCell &prev_cell = the_row.overlay_cells[ cursor().col - 1 ];
 	  const Cell *prev_cell_actual = fb.get_cell( cursor().row, cursor().col - 1 );
 	  if ( prev_cell.active && (!prev_cell.unknown) ) {
-	    cell.replacement.renditions = prev_cell.replacement.renditions;
+	    cell.replacement.get_renditions() = prev_cell.replacement.get_renditions();
 	  } else {
-	    cell.replacement.renditions = prev_cell_actual->renditions;
+	    cell.replacement.get_renditions() = prev_cell_actual->get_renditions();
 	  }
 	}
 
-	cell.replacement.contents.clear();
-	cell.replacement.contents.push_back( ch );
+	cell.replacement.clear();
+	cell.replacement.append( ch );
 	cell.original_contents.push_back( *fb.get_cell( cursor().row, cursor().col ) );
 
 	/*
@@ -876,7 +876,7 @@ void PredictionEngine::newline_carriage_return( const Framebuffer &fb )
       j->active = true;
       j->tentative_until_epoch = prediction_epoch;
       j->expire( local_frame_sent + 1, now );
-      j->replacement.contents.clear();
+      j->replacement.clear();
     }
   } else {
     cursor().row++;

src/terminal/terminal.cc

@@ -107,7 +107,7 @@ void Emulator::print( const Parser::Print *act )
 
     fb.reset_cell( this_cell );
     this_cell->append( ch );
-    this_cell->width = chwidth;
+    this_cell->set_width( chwidth );
     fb.apply_renditions_to_cell( this_cell );
 
     if ( chwidth == 2 ) { /* erase overlapped cell */
@@ -128,18 +128,17 @@ void Emulator::print( const Parser::Print *act )
 	break;
       }
 
-      if ( combining_cell->contents.size() == 0 ) {
+      if ( combining_cell->empty() ) {
 	/* cell starts with combining character */
 	/* ... but isn't necessarily the target for a new
 	   base character [e.g. start of line], if the
 	   combining character has been cleared with
 	   a sequence like ED ("J") or EL ("K") */
-	assert( combining_cell->width == 1 );
-	combining_cell->fallback = true;
+	assert( combining_cell->get_width() == 1 );
+	combining_cell->set_fallback( true );
 	fb.ds.move_col( 1, true, true );
       }
-      if ( combining_cell->contents.size() < 32 ) {
-	/* seems like a reasonable limit on combining characters */
+      if ( !combining_cell->full() ) {
 	combining_cell->append( ch );
       }
       act->handled = true;

src/terminal/terminaldisplay.cc

@@ -337,10 +337,10 @@ bool Display::put_row( bool initialized, FrameState &frame, const Framebuffer &f
   /* If we're forced to write the first column because of wrap, go ahead and do so. */
   if ( wrap ) {
     const Cell &cell = cells.at( 0 );
-    frame.update_rendition( cell.renditions );
+    frame.update_rendition( cell.get_renditions() );
     frame.append_cell( cell );
-    frame_x += cell.width;
-    frame.cursor_x += cell.width;
+    frame_x += cell.get_width();
+    frame.cursor_x += cell.get_width();
   }
 
   /* If rows are the same object, we don't need to do anything at all. */
@@ -361,16 +361,16 @@ bool Display::put_row( bool initialized, FrameState &frame, const Framebuffer &f
     if ( initialized
 	 && !clear_count
 	 && ( cell == old_cells.at( frame_x ) ) ) {
-      frame_x += cell.width;
+      frame_x += cell.get_width();
       continue;
     }
 
     /* Slurp up all the empty cells */
-    if ( cell.contents.empty() ) {
+    if ( cell.empty() ) {
       if ( !clear_count ) {
-	blank_renditions = cell.renditions;
+	blank_renditions = cell.get_renditions();
       }
-      if ( cell.renditions == blank_renditions ) {
+      if ( cell.get_renditions() == blank_renditions ) {
 	/* Remember run of blank cells */
 	clear_count++;
 	frame_x++;
@@ -394,8 +394,8 @@ bool Display::put_row( bool initialized, FrameState &frame, const Framebuffer &f
       clear_count = 0;
       // If the current character is *another* empty cell in a different rendition,
       // we restart counting and continue here
-      if ( cell.contents.empty() ) {
-	blank_renditions = cell.renditions;
+      if ( cell.empty() ) {
+	blank_renditions = cell.get_renditions();
 	clear_count = 1;
 	frame_x++;
 	continue;
@@ -406,10 +406,10 @@ bool Display::put_row( bool initialized, FrameState &frame, const Framebuffer &f
     /* Now draw a character cell. */
     /* Move to the right position. */
     frame.append_silent_move( frame_y, frame_x );
-    frame.update_rendition( cell.renditions );
+    frame.update_rendition( cell.get_renditions() );
     frame.append_cell( cell );
-    frame_x += cell.width;
-    frame.cursor_x += cell.width;
+    frame_x += cell.get_width();
+    frame.cursor_x += cell.get_width();
     if ( frame_x >= f.ds.get_width() ) {
       wrote_last_cell = true;
     }

src/terminal/terminaldisplay.h

@@ -53,7 +53,6 @@ namespace Terminal {
     void append( const size_t s, const char c ) { str.append( s, c ); }
     void append( const wchar_t wc ) { Cell::append_to_str( str, wc ); }
     void append( const char * s ) { str.append( s ); }
-    void append( const Cell::content_type &contents ) { str.append( contents.begin(), contents.end() ); }
     void append_string( const std::string &append ) { str.append(append); }
 
     void append_cell(const Cell & cell) { cell.print_grapheme( str ); }

src/terminal/terminalframebuffer.cc

@@ -270,7 +270,7 @@ void Framebuffer::apply_renditions_to_cell( Cell *cell )
   if (!cell) {
     cell = get_mutable_cell();
   }
-  cell->renditions = ds.get_renditions();
+  cell->set_renditions( ds.get_renditions() );
 }
 
 SavedCursor::SavedCursor()

src/terminal/terminalframebuffer.h

@@ -85,13 +85,14 @@ namespace Terminal {
   };
 
   class Cell {
-  public:
+  private:
     typedef std::string content_type; /* can be std::string, std::vector<uint8_t>, or __gnu_cxx::__vstring */
     content_type contents;
     Renditions renditions;
     uint8_t width;
     bool fallback; /* first character is combining character */
 
+  public:
     Cell( color_type background_color );
     Cell(); /* default constructor required by C++11 STL */
 
@@ -107,8 +108,14 @@ namespace Terminal {
 
     bool operator!=( const Cell &x ) const { return !operator==( x ); }
 
+    /* Accessors for contents field */
     std::string debug_contents( void ) const;
 
+    bool empty( void ) const { return contents.empty(); }
+    /* 32 seems like a reasonable limit on combining characters */
+    bool full( void ) const { return contents.size() >= 32; }
+    void clear( void ) { contents.clear(); }
+
     bool is_blank( void ) const
     {
       // XXX fix.
@@ -176,6 +183,15 @@ namespace Terminal {
       }
       output.append( contents );
     }
+
+    /* Other accessors */
+    const Renditions& get_renditions( void ) const { return renditions; }
+    Renditions& get_renditions( void ) { return renditions; }
+    void set_renditions( const Renditions& r ) { renditions = r; }
+    unsigned int get_width( void ) const { return width; }
+    void set_width( unsigned int w ) { width = w; }
+    bool get_fallback( void ) const { return fallback; }
+    void set_fallback( bool f ) { fallback = f; }
   };
 
   class Row {
@@ -299,7 +315,8 @@ namespace Terminal {
     void set_foreground_color( int x ) { renditions.set_foreground_color( x ); }
     void set_background_color( int x ) { renditions.set_background_color( x ); }
     void add_rendition( color_type x ) { renditions.set_rendition( x ); }
-    Renditions get_renditions( void ) const { return renditions; }
+    const Renditions& get_renditions( void ) const { return renditions; }
+    Renditions& get_renditions( void ) { return renditions; }
     int get_background_rendition( void ) const { return renditions.background_color; }
 
     void save_cursor( void );

src/terminal/terminalfunctions.cc

@@ -148,7 +148,7 @@ static void Esc_DECALN( Framebuffer *fb, Dispatcher *dispatch __attribute((unuse
   for ( int y = 0; y < fb->ds.get_height(); y++ ) {
     for ( int x = 0; x < fb->ds.get_width(); x++ ) {
       fb->reset_cell( fb->get_mutable_cell( y, x ) );
-      fb->get_mutable_cell( y, x )->contents.push_back( L'E' );
+      fb->get_mutable_cell( y, x )->append( 'E' );
     }
   }
 }

src/tests/Makefile.am

@@ -21,6 +21,7 @@ displaytests = \
 	emulation-cursor-motion.test \
 	emulation-multiline-scroll.test \
 	emulation-wrap-across-frames.test \
+	prediction-unicode.test \
 	pty-deadlock.test \
 	server-network-timeout.test \
 	server-signal-timeout.test \

src/tests/README.md

@@ -90,6 +90,11 @@ state.  Alternately, this could use expect or something similar.
 between tmux and mosh.  It's expected to interact with its wrapped
 command line as `expect` might do.  This is not actually tested yet.
 
+### Flags
+
+`mosh-args`, `client-args` and `server-args` inject extra arguments
+into the invocations of the respective commands.
+
 ## Logging and error reporting
 
 Each execution action is run, and recorded in

src/tests/e2e-test

@@ -96,7 +96,7 @@ mosh_client()
 	test_error "mosh_client: variables missing\n"
     fi
     exec 2> "${MOSH_E2E_TEST}.client.stderr"
-    exec "$MOSH_CLIENT" "$@"
+    exec "$MOSH_CLIENT" $MOSH_CLIENT_ARGS "$@"
 }
 
 mosh_server()
@@ -105,7 +105,7 @@ mosh_server()
 	test_error "mosh_server: variables missing\n"
     fi
     exec 2> "${MOSH_E2E_TEST}.server.stderr"
-    exec "$MOSH_SERVER" new -v -@ "$@"
+    exec "$MOSH_SERVER" new -v $MOSH_SERVER_ARGS -@ "$@"
 }
 
 # main
@@ -176,6 +176,12 @@ for i in $test_args; do
 	    client=1;;
 	post)
 	    post=1;;
+	mosh-args)
+	    mosh_args=$(${test_script} mosh-args);;
+	client-args)
+	    export MOSH_CLIENT_ARGS=$(${test_script} client-args);;
+	server-args)
+	    export MOSH_SERVER_ARGS=$(${test_script} server-args);;
 	*)
 	    error "unknown test type argument %s", $i
 	    exit 99
@@ -201,7 +207,7 @@ for run in $server_tests; do
     export MOSH_SERVER="$PWD/../frontend/mosh-server"
     export MOSH_E2E_TEST="$PWD/${test_dir}/${run}"
     # XXX need to quote special chars in server pathname here somehow
-    sut="../../scripts/mosh --client=${srcdir}/mosh-client --server=${srcdir}/mosh-server --local --bind-server=127.0.0.1 127.0.0.1"
+    sut="../../scripts/mosh --client=${srcdir}/mosh-client --server=${srcdir}/mosh-server --local --bind-server=127.0.0.1 ${mosh_args} 127.0.0.1"
     testarg=$run
     if [ "$run" = "direct" ]; then
 	sut=""

src/tests/prediction-unicode.test

@@ -0,0 +1,150 @@
+#!/bin/sh
+
+#
+# This is a regression test for a bug seen where mosh-client's
+# prediction code would sometimes show "gück" when "glück" was typed.
+# mosh-client would output a predicted Unicode input character
+# first as an 8-bit character containing the lowest 8 bits of the
+# Unicode code point, then redraw it correctly with its UTF-8 sequence
+# when the prediction is validated.  For many accented Latin
+# characters, the 8-bit character is an illegal UTF-8 code sequence.
+# Most terminal emulators will output the Unicode replacement
+# character, which is only visible until validation.  urxvt, however,
+# draws no character and does not change the cursor location on an
+# illegal UTF-8 sequence, causing this bug to be visible as ongoing
+# display corruption.  A subset of wide characters (including CJK)
+# will show display corruption with all terminal emulators, because a
+# narrow replacement character will be drawn when a wide character
+# should have been.
+#
+# tmux draws a replacement character for invalid UTF-8, and we
+# depend on that in this test.
+#
+# Another similar failing case is typing "faĩl".  In this case the "ĩ"
+# would be predicted as ")" before being replaced by the
+# correct character.
+#
+fail()
+{
+    printf "$@" 2>&1
+    exit 99
+}
+
+
+
+PATH=$PATH:.:$srcdir
+# Top-level wrapper.
+if [ $# -eq 0 ]; then
+    e2e-test $0 tmux baseline mosh-args post
+    exit
+fi
+
+sleepf()
+{
+    (sleep .1 || sleep 1) > /dev/null 2>&1
+}
+
+seq()
+{
+    if [ $# -lt 1 -o $# -gt 3 ]; then
+	echo "bad args" >&2
+    fi
+    first=$1
+    incr=1
+    last=0
+    case $# in
+	3)
+	    incr=$2
+	    last=$3
+	    ;;
+	2)
+	    last=$2
+	    ;;
+	1)
+	    ;;
+    esac
+    while :; do
+	printf '%d\n' $first
+	first=$(expr $first + $incr)
+	if [ $first -gt $last ]; then
+	    break
+	fi
+    done
+}
+
+chr()
+{
+    printf "\\$(printf %03o $1)"
+}
+
+utf8cp()
+{
+    local c=$1
+    if [ $c -gt $((0x10ffff)) ]; then
+	fail "illegal Unicode code point %x\n" $c
+    elif [ $c -lt $((0x80)) ]; then
+	chr $c
+    elif [ $c -lt $((0x800)) ]; then
+	chr $(( (($c >>  6) & 0x1f) | 0xc0 ))
+	chr $(( ($c & 0x3f)         | 0x80 ))
+    elif [ $c -lt $((0x10000)) ]; then
+	chr $(( (($c >> 12) & 0x0f) | 0xe0 ))
+	chr $(( (($c >>  6) & 0x3f) | 0x80 ))
+	chr $(( ($c & 0x3f)         | 0x80 ))
+    elif [ $c -lt $((0x200000)) ]; then
+	chr $(( (($c >> 18) & 0x03) | 0xf0 ))
+	chr $(( (($c >> 12) & 0x3f) | 0x80 ))
+	chr $(( (($c >>  6) & 0x3f) | 0x80 ))
+	chr $(( ($c & 0x3f)         | 0x80 ))
+    fi
+}
+
+tmux_commands()
+{
+    for x in $(seq 1 5); do
+	for y in $(seq 1 5); do 
+	    for i in g l ü c k " " f a ĩ l " "; do
+		printf "send-keys '%s'\n" "$i"
+		sleepf
+	    done
+	done
+	printf "send-keys 0x0d\n"
+    done
+    printf "send-keys 0x04\n"
+    sleep 5
+} 
+
+tmux_stdin()
+{
+    tmux_commands | "$@"
+    exit
+}
+
+baseline()
+{
+    # Just receive and toss input in canonical mode.
+    cat > /dev/null
+}
+
+post()
+{
+    # Look for bad output: ')' or \374
+    if [ -n "$(env -u LC_ALL -u LC_CTYPE -u LANGUAGE LANG=C egrep "%output %0 (\)|$(printf \\374))" $(basename $0).d/baseline.tmux.log)" ]; then
+        exit 1
+    fi
+    exit 0
+}
+
+case $1 in
+    tmux)
+	shift;
+	tmux_stdin "$@";;
+    baseline)
+	baseline;;
+    mosh-args)
+	printf "%s\n" "--predict=always";;
+    post)
+	post;;
+    *)
+	fail "unknown test argument %s\n" $1;;
+esac