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

[Raphael Isemann via Phabricator via lldb-commits]

teemperor added a comment.

Added some comments on the new code. Also we should be able to see these spaces in the pexpect output, so I guess we should be able to test this too? To make this special case less fragile to test, you can make a new subtest in the Test case by just defining a new function:

  @skipIfAsan
  @skipIfEditlineSupportMissing
  def test_hidden_autosuggestion(self):
  @skipIfAsan
  @skipIfEditlineSupportMissing
  def test_autosuggestion(self):
      self.launch(extra_args=["-o", "settings set show-autosuggestion true", "-o", "settings set use-color true"])
      self.expect("help frame v")
      self.expect("help frame info")
      [type 'help frame v' and check for the three space after the grey part to cover up the "nfo" text]

So, I *believe* that everything that we want to have in this patch has been addressed? There are quite a few comments in this thread so it's hard to say if there are open issues. I played around with the current patch for a bit and I couldn't find anymore issues, so I think this is in pretty good shape in any case.

Let's see if @labath or @JDevlieghere have any more comments, otherwise I would say this can go in and the rest can be fixed as follow up commits.



================
Comment at: lldb/include/lldb/Host/Editline.h:377
+  void *m_suggestion_callback_baton = nullptr;
+  int m_previous_autosuggestion_size = 0;
   std::mutex m_output_mutex;
----------------
This probably should be `std::size_t` type (a negative previous suggestion size seems weird). Also add a comment that this is the length of the previous autosuggestion + user input in bytes.


================
Comment at: lldb/source/Host/common/Editline.cpp:1071
+                          (int)to_add.getValue().length();
+    if (spaces_to_print > 0) {
+      std::string spaces = std::string(spaces_to_print, ' ');
----------------
Whoever sees this in the future will really wonder why we print a bunch of spaces here, so I think a comment like "Print spaces to hide any remains of a previous longer autosuggestion".


================
Comment at: lldb/source/Host/common/Editline.cpp:1504
 #endif
+      if ((int)line.length() > m_previous_autosuggestion_size)
+        m_previous_autosuggestion_size = (int)line.length();
----------------
I don't think the value of `m_previous_autosuggestion_size` should only grow (which is what this `if` is doing), as this way we just keep printing a longer and longer space buffer over time. Just printing enough to clear the buffer of the previous completion is enough.

Also you can just do this calculation directly after you calculated `int spaces_to_print` above. This way this code is closer together which hopefully makes this easier to understand.


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

https://reviews.llvm.org/D81001