added a command line flag and associated code to support dumb terminals #99

aperotte · 2017-02-05T19:03:19Z

Emacs and other subshells that manage their own input result in noisy output at the cling interpreter. More specifically, entering "int x = 5;" at the prompt will result in the interpreter recursively printing the input adding one character each time - "iinintint int xint x int x =int x = int x = 5int x = 5;int x = 5;". This seems to be because cling cannot control the cursor location in dumb terminals and therefore cannot update the text at the prompt with each new character. It instead prints each update.

Subshells such as those in emacs, manage user input until a newline and then send the input to the underlying process. The output of the underlying process is echoed in the subshell. Because this shell is managed and cling does not have as much control, this results in noisy output.

This pull request adds a command line flag to indicate whether this is a dumb terminal. If so, it does not update the content of the terminal on each input character and does not add a newline (since the dumb terminal would add its own). This is implemented as a member variable of the TextInput class.

… terminal

Axel-Naumann · 2017-02-07T15:58:44Z

Thanks, Adler!

therefore cannot update the text at the prompt with each new character. It instead prints each update.

I was hoping for textinput to only print that new character. That's what I expected it to do - instead of re-writing the whole line. Can you find out / remind us why it prints everything again and again when adding a single character?

If we manage to fix that (i.e. only add that new character) then we don't need a flag at all - everything will work, magically :-)

aperotte · 2017-02-08T20:37:01Z

I'll take a look! However, I do think it's still a better user experience if the input is not echoed at all on these dumb terminals because the input will already be there and would be duplicated if there is any printing on user input.

Axel-Naumann · 2017-02-09T15:54:25Z

Let's step back for a second: when I run M-x term in emacs and then start cling, the prompt without your patch seems to work fine. I have this as TERM:

$ echo $TERM
eterm-color

What do you have?

aperotte · 2017-02-09T16:24:32Z

Yes, unfortunately, term is not the subshell that the other emacs tools are built on (comint, for example). I believe that the tools are built on eshell which is why the interactive cling mode that I am working on for emacs has the noisy echo.

Welcome to the Emacs shell

~ $ echo $TERM
dumb
~ $

Axel-Naumann · 2017-02-09T19:41:58Z

Ah that's great! textinput already checks for $TERM (in TerminalDisplayUnix.cpp) - can we switch to dumb mode for a dumb TERM instead of adding an extra flag?

aperotte · 2017-02-09T23:17:52Z

Yes, that makes sense! I'll look into that.

Axel-Naumann · 2017-02-21T15:03:23Z

Any news? I really like your patch, and changing this to be $TERM sensitive sounds like a small-ish change?

aperotte · 2017-02-22T00:20:02Z

I've been swamped, but I will work on it very soon

…l type and respond appropriately when type is dumb

aperotte · 2017-03-05T16:31:51Z

Sorry for the delay. I pushed an update to my fork that removes the flag and detects when terminal type is "dumb". However, since I had to make the change to TerminalDisplay, this may affect Windows as well (which I didn't test). Since TerminalDisplay has a fTerm field that holds the terminal type as a string, I set it to "win" in the subclass TerminalDisplayWin

Axel-Naumann

Very nice, thanks a lot! That's much better! I'd still move some of the interfaces around, to make this even more transparent / simple. Could you address the comments?

Axel-Naumann · 2017-03-07T13:08:40Z

lib/Interpreter/InvocationOptions.cpp

@@ -145,9 +145,11 @@ void CompilerOptions::Parse(int argc, const char* const argv[],
 }

 InvocationOptions::InvocationOptions(int argc, const char* const* argv) :
+


Can you remove the changes in this file from this PR?

Axel-Naumann · 2017-03-07T13:10:29Z

lib/UserInterface/UserInterface.cpp

@@ -98,6 +98,9 @@ namespace cling {
    UITabCompletion* Completion =
                      new UITabCompletion(m_MetaProcessor->getInterpreter());
    TI.SetCompletion(Completion);
+    if (D->GetTERM() &&  strstr(D->GetTERM(), "dumb")) {
+        TI.SetIsDumbTerm(true);


Can't this be done inside textinput itself? See below...

Axel-Naumann · 2017-03-07T13:10:55Z

lib/UserInterface/textinput/TerminalDisplay.h

@@ -40,6 +40,8 @@ namespace textinput {
    void Detach();
    void DisplayInfo(const std::vector<std::string>& Options);
    bool IsTTY() const { return fIsTTY; }
+    void SetTERM(const char* TERM) { fTERM = TERM; };
+    const char* GetTERM() { return fTERM; };


These interfaces (nor the data member) should not be needed, see below. Instead we'd need an IsDumb().

Axel-Naumann · 2017-03-07T13:11:42Z

lib/UserInterface/textinput/TextInput.cpp

@@ -31,6 +31,7 @@
 namespace textinput {
  TextInput::TextInput(Reader& reader, Display& display,
                       const char* HistFile /* = 0 */):
+  fIsDumbTerm(false),


Isn't that a property of the TerminalDisplay? I'd prefer to have the interface there.

Axel-Naumann · 2017-03-07T13:12:27Z

lib/UserInterface/textinput/TerminalDisplayUnix.cpp

@@ -119,6 +119,7 @@ namespace textinput {
    TerminalConfigUnix::Get().TIOS()->c_lflag |= ECHOCTL|ECHOKE|ECHOE;
 #endif
    const char* TERM = getenv("TERM");
+    SetTERM(TERM);


If TERM is "dumb", the dumb flag on the TerminalDisplay base should be set.

Axel-Naumann · 2017-03-07T13:13:36Z

lib/UserInterface/textinput/TextInput.cpp

+    if (!fIsDumbTerm) {
+        // Signal displays that the input got taken.
+        std::for_each(fContext->GetDisplays().begin(), fContext->GetDisplays().end(),
+                      std::mem_fun(&Display::NotifyResetInput));


The if should test the display of the current iteration: if it's dumb, don't call NotifyResetInput..

Axel-Naumann · 2017-03-07T13:14:16Z

lib/UserInterface/textinput/TextInput.cpp

@@ -124,7 +127,9 @@ namespace textinput {
             || (*iR)->HaveBufferedInput()) {
        if ((*iR)->ReadInput(nRead, in)) {
          ProcessNewInput(in, R);
-          DisplayNewInput(R, OldCursorPos);
+          if (!IsDumbTerm()) {


I'd move that if into DisplayNewInput, testing the dumbness of the display.

Axel-Naumann · 2017-03-07T13:14:36Z

lib/UserInterface/textinput/TextInput.h

+    void SetIsDumbTerm(bool isDumbTerm) { fIsDumbTerm = isDumbTerm; }
+
+  protected:
+    bool fIsDumbTerm; // whether this is a dumb terminal or not


These should all be moved to TerminalDisplay.

aperotte · 2017-03-13T11:58:37Z

I will take a look, thanks for the comments.

lazywithclass · 2017-10-15T15:55:37Z

@Axel-Naumann Hello, I'd love this to get merged. C++ is definitely not my thing but I'm learning it, would you be open to accept the requested changes and possibly give me some advice on the way?

aperotte · 2018-08-21T15:06:57Z

Hi, apologies, but I had to stop working on this pull request because of other responsibilities. I would also love to see this completed if @rilerez or @lazywithclass has some time to work on the refactoring and new conflicts.

rileylev · 2020-07-17T22:48:46Z

Has there been any progress? If not, I'm happy to give it a go, but I might need some pointers to get started.

ferdymercury · 2024-04-12T09:36:20Z

I think this repo is just a mirror, consider moving your PR here instead: https://github.com/root-project/root/pulls (subdirectory interpreter/cling). Thanks!

added a command line flag and associated code to support using a dumb…

4cf547b

… terminal

Axel-Naumann mentioned this pull request Feb 8, 2017

Fix returning to stdout after redirection. #103

Closed

aperotte and others added 2 commits February 21, 2017 19:22

Merge branch 'master' into master

b98c716

removed the flag and modified TerminalDisplay class to detect termina…

a33965a

…l type and respond appropriately when type is dumb

Axel-Naumann requested changes Mar 7, 2017

View reviewed changes

rileylev mentioned this pull request Aug 21, 2018

Noisy "cling" outputs brianqq/inferior-cling#1

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

added a command line flag and associated code to support dumb terminals #99

added a command line flag and associated code to support dumb terminals #99

aperotte commented Feb 5, 2017

Axel-Naumann commented Feb 7, 2017

aperotte commented Feb 8, 2017

Axel-Naumann commented Feb 9, 2017

aperotte commented Feb 9, 2017

Axel-Naumann commented Feb 9, 2017

aperotte commented Feb 9, 2017

Axel-Naumann commented Feb 21, 2017

aperotte commented Feb 22, 2017

aperotte commented Mar 5, 2017

Axel-Naumann left a comment

Axel-Naumann Mar 7, 2017

Axel-Naumann Mar 7, 2017

Axel-Naumann Mar 7, 2017

Axel-Naumann Mar 7, 2017

Axel-Naumann Mar 7, 2017

Axel-Naumann Mar 7, 2017

Axel-Naumann Mar 7, 2017

Axel-Naumann Mar 7, 2017

aperotte commented Mar 13, 2017

lazywithclass commented Oct 15, 2017

aperotte commented Aug 21, 2018

rileylev commented Jul 17, 2020

ferdymercury commented Apr 12, 2024

		@@ -145,9 +145,11 @@ void CompilerOptions::Parse(int argc, const char* const argv[],
		}

		InvocationOptions::InvocationOptions(int argc, const char* const* argv) :

added a command line flag and associated code to support dumb terminals #99

Are you sure you want to change the base?

added a command line flag and associated code to support dumb terminals #99

Conversation

aperotte commented Feb 5, 2017

Axel-Naumann commented Feb 7, 2017

aperotte commented Feb 8, 2017

Axel-Naumann commented Feb 9, 2017

aperotte commented Feb 9, 2017

Axel-Naumann commented Feb 9, 2017

aperotte commented Feb 9, 2017

Axel-Naumann commented Feb 21, 2017

aperotte commented Feb 22, 2017

aperotte commented Mar 5, 2017

Axel-Naumann left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aperotte commented Mar 13, 2017

lazywithclass commented Oct 15, 2017

aperotte commented Aug 21, 2018

rileylev commented Jul 17, 2020

ferdymercury commented Apr 12, 2024