[Lldb-commits] [PATCH] D81001: [lldb] Display autosuggestion part in gray if there is one possible suggestion

[Raphael Isemann via Phabricator via lldb-commits]

teemperor requested changes to this revision.
teemperor added a comment.
This revision now requires changes to proceed.

Sorry this is taking so long, but I think beside one minor change this only needs some testing and then it's ready.

So the way I would like to see this tested would be with a pexpect test. Pexpect just launches a virtual terminal where LLDB runs inside and you can send input and read output. There is a good example test here that you can copy `lldb/test/API/commands/expression/multiline-navigation/TestMultilineNavigation.py`. I think it's a good enough test if you just type any lldb command like `help frame` in the test, press enter and then check that if you type like `hel` you get the autosuggestion displayed in the terminal output. And then you press Ctrl+F in the terminal (you have to find the escape sequence for that when sending it to pexpect) and should see the complete autosuggestion.

Be aware that there is chance that the Pexpect terminal behaves differently than a real terminal and you see wrong results there. In that case we need to come up with another solution, but let's first try pexpect. Also you should know that pexpect tests don't know if LLDB is actually still producing output or idling, so if get timeouts while reading the output that just means we didn't see the right output.



================
Comment at: lldb/source/Host/common/Editline.cpp:1007
+      std::string to_add_autosuggestion = "";
+      m_suggestion_callback(line, to_add_autosuggestion,
+                            m_suggestion_callback_baton);
----------------
This will crash with disabled suggestions (m_suggestion_callback can be null if the feature is disabled).


================
Comment at: lldb/source/Host/common/Editline.cpp:1017
+          el_insertstr(m_editline, to_add.c_str());
+          return CC_REFRESH;
+        }
----------------
gedatsu217 wrote:
> teemperor wrote:
> > If I understand the only effect this whole code has is to return CC_REFRESH instead of CC_REDISPLAY? If yes, then I think you can just replace the `break` below with `return CC_REFRESH;` which will be much simpler.
> > If yes, then I think you can just replace the break below with return CC_REFRESH; which will be much simpler.
> 
> Isn't it "return CC_REDISPLAY", not "CC_REFRESH"? I want to return CC_REFRESH only when "to_add" is in "to_add_autosuggestion" (e.g. to_add = b, to_add_autosuggestion = "breakpoint").  
> 
> That is because CC_REDISPLAY deletes the gray-colored autosuggested part, while CC_REFRESH leaves it.
> 
> 
I see. What about just retuning always `CC_REFRESH` here? That should work as we only add text to the end with a normal completion (which is IIRC that's what `CC_REFRESH` is for, but 

```
lang=c++
    case CompletionMode::Normal: {
      std::string to_add = completion.GetCompletion();
      to_add = to_add.substr(request.GetCursorArgumentPrefix().size());
      std::string to_add_autosuggestion = "";
      to_add.push_back(' ');
      el_insertstr(m_editline, to_add.c_str());
      return CC_REFRESH;
    }
```

That seems to work for me (and it avoids the crash I pointed out above).

Also my main point here is that this is quite a large change just to change the return value (and the other tab completions below aren't covered and would need similar changes if we do this change).


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D81001/new/

https://reviews.llvm.org/D81001