[shell] Hue shells cannot recall command history

Review Request #2082 - Created May 16, 2012 and submitted

Romain Rigaux
old-hue-rw
hue
bcwalrus
commit cf7cdb05454708c71ebc793f632e5c9af667e2f4
Author: Romain Rigaux <romain@cloudera.com>
Date:   Wed May 16 15:07:16 2012 -0700

    [shell] Hue shells cannot recall command history
    
    Go back up/down the history
    Restore the initial input data if no history command was selected

:100644 100644 78853b2... dc85cf3... M	apps/shell/src/shell/templates/index.mako
Manual testing in Chrome/FF.
Unit tests.
  • 6
  • 0
  • 2
  • 0
  • 8
Description From Last Updated
Are we storing unlimited history? bc Wong
I don't like this indexing scheme. Why isn't currentCommandIndex pointing to the command that is displayed? By default, currentCommandIndex should ... bc Wong
To have it point past the history: currentCommandIndex = previousCommands.length bc Wong
The purpose of this block is to record the `input'. How about simply: if (currentCommandIndex == previousCommands.length) { temporaryInput = ... bc Wong
if (currentCommandIndex < previousCommands.length) ... bc Wong
You can simply do: else { command = temporaryInput; } bc Wong
  1. 
      
    1. I can't upload the new diff, here is the new version while I try to do it.
      
      
              var history = (function() {
                  // The initial input ("enter" key not pressed yet before navigating in the history) is
                  // pushed as a new command into the history in order to simplify the logic.
                  var previousCommands = [];
                  var currentCommandIndex = -1;
      
                  return {
                      recordCommand: function(command) {
                          if (command) {
                              if (previousCommands[previousCommands.length - 1] != command) {
                                  previousCommands.push(command);
                                  currentCommandIndex = previousCommands.length - 1;
                              }
                          }
                      },
                      getPreviousCommand: function(input) {
                          // If first getPreviousCommand call, backup the current shell input
                          if (currentCommandIndex == previousCommands.length - 1) {
                              previousCommands.push(input);
                              currentCommandIndex = previousCommands.length - 1;
                          }
      
                          var command = null;
                          if (previousCommands[currentCommandIndex - 1]) {
                              if (currentCommandIndex > 0) {
                                  currentCommandIndex--;
                              }
                              command = previousCommands[currentCommandIndex];                        
                          }
                          return command;
                      },
                      getNextCommand: function() {
                          if (previousCommands[currentCommandIndex + 1] != null) {
                              currentCommandIndex++;
                              command = previousCommands[currentCommandIndex];
      
                              // If last getNextCommand call, remove original shell input from history
                              if (currentCommandIndex == previousCommands.length - 1) {                            
                                  previousCommands.pop();
                                  currentCommandIndex = previousCommands.length - 1;
                              }                        
                          }
                          return command;
                      }
                  };
              })();
  2. Are we storing unlimited history?
    1. Currently yes.
      
      We could keep it like that (we don't support the restore session) for now? (I don't think it would blow up)
  3. apps/shell/src/shell/templates/index.mako (Diff revision 1)
     
     
     
     
     
    Fancy :-)
  4. apps/shell/src/shell/templates/index.mako (Diff revision 1)
     
     
     
    I don't like this indexing scheme. Why isn't currentCommandIndex pointing to the command that is displayed?
    
    By default, currentCommandIndex should be previousCommands.length. When you do getPreviousCommand(), you check if (index - 1) is in bound, and then return that command. When you do getNextCommand(), you check if (index + 1) is in bound, and return that command. This way, they're symmetrical.
    1. The command that is displayed might not be pushed to the history yet (e.g. when you type something and don't do <enter> but go back up in the history). The idea was to have the previousCommands list as the actual history and we only push() into the history and never manage any pop() (we just go up/down the index). I am trying to do the symmetrical logic but getNextCommand() and getPreviousCommand() need more logic.
      
      In this version we also don't need the two:
      if (oldCommand) {
        command = oldCommand;
      }
      
    2. > The command that is displayed might not be pushed to the history yet (e.g. when you type something and don't do <enter> but go back up in the history).
      
      I understand. And we shouldn't push that into the history. You're saving it in temporaryInput. Good. In getNextCommand(), if (index == history.length - 1), then you do index++ and show the temporaryInput.
      
      What's confusing me is that the index does not correspond to the historical item being displayed.
    3. Yes this is normal, as we use a list for the history and temporaryOutput variable the index will never point to the temporary output.
      
      In getNextCommand there are just two cases:
        - we are still in the history (we get the command and increment)
        - we went back to the top of the history (if (index == history.length - 1) it means we are out of the history and we return the temporary output (we just return it and don't any index++))
      
      I did a second version here but not sure it is simpler: https://review.cloudera.org/r/2089/
      
      
  5. 
      
  1. 
      
  2. Would initialize to 0. The currentCommandIndex should point beyond the history list, when it's not in historical mode.
  3. To have it point past the history:
      currentCommandIndex = previousCommands.length
  4. apps/shell/src/shell/templates/index.mako (Diff revision 3)
     
     
     
     
    We should always clear the temporaryInput.
  5. apps/shell/src/shell/templates/index.mako (Diff revision 3)
     
     
     
     
     
    The purpose of this block is to record the `input'. How about simply:
        if (currentCommandIndex == previousCommands.length) {
            temporaryInput = input;
        }
    1. Yes, this is much cleaner.
  6. apps/shell/src/shell/templates/index.mako (Diff revision 3)
     
     
     
     
    if (currentCommandIndex < previousCommands.length)
        ...
    1. This means we can go even if we are currently pointing to the last element of the history. As the index is 0 indexed we need the -1 for saying "go only if we have one more element" (as we do a index++ just after).
          
    2. You're right. This is correct:
                if (currentCommandIndex < previousCommands.length - 1) {
      
  7. apps/shell/src/shell/templates/index.mako (Diff revision 3)
     
     
     
     
     
    You can simply do:
        else {
            command = temporaryInput;
        }
    1. Cleaner indeed.
      
      I also added:
      
      } else {
         command = temporaryInput;
         currentCommandIndex = previousCommands.length; // need to reset index because of comment above
      }
    2. You also should also check if currentCommandIndex is pointing beyond the history size already. In that case, it's a no-op.
  8. 
      
  1. Ship It!
  2. 
      
Review request changed

Status: Closed (submitted)

Loading...