[LLDB] Proposed change in multi-line edit behavior (Return = end/append, Meta+Return = line break)

[k8stone]

Summary

Editing multi-line content in a terminal environment involves a lot of trade-offs. When LLDB's multi-line editing support was first introduced for expressions / REPL contexts the behavior was as follows:

• The Return key is treated as a line-break except at the end of the input buffer, where a completeness test is applied

This worked well enough when writing code, and makes it trivial to insert new lines above code you've already typed. Just use cursor navigation to move up and type freely. Where it was awkward is that the gesture to insert a line break and end editing is conflated for most people. Sometimes you want Return to end the editing session and other times you want to insert a line break.

This patch proposes a change in the behavior as follows:

• The Return key is treated as the end of editing except at the end of the input buffer, where a completeness test is applied
• The Meta+Return sequence is always treated as a line break. This is consistent with conventions in Facebook and elsewhere since Alt/Option+Return is often mapped to Meta+Return. The unfortunate exception is on macOS where this *can* be the case, but isn't by default. Sigh.

Note that by design both before and after the patch pasting a Return character always introduces a line break.

[k8stone]

This is based on a patch originally submitted by Bartosz Wróblewski here: https://llvm.org/bugs/show_bug.cgi?id=27365

[Eugene.Zelenko]

include/lldb/Host/Editline.h

289

Will be good idea to not add space between name and arguments list in new code. Same for other places.

[labath]

looks good as far as I can tell

I second the idea of running clang-format over the patch.

[clayborg]

We should document this behavior somewhere in some help text, and run clang-format.

[k8stone]

clang-format applied. Greg's recommendation to document the multi-line editing behavior in help make sense, but should land after the major help rework in progress.

[Eugene.Zelenko]

Committed in r275482.

Please mention URL of Differential revision to close it automatically after commit.

[Eugene.Zelenko]

Sorry, forgot to add "[revision] in commit summary".

[k8stone]

Fair enough! I was planning to close both out tomorrow after getting Jim to approve the other patch.

include/lldb/Host/Editline.h

         /// Prompt implementation for EditLine.
         const char *
         Prompt();
-        
-        /// Line break command used when return is pressed in multi-line mode.
+        /// Line break command used when meta+return is pressed in multi-line mode.
         unsigned char
         BreakLineCommand (int ch);
-        
+        /// Command used when return is pressed in multi-line mode.
+        unsigned char
+        EndOrAddLineCommand(int ch);
+
         /// Delete command used when delete is pressed in multi-line mode.
         unsigned char
         DeleteNextCharCommand (int ch);

         /// Line navigation command used when ^N or down arrow are pressed in multi-line mode.
         unsigned char
         NextLineCommand (int ch);
-        
+        /// History navigation command used when Alt + up arrow is pressed in multi-line mode.
+        unsigned char
+        PreviousHistoryCommand(int ch);
+
+        /// History navigation command used when Alt + down arrow is pressed in multi-line mode.
+        unsigned char
+        NextHistoryCommand(int ch);
+
         /// Buffer start command used when Esc < is typed in multi-line emacs mode.
         unsigned char
         BufferStartCommand (int ch);

source/Host/common/Editline.cpp

     // Establish the new cursor position at the start of a line when inserting a line break
     m_revert_cursor_index = 0;
 
-    // Don't perform end of input detection or automatic formatting when pasting
+    // Don't perform automatic formatting when pasting
     if (!IsInputPending (m_input_file))
     {
-        // If this is the end of the last line, treat this as a potential exit
-        if (m_current_line_index == m_input_lines.size() - 1 && new_line_fragment.length() == 0)
-        {
-            bool end_of_input = true;
-            if (m_is_input_complete_callback) 
-            {
-                SaveEditedLine();
-                auto lines = GetInputAsStringList();
-                end_of_input = m_is_input_complete_callback (this, lines, m_is_input_complete_callback_baton);
-
-                // The completion test is allowed to change the input lines when complete
-                if (end_of_input)
-                {
-                    m_input_lines.clear();
-                    for (unsigned index = 0; index < lines.GetSize(); index++)
-                    {
-#if LLDB_EDITLINE_USE_WCHAR
-                        m_input_lines.insert (m_input_lines.end(), m_utf8conv.from_bytes (lines[index]));
-#else
-                        m_input_lines.insert (m_input_lines.end(), lines[index]);
-#endif
-                    }
-                }
-            }
-            if (end_of_input)
-            {
-                fprintf (m_output_file, "\n");
-                m_editor_status = EditorStatus::Complete;
-                return CC_NEWLINE;
-            }
-        }
-        
         // Apply smart indentation
         if (m_fix_indentation_callback) 
         {

 }
 
 unsigned char
-Editline::DeleteNextCharCommand (int ch)
+Editline::EndOrAddLineCommand(int ch)
 {
+    // Don't perform end of input detection when pasting, always treat this as a line break
+    if (IsInputPending(m_input_file))
+    {
+        return BreakLineCommand(ch);
+    }
+
+    // Save any edits to this line
+    SaveEditedLine();
+
+    // If this is the end of the last line, consider whether to add a line instead
+    const LineInfoW *info = el_wline(m_editline);
+    if (m_current_line_index == m_input_lines.size() - 1 && info->cursor == info->lastchar)
+    {
+        if (m_is_input_complete_callback)
+        {
+            auto lines = GetInputAsStringList();
+            if (!m_is_input_complete_callback(this, lines, m_is_input_complete_callback_baton))
+            {
+                return BreakLineCommand(ch);
+            }
+
+            // The completion test is allowed to change the input lines when complete
+            m_input_lines.clear();
+            for (unsigned index = 0; index < lines.GetSize(); index++)
+            {
+#if LLDB_EDITLINE_USE_WCHAR
+                m_input_lines.insert(m_input_lines.end(), m_utf8conv.from_bytes(lines[index]));
+#else
+                m_input_lines.insert(m_input_lines.end(), lines[index]);
+#endif
+            }
+        }
+    }
+    MoveCursor(CursorLocation::EditingCursor, CursorLocation::BlockEnd);
+    fprintf(m_output_file, "\n");
+    m_editor_status = EditorStatus::Complete;
+    return CC_NEWLINE;
+}
+
+unsigned char
+Editline::DeleteNextCharCommand(int ch)
+{
     LineInfoW * info = const_cast<LineInfoW *>(el_wline (m_editline));
     
     // Just delete the next character normally if possible

 }
 
 unsigned char
-Editline::FixIndentationCommand (int ch)
+Editline::PreviousHistoryCommand(int ch)
 {
+    SaveEditedLine();
+
+    return RecallHistory(true);
+}
+
+unsigned char
+Editline::NextHistoryCommand(int ch)
+{
+    SaveEditedLine();
+
+    return RecallHistory(false);
+}
+
+unsigned char
+Editline::FixIndentationCommand(int ch)
+{
     if (!m_fix_indentation_callback)
         return CC_NORM;
 

     el_wset(m_editline, EL_ADDFN, EditLineConstString("lldb-break-line"), EditLineConstString("Insert a line break"),
             (EditlineCommandCallbackType)(
                 [](EditLine *editline, int ch) { return Editline::InstanceFor(editline)->BreakLineCommand(ch); }));
+    el_wset(m_editline, EL_ADDFN, EditLineConstString("lldb-end-or-add-line"),
+            EditLineConstString("End editing or continue when incomplete"),
+            (EditlineCommandCallbackType)(
+                [](EditLine *editline, int ch) { return Editline::InstanceFor(editline)->EndOrAddLineCommand(ch); }));
     el_wset(m_editline, EL_ADDFN, EditLineConstString("lldb-delete-next-char"),
             EditLineConstString("Delete next character"), (EditlineCommandCallbackType)([](EditLine *editline, int ch) {
                 return Editline::InstanceFor(editline)->DeleteNextCharCommand(ch);

     el_wset(m_editline, EL_ADDFN, EditLineConstString("lldb-next-line"), EditLineConstString("Move to next line"),
             (EditlineCommandCallbackType)(
                 [](EditLine *editline, int ch) { return Editline::InstanceFor(editline)->NextLineCommand(ch); }));
+    el_wset(m_editline, EL_ADDFN, EditLineConstString("lldb-previous-history"),
+            EditLineConstString("Move to previous history"),
+            (EditlineCommandCallbackType)([](EditLine *editline, int ch) {
+                return Editline::InstanceFor(editline)->PreviousHistoryCommand(ch);
+            }));
+    el_wset(m_editline, EL_ADDFN, EditLineConstString("lldb-next-history"), EditLineConstString("Move to next history"),
+            (EditlineCommandCallbackType)(
+                [](EditLine *editline, int ch) { return Editline::InstanceFor(editline)->NextHistoryCommand(ch); }));
     el_wset(m_editline, EL_ADDFN, EditLineConstString("lldb-buffer-start"),
             EditLineConstString("Move to start of buffer"),
             (EditlineCommandCallbackType)(

     // Multi-line editor bindings
     if (multiline)
     {
-        el_set (m_editline, EL_BIND, "\n", "lldb-break-line", NULL);
-        el_set (m_editline, EL_BIND, "\r", "lldb-break-line", NULL);
+        el_set(m_editline, EL_BIND, "\n", "lldb-end-or-add-line", NULL);
+        el_set(m_editline, EL_BIND, "\r", "lldb-end-or-add-line", NULL);
+        el_set(m_editline, EL_BIND, ESCAPE "\n", "lldb-break-line", NULL);
+        el_set(m_editline, EL_BIND, ESCAPE "\r", "lldb-break-line", NULL);
         el_set (m_editline, EL_BIND, "^p", "lldb-previous-line", NULL);
         el_set (m_editline, EL_BIND, "^n", "lldb-next-line", NULL);
         el_set (m_editline, EL_BIND, "^?", "lldb-delete-previous-char", NULL);

             el_set (m_editline, EL_BIND, ESCAPE ">", "lldb-buffer-end", NULL);
             el_set (m_editline, EL_BIND, ESCAPE "[A", "lldb-previous-line", NULL);
             el_set (m_editline, EL_BIND, ESCAPE "[B", "lldb-next-line", NULL);
+            el_set(m_editline, EL_BIND, ESCAPE ESCAPE "[A", "lldb-previous-history", NULL);
+            el_set(m_editline, EL_BIND, ESCAPE ESCAPE "[B", "lldb-next-history", NULL);
+            el_set(m_editline, EL_BIND, ESCAPE "[1;3A", "lldb-previous-history", NULL);
+            el_set(m_editline, EL_BIND, ESCAPE "[1;3B", "lldb-next-history", NULL);
         }
         else
         {