Add a way to enable/disable echo in TermImpl echoHandler

[diggernet]

Read me

Read this first before creating an issue:

• do not use this issue tracker to ask questions, instead use one of these channels. Questions will likely be closed without notice.
• you shall create a feature request only when it is general purpose enough.
• make sure that the feature is not already

Describe the feature

Allow enable/disable echoing input characters back to the remote client.

Use cases

This allows temporarily disabling echo, for example to input a password, without replacing echoHandler and losing built-in readline functionality.

Contribution

Proposed implementation in TermImpl.java:

• Add a new class field:

  private boolean echoOn = true;

• Add an if block in the constructor:

    echoHandler = codePoints -> {
      // Echo
      if (echoOn) {
        echo(codePoints);
      }
      readline.queueEvent(codePoints);
    };

• Add a setter:

  public void setEchoOn(boolean echoOn) {
    this.echoOn = echoOn;
  }

[vietj]

@diggernet can you provide a PR for this ?

[diggernet]

Hi Julien,
I started to do a quick PR, but setting up CLA and commit signing were a lot of overhead for 6 lines of code. This seemed like a quicker route for such a small change.
If you want I can do that, but it'll be a bit before I can focus on it.
By the way, what's the timeline for 5.0?

[vietj]

I can apply the patch myself, however can you provide a test for it ?

5.0 GA is ready, however it depends on Netty 4.2 GA release that should be released by the end of this month hopefully

[diggernet]

Ok, I gave it a shot. Here's my branch with my implementation and test:
https://github.com/diggernet/vertx-shell/tree/echo
I created ShellTest.testEchoEnableDisable(), copied largely from ShellTest.testEchoCharsDuringExecute(), which was the only test I found that already actually hit TermImpl.echoHandler.

But there is a concurrency problem in the test. This line fails:
testContext.assertNull(conn.checkWritten("C"));
When I have it print out the return value from checkWritten("C"), it is getting:
Was expecting <B> to be equals to <C>
But when I step through the test in the debugger everything works perfectly.

So how is "B" is getting into TestTtyConnection.out?
When I'm not stepping in the debugger, the execution flow turns out to be:

• echoHandler("A")
• echoOn(false)
• echoOn(true)
• echoHandler("B") <-- should have been called between the echoOn calls
• echoHandler("C")

Presumably that is a side-effect of runOnContext() in TestTtyConnection.read(). Any suggestions to get this test executing in the right order?

Thanks!

[diggernet]

Hi @vietj ,
Got it working!
I just called conn.getStdinHandler.accept() directly, instead of indirectly through runOnContext() by using conn.read().
Changes at the link above, or if you prefer here's the full diff:

git diff b1115ee 496225
diff --git a/src/main/java/io/vertx/ext/shell/term/Term.java b/src/main/java/io/vertx/ext/shell/term/Term.java
index 69780f6..735fd4f 100644
--- a/src/main/java/io/vertx/ext/shell/term/Term.java
+++ b/src/main/java/io/vertx/ext/shell/term/Term.java
@@ -77,6 +77,14 @@ public interface Term extends Tty {
    */
   Term setSession(Session session);
 
+  /**
+   * Enable or disable echoing input chars to the remote client in the default stdinHandler.
+   * 
+   * @param echoOn True to enable echo (default), false to disable.
+   * @return a reference to this, so the API can be used fluently
+   */
+  Term setEchoOn(boolean echoOn);
+
   /**
    * Set an interrupt signal handler on the term.
    *
diff --git a/src/main/java/io/vertx/ext/shell/term/impl/TermImpl.java b/src/main/java/io/vertx/ext/shell/term/impl/TermImpl.java
index 48f4dc5..671a2c2 100644
--- a/src/main/java/io/vertx/ext/shell/term/impl/TermImpl.java
+++ b/src/main/java/io/vertx/ext/shell/term/impl/TermImpl.java
@@ -67,6 +67,7 @@ public class TermImpl implements Term {
   private SignalHandler suspendHandler;
   private Session session;
   private boolean inReadline;
+  private boolean echoOn = true;
 
   public TermImpl(Vertx vertx, Keymap keymap, TtyConnection conn) {
     this(vertx, keymap, conn, null);
@@ -88,7 +89,9 @@ public class TermImpl implements Term {
     this.readlineFunctions.forEach(readline::addFunction);
     this.echoHandler = codePoints -> {
       // Echo
-      echo(codePoints);
+      if (echoOn) {
+        echo(codePoints);
+      }
       readline.queueEvent(codePoints);
     };
     conn.setStdinHandler(echoHandler);
@@ -123,6 +126,12 @@ public class TermImpl implements Term {
     return this;
   }
 
+  @Override
+  public Term setEchoOn(boolean echoOn) {
+    this.echoOn = echoOn;
+    return this;
+  }
+
   @Override
   public void readline(String prompt, Handler<String> lineHandler) {
     if (conn.getStdinHandler() != echoHandler) {
diff --git a/src/test/java/io/vertx/ext/shell/ShellTest.java b/src/test/java/io/vertx/ext/shell/ShellTest.java
index d71d9cd..1a4112a 100644
--- a/src/test/java/io/vertx/ext/shell/ShellTest.java
+++ b/src/test/java/io/vertx/ext/shell/ShellTest.java
@@ -33,6 +33,7 @@
 package io.vertx.ext.shell;
 
 import io.termd.core.tty.TtyEvent;
+import io.termd.core.util.Helper;
 import io.vertx.core.AbstractVerticle;
 import io.vertx.core.Context;
 import io.vertx.core.Vertx;
@@ -49,6 +50,7 @@ import io.vertx.ext.shell.system.ExecStatus;
 import io.vertx.ext.shell.system.Job;
 import io.vertx.ext.shell.system.impl.InternalCommandManager;
 import io.vertx.ext.shell.system.impl.JobImpl;
+import io.vertx.ext.shell.term.Term;
 import io.vertx.ext.shell.term.impl.TermImpl;
 import io.vertx.ext.unit.Async;
 import io.vertx.ext.unit.TestContext;
@@ -90,7 +92,11 @@ public class ShellTest {
   }
 
   private ShellImpl createShell(TestTtyConnection conn) {
-    return new ShellImpl(new TermImpl(vertx, conn), new InternalCommandManager(commands));
+    return createShell(new TermImpl(vertx, conn));
+  }
+
+  private ShellImpl createShell(Term term) {
+    return new ShellImpl(term, new InternalCommandManager(commands));
   }
 
   @Test
@@ -403,6 +409,34 @@ public class ShellTest {
       .onComplete(testContext.asyncAssertSuccess(id -> conn.read("foo\r")));
   }
 
+  @Test
+  public void testEchoEnableDisable(TestContext testContext) throws Exception {
+    TestTtyConnection conn = new TestTtyConnection(vertx);
+    Term term = new TermImpl(vertx, conn);
+    ShellImpl shell = createShell(term);
+    shell.init().readline();
+    Async async = testContext.async();
+    vertx.deployVerticle(new AbstractVerticle() {
+        @Override
+        public void start() {
+          commands.add(CommandBuilder.command("foo").processHandler(process -> {
+            testContext.assertEquals(null, conn.checkWritten("% foo\n"));
+            conn.getStdinHandler().accept(Helper.toCodePoints("A"));
+            testContext.assertNull(conn.checkWritten("A"));
+            term.setEchoOn(false);
+            conn.getStdinHandler().accept(Helper.toCodePoints("B"));
+            testContext.assertNull(conn.checkWritten(""));
+            term.setEchoOn(true);
+            conn.out().setLength(0);
+            conn.getStdinHandler().accept(Helper.toCodePoints("C"));
+            testContext.assertNull(conn.checkWritten("C"));
+            async.complete();
+          }));
+        }
+      })
+      .onComplete(testContext.asyncAssertSuccess(id -> conn.read("foo\r")));
+  }
+
   public void testExit(TestContext context) throws Exception {
     commands.add(Command.create(vertx, Sleep.class));
     for (String cmd : Arrays.asList("exit", "logout")) {