Further work on keyboard navigating the completion list

7 files changed, 230 insertions, 76 deletions

diff --git a/Makefile.in b/Makefile.in
index c016d9f7..b67ffac0 100644
--- a/Makefile.in
+++ b/Makefile.in
@@ -91,7 +91,8 @@ FISH_OBJS := function.o builtin.o complete.o env.o exec.o expand.o		\
 	env_universal.o env_universal_common.o input_common.o event.o		\
 	signal.o io.o parse_util.o common.o screen.o path.o autoload.o		\
 	parser_keywords.o iothread.o color.o postfork.o	\
-	builtin_test.o parse_tree.o parse_productions.o parse_execution.cpp
+	builtin_test.o parse_tree.o parse_productions.o parse_execution.cpp \
+	pager.cpp
 
 FISH_INDENT_OBJS := fish_indent.o print_help.o common.o	\
 parser_keywords.o wutil.o tokenizer.o
diff --git a/common.h b/common.h
index f789b749..2a96b749 100644
--- a/common.h
+++ b/common.h
@@ -88,14 +88,33 @@ enum
 typedef unsigned int escape_flags_t;
 
 /* Directions */
-enum cardinal_direction_t
+enum selection_direction_t
 {
+    /* visual directions */
     direction_north,
     direction_east,
     direction_south,
-    direction_west
+    direction_west,
+    
+    /* logical directions */
+    direction_next,
+    direction_prev
 };
 
+inline bool selection_direction_is_cardinal(selection_direction_t dir)
+{
+    switch (dir)
+    {
+        case direction_north:
+        case direction_east:
+        case direction_south:
+        case direction_west:
+            return true;
+        default:
+            return false;
+    }
+}
+
 /**
  Helper macro for errors
  */
diff --git a/fish.cpp b/fish.cpp
index 77686195..bf6470f8 100644
--- a/fish.cpp
+++ b/fish.cpp
@@ -182,7 +182,7 @@ static struct config_paths_t determine_config_directory_paths(const char *argv0)
             {
                 wcstring base_path = str2wcstring(exec_path);
                 base_path.resize(base_path.size() - strlen(suffix));
-
+                
                 paths.data = base_path + L"/share/fish";
                 paths.sysconf = base_path + L"/etc/fish";
                 paths.doc = base_path + L"/share/doc/fish";
diff --git a/pager.cpp b/pager.cpp
index ebf5fc93..4093cca0 100644
--- a/pager.cpp
+++ b/pager.cpp
@@ -59,6 +59,8 @@
 #include "print_help.h"
 #include "highlight.h"
 
+#define PAGER_SELECTION_NONE ((size_t)(-1))
+
 typedef pager_t::comp_t comp_t;
 typedef std::vector<completion_t> completion_list_t;
 typedef std::vector<comp_t> comp_info_list_t;
@@ -365,7 +367,9 @@ void pager_t::completion_print(int cols, int *width_per_column, int row_start, i
 {
 
     size_t rows = (lst.size()-1)/cols+1;
-
+    
+    size_t effective_selected_idx = this->saturated_selected_completion_index();
+    
     for (size_t row = row_start; row < row_stop; row++)
     {
         for (size_t col = 0; col < cols; col++)
@@ -377,7 +381,7 @@ void pager_t::completion_print(int cols, int *width_per_column, int row_start, i
 
             size_t idx = col * rows + row;
             const comp_t *el = &lst.at(idx);
-            bool is_selected = (idx == this->selected_completion_idx);
+            bool is_selected = (idx == effective_selected_idx);
 
             /* Print this completion on its own "line" */
             line_t line = completion_print_item(prefix, el, row, col, width_per_column[col] - (is_last?0:2), row%2, is_selected, rendering);
@@ -873,7 +877,7 @@ page_rendering_t pager_t::render() const
     page_rendering_t rendering;
     rendering.term_width = this->term_width;
     rendering.term_height = this->term_height;
-    rendering.selected_completion_idx = this->selected_completion_idx;
+    rendering.selected_completion_idx = this->saturated_selected_completion_index();
     
     if (! this->empty())
     {
@@ -913,7 +917,7 @@ page_rendering_t pager_t::render() const
 
 void pager_t::update_rendering(page_rendering_t *rendering) const
 {
-    if (rendering->term_width != this->term_width || rendering->term_height != this->term_height || rendering->selected_completion_idx != this->selected_completion_idx)
+    if (rendering->term_width != this->term_width || rendering->term_height != this->term_height || rendering->selected_completion_idx != this->saturated_selected_completion_index())
     {
         *rendering = this->render();
     }
@@ -928,66 +932,177 @@ bool pager_t::empty() const
     return completions.empty();
 }
 
-void pager_t::set_selected_completion(size_t idx)
-{
-    this->selected_completion_idx = idx;
-}
-
-bool pager_t::select_next_completion_in_direction(cardinal_direction_t direction, const page_rendering_t &rendering)
+const completion_t *pager_t::select_next_completion_in_direction(selection_direction_t direction, const page_rendering_t &rendering)
 {
+    /* Must have something to select */
+    if (completions.empty())
+    {
+        return NULL;
+    }
+    
     /* Handle the case of nothing selected yet */
-    if (selected_completion_idx == (size_t)(-1))
+    if (selected_completion_idx == PAGER_SELECTION_NONE)
     {
-        return false;
+        if (selection_direction_is_cardinal(direction))
+        {
+            /* Cardinal directions do nothing unless something is selected */
+            return NULL;
+        }
+        else
+        {
+            /* Forward/backward do something sane */
+            selected_completion_idx = (direction == direction_next ? 0 : completions.size() - 1);
+            return selected_completion();
+        }
     }
- 
-    /* We have a completion index; we wish to compute its row and column. Completions are rendered column first, i.e. we go south before we go west. */
-    size_t current_row = selected_completion_idx % rendering.rows;
-    size_t current_col = selected_completion_idx / rendering.rows;
     
-    switch (direction)
+    /* Ok, we had something selected already. Select something different. */
+    size_t new_selected_completion_idx = selected_completion_idx;
+    if (! selection_direction_is_cardinal(direction))
     {
-        case direction_north:
+        /* Next / previous, easy */
+        if (direction == direction_next)
         {
-            /* Go up a whole row */
-            if (current_row > 0)
-                current_row--;
-            break;
+            new_selected_completion_idx = selected_completion_idx + 1;
+            if (new_selected_completion_idx > completion_infos.size())
+            {
+                new_selected_completion_idx = 0;
+            }
         }
-            
-        case direction_south:
+        else if (direction == direction_prev)
         {
-            /* Go down, unless we are in the last row. Note that this means that we may set selected_completion_idx to an out-of-bounds value if the last row is incomplete; this is a feature (it allows "last column memory"). */
-            if (current_row + 1 < rendering.rows)
-                current_row++;
-            break;
+            if (selected_completion_idx == 0)
+            {
+                new_selected_completion_idx = completion_infos.size() - 1;
+            }
+            else
+            {
+                new_selected_completion_idx = selected_completion_idx - 1;
+            }
         }
-        
-        case direction_east:
+        else
         {
-            if (current_col + 1 < rendering.cols)
-                current_col++;
-            break;
+            assert(0 && "Unknown non-cardinal direction");
         }
+    }
+    else
+    {
+        /* Cardinal directions. We have a completion index; we wish to compute its row and column. Completions are rendered column first, i.e. we go south before we go west. */
+        size_t current_row = selected_completion_idx % rendering.rows;
+        size_t current_col = selected_completion_idx / rendering.rows;
         
-        case direction_west:
+        switch (direction)
         {
-            if (current_col > 0)
-                current_col--;
-            break;
+            case direction_north:
+            {
+                /* Go up a whole row. If we cycle, go to the previous column. */
+                if (current_row > 0)
+                {
+                    current_row--;
+                }
+                else
+                {
+                    current_row = rendering.rows - 1;
+                    if (current_col > 0)
+                        current_col--;
+                }
+                break;
+            }
+                
+            case direction_south:
+            {
+                /* Go down, unless we are in the last row. Note that this means that we may set selected_completion_idx to an out-of-bounds value if the last row is incomplete; this is a feature (it allows "last column memory"). */
+                if (current_row + 1 < rendering.rows)
+                {
+                    current_row++;
+                }
+                else
+                {
+                    current_row = 0;
+                    if (current_col + 1 < rendering.cols)
+                        current_col++;
+                    
+                }
+                break;
+            }
+            
+            case direction_east:
+            {
+                /* Go east, wrapping to the next row */
+                if (current_col + 1 < rendering.cols)
+                {
+                    current_col++;
+                }
+                else
+                {
+                    current_col = 0;
+                    if (current_row + 1 < rendering.rows)
+                        current_row++;
+                }
+                break;
+            }
+            
+            case direction_west:
+            {
+                /* Go west, wrapping to the previous row */
+                if (current_col > 0)
+                {
+                    current_col--;
+                }
+                else
+                {
+                    current_col = rendering.cols - 1;
+                    if (current_row > 0)
+                        current_row--;
+                }
+                break;
+            }
+            
+            default:
+                assert(0 && "Unknown cardinal direction");
+                break;
         }
+        
+        /* Compute the new index based on the changed row */
+        new_selected_completion_idx = current_col * rendering.rows + current_row;
     }
     
-    size_t new_selected_completion_idx = current_col * rendering.rows + current_row;
     if (new_selected_completion_idx != selected_completion_idx)
     {
         selected_completion_idx = new_selected_completion_idx;
-        return true;
+        return selected_completion();
     }
     else
     {
-        return false;
+        return NULL;
+    }
+}
+
+size_t pager_t::saturated_selected_completion_index() const
+{
+    size_t result = selected_completion_idx;
+    if (result != PAGER_SELECTION_NONE && result >= completions.size())
+    {
+        result = completions.size() - 1;
     }
+    assert(result == PAGER_SELECTION_NONE || result < completions.size());
+    return result;
+}
+
+void pager_t::set_selected_completion_index(size_t completion_idx)
+{
+    selected_completion_idx = completion_idx;
+}
+
+const completion_t *pager_t::selected_completion() const
+{
+    const completion_t * result = NULL;
+    size_t idx = saturated_selected_completion_index();
+    if (idx != PAGER_SELECTION_NONE)
+    {
+        result = &completions.at(idx);
+    }
+    return result;
 }
 
 void pager_t::clear()
diff --git a/pager.h b/pager.h
index e53e919c..b3561d38 100644
--- a/pager.h
+++ b/pager.h
@@ -32,6 +32,9 @@ class pager_t
     
     size_t selected_completion_idx;
     
+    /* Returns the selected completion index, but not to exceed completions.size() */
+    size_t saturated_selected_completion_index() const;
+    
     /** Data structure describing one or a group of related completions */
     public:
     struct comp_t
@@ -86,10 +89,13 @@ class pager_t
     void set_term_size(int w, int h);
     
     /* Sets the index of the selected completion */
-    void set_selected_completion(size_t completion_idx);
+    void set_selected_completion_index(size_t completion_idx);
     
     /* Changes the selected completion in the given direction according to the layout of the given rendering. Returns true if the values changed. */
-    bool select_next_completion_in_direction(cardinal_direction_t direction, const page_rendering_t &rendering);
+    const completion_t *select_next_completion_in_direction(selection_direction_t direction, const page_rendering_t &rendering);
+    
+    /* Returns the currently selected completion */
+    const completion_t *selected_completion() const;
     
     /* Produces a rendering of the completions, at the given term size */
     page_rendering_t render() const;
diff --git a/reader.cpp b/reader.cpp
index 8c6d484e..f01c9207 100644
--- a/reader.cpp
+++ b/reader.cpp
@@ -1525,6 +1525,28 @@ static void accept_autosuggestion(bool full)
     }
 }
 
+static bool is_navigating_pager_contents()
+{
+    return data && data->current_pager.selected_completion() != NULL;
+}
+
+static void select_completion_in_direction(enum selection_direction_t dir, const wcstring &cycle_command_line, size_t cycle_cursor_pos)
+{
+    const completion_t *next_comp = data->current_pager.select_next_completion_in_direction(dir, data->current_page_rendering);
+    if (next_comp != NULL)
+    {
+        size_t cursor_pos = cycle_cursor_pos;
+        const wcstring new_cmd_line = completion_apply_to_command_line(next_comp->completion, next_comp->flags, cycle_command_line, &cursor_pos, false);
+        reader_set_buffer(new_cmd_line, cursor_pos);
+
+        /* Since we just inserted a completion, don't immediately do a new autosuggestion */
+        data->suppress_autosuggestion = true;
+
+        /* Trigger repaint (see #765) */
+        reader_repaint();
+    }
+}
+
 /**
   Flash the screen. This function only changed the color of the
   current line, since the flash_screen sequnce is rather painful to
@@ -2975,9 +2997,6 @@ const wchar_t *reader_readline(void)
 
     /* The cycle index in our completion list */
     size_t completion_cycle_idx = (size_t)(-1);
-    
-    /* Indicates if we are currently navigating pager contents */
-    bool is_navigating_pager_contents = false;
 
     /* The command line before completion */
     wcstring cycle_command_line;
@@ -3153,25 +3172,8 @@ const wchar_t *reader_readline(void)
 
                 if (! comp_empty && last_char == R_COMPLETE)
                 {
-                    /* The user typed R_COMPLETE more than once in a row. Cycle through our available completions */
-                    const completion_t *next_comp = cycle_competions(comp, cycle_command_line, &completion_cycle_idx);
-                    
-                    data->current_pager.set_selected_completion(completion_cycle_idx);
-                    
-                    if (next_comp != NULL)
-                    {
-                        is_navigating_pager_contents = true;
-                    
-                        size_t cursor_pos = cycle_cursor_pos;
-                        const wcstring new_cmd_line = completion_apply_to_command_line(next_comp->completion, next_comp->flags, cycle_command_line, &cursor_pos, false);
-                        reader_set_buffer(new_cmd_line, cursor_pos);
-
-                        /* Since we just inserted a completion, don't immediately do a new autosuggestion */
-                        data->suppress_autosuggestion = true;
-
-                        /* Trigger repaint (see #765) */
-                        reader_repaint_if_needed();
-                    }
+                    /* The user typed R_COMPLETE more than once in a row. Cycle through our available completions. */
+                    select_completion_in_direction(direction_next, cycle_command_line, cycle_cursor_pos);
                 }
                 else
                 {
@@ -3555,7 +3557,11 @@ const wchar_t *reader_readline(void)
             /* Move left*/
             case R_BACKWARD_CHAR:
             {
-                if (data->buff_pos > 0)
+                if (is_navigating_pager_contents())
+                {
+                    select_completion_in_direction(direction_west, cycle_command_line, cycle_cursor_pos);
+                }
+                else if (data->buff_pos > 0)
                 {
                     data->buff_pos--;
                     reader_repaint();
@@ -3566,7 +3572,11 @@ const wchar_t *reader_readline(void)
             /* Move right*/
             case R_FORWARD_CHAR:
             {
-                if (data->buff_pos < data->command_length())
+                if (is_navigating_pager_contents())
+                {
+                    select_completion_in_direction(direction_east, cycle_command_line, cycle_cursor_pos);
+                }
+                else if (data->buff_pos < data->command_length())
                 {
                     data->buff_pos++;
                     reader_repaint();
@@ -3638,12 +3648,9 @@ const wchar_t *reader_readline(void)
             case R_UP_LINE:
             case R_DOWN_LINE:
             {
-                if (is_navigating_pager_contents)
+                if (is_navigating_pager_contents())
                 {
-                    if (data->current_pager.select_next_completion_in_direction(c == R_UP_LINE ? direction_north : direction_south, data->current_page_rendering))
-                    {
-                        reader_repaint();
-                    }
+                    select_completion_in_direction(c == R_UP_LINE ? direction_north : direction_south, cycle_command_line, cycle_cursor_pos);
                 }
                 else
                 {
@@ -3882,7 +3889,7 @@ int reader_has_pager_contents()
         return -1;
     }
 
-    return data->current_page_rendering.screen_data.empty() ? 1 : 0;
+    return ! data->current_page_rendering.screen_data.empty();
 }
 
 
diff --git a/share/functions/up-or-search.fish b/share/functions/up-or-search.fish
index fc51d710..98179ad7 100644
--- a/share/functions/up-or-search.fish
+++ b/share/functions/up-or-search.fish
@@ -5,6 +5,12 @@ function up-or-search -d "Depending on cursor position and current mode, either
 		return
 	end
 
+	# If we are navigating the pager, then up always navigates
+	if commandline --paging-mode
+		commandline -f up-line
+		return
+	end
+
 	# We are not already in search mode.
 	# If we are on the top line, start search mode,
 	# otherwise move up