Bug 1465352 - Clean up history reducer state; r=nchevobbe
authorJan Odvarko <odvarko@gmail.com>
Thu, 19 Jul 2018 14:26:34 +0200
changeset 821107 2aca3a665afe56e1480c2358c8b9d3047cbbdc32
parent 821106 ab6c2651f8eff6b2846ee89732089c9ca62f6a13
child 821108 8cce0e7ad6f3f7e91afe13e97e04fc0f0ed400c6
push id117018
push userbmo:sfoster@mozilla.com
push dateSat, 21 Jul 2018 04:05:10 +0000
reviewersnchevobbe
bugs1465352
milestone63.0a1
Bug 1465352 - Clean up history reducer state; r=nchevobbe MozReview-Commit-ID: LgWQ5fe7WXJ
devtools/client/webconsole/actions/history.js
devtools/client/webconsole/components/JSTerm.js
devtools/client/webconsole/constants.js
devtools/client/webconsole/reducers/history.js
devtools/client/webconsole/selectors/history.js
devtools/client/webconsole/test/mochitest/browser_jsterm_history_persist.js
--- a/devtools/client/webconsole/actions/history.js
+++ b/devtools/client/webconsole/actions/history.js
@@ -5,17 +5,17 @@
  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
 
 "use strict";
 
 const {
   APPEND_TO_HISTORY,
   CLEAR_HISTORY,
   HISTORY_LOADED,
-  UPDATE_HISTORY_PLACEHOLDER,
+  UPDATE_HISTORY_POSITION,
 } = require("devtools/client/webconsole/constants");
 
 /**
  * Append a new value in the history of executed expressions,
  * or overwrite the most recent entry. The most recent entry may
  * contain the last edited input value that was not evaluated yet.
  */
 function appendToHistory(expression) {
@@ -44,22 +44,22 @@ function historyLoaded(entries) {
     type: HISTORY_LOADED,
     entries,
   };
 }
 
 /**
  * Update place-holder position in the history list.
  */
-function updatePlaceHolder(direction, expression) {
+function updateHistoryPosition(direction, expression) {
   return {
-    type: UPDATE_HISTORY_PLACEHOLDER,
+    type: UPDATE_HISTORY_POSITION,
     direction,
     expression,
   };
 }
 
 module.exports = {
   appendToHistory,
   clearHistory,
   historyLoaded,
-  updatePlaceHolder,
+  updateHistoryPosition,
 };
--- a/devtools/client/webconsole/components/JSTerm.js
+++ b/devtools/client/webconsole/components/JSTerm.js
@@ -71,17 +71,17 @@ class JSTerm extends Component {
       // Console object.
       hud: PropTypes.object.isRequired,
       // Needed for opening context menu
       serviceContainer: PropTypes.object.isRequired,
       // Handler for clipboard 'paste' event (also used for 'drop' event, callback).
       onPaste: PropTypes.func,
       codeMirrorEnabled: PropTypes.bool,
       // Update position in the history after executing an expression (action).
-      updatePlaceHolder: PropTypes.func.isRequired,
+      updateHistoryPosition: PropTypes.func.isRequired,
     };
   }
 
   constructor(props) {
     super(props);
 
     const {
       hud,
@@ -954,27 +954,27 @@ class JSTerm extends Component {
    *        History navigation direction: HISTORY_BACK or HISTORY_FORWARD.
    *
    * @returns boolean
    *          True if the input value changed, false otherwise.
    */
   historyPeruse(direction) {
     const {
       history,
-      updatePlaceHolder,
+      updateHistoryPosition,
       getValueFromHistory,
     } = this.props;
 
     if (!history.entries.length) {
       return false;
     }
 
     const newInputValue = getValueFromHistory(direction);
     const expression = this.getInputValue();
-    updatePlaceHolder(direction, expression);
+    updateHistoryPosition(direction, expression);
 
     if (newInputValue != null) {
       this.setInputValue(newInputValue);
       return true;
     }
 
     return false;
   }
@@ -1574,14 +1574,14 @@ function mapStateToProps(state) {
     getValueFromHistory: (direction) => getHistoryValue(state, direction),
   };
 }
 
 function mapDispatchToProps(dispatch) {
   return {
     appendToHistory: (expr) => dispatch(historyActions.appendToHistory(expr)),
     clearHistory: () => dispatch(historyActions.clearHistory()),
-    updatePlaceHolder: (direction, expression) =>
-      dispatch(historyActions.updatePlaceHolder(direction, expression)),
+    updateHistoryPosition: (direction, expression) =>
+      dispatch(historyActions.updateHistoryPosition(direction, expression)),
   };
 }
 
 module.exports = connect(mapStateToProps, mapDispatchToProps)(JSTerm);
--- a/devtools/client/webconsole/constants.js
+++ b/devtools/client/webconsole/constants.js
@@ -28,17 +28,17 @@ const actionTypes = {
   SHOW_OBJECT_IN_SIDEBAR: "SHOW_OBJECT_IN_SIDEBAR",
   TIMESTAMPS_TOGGLE: "TIMESTAMPS_TOGGLE",
   APPEND_NOTIFICATION: "APPEND_NOTIFICATION",
   REMOVE_NOTIFICATION: "REMOVE_NOTIFICATION",
   SPLIT_CONSOLE_CLOSE_BUTTON_TOGGLE: "SPLIT_CONSOLE_CLOSE_BUTTON_TOGGLE",
   APPEND_TO_HISTORY: "APPEND_TO_HISTORY",
   CLEAR_HISTORY: "CLEAR_HISTORY",
   HISTORY_LOADED: "HISTORY_LOADED",
-  UPDATE_HISTORY_PLACEHOLDER: "UPDATE_HISTORY_PLACEHOLDER",
+  UPDATE_HISTORY_POSITION: "UPDATE_HISTORY_POSITION",
 };
 
 const prefs = {
   PREFS: {
     // Filter preferences only have the suffix since they can be used either for the
     // webconsole or the browser console.
     FILTER: {
       ERROR: "filter.error",
--- a/devtools/client/webconsole/reducers/history.js
+++ b/devtools/client/webconsole/reducers/history.js
@@ -4,78 +4,75 @@
  * License, v. 2.0. If a copy of the MPL was not distributed with this
  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
 "use strict";
 
 const {
   APPEND_TO_HISTORY,
   CLEAR_HISTORY,
   HISTORY_LOADED,
-  UPDATE_HISTORY_PLACEHOLDER,
+  UPDATE_HISTORY_POSITION,
   HISTORY_BACK,
   HISTORY_FORWARD,
 } = require("devtools/client/webconsole/constants");
 
 /**
  * Create default initial state for this reducer.
  */
 function getInitialState() {
   return {
     // Array with history entries
     entries: [],
 
-    // Holds the index of the history entry that the user is currently
-    // viewing. This is reset to this.history.length when APPEND_TO_HISTORY
-    // action is fired.
-    placeHolder: undefined,
+    // Holds position (index) in history entries that the user is
+    // currently viewing. This is reset to this.entries.length when
+    // APPEND_TO_HISTORY action is fired.
+    position: undefined,
 
-    // Holds the number of entries in history. This value is incremented
-    // when APPEND_TO_HISTORY action is fired and used to get previous
-    // value from the command line when the user goes backward.
-    index: 0,
+    // Backups the original user value (if any) that can be set in
+    // the input field. It might be used again if the user doesn't
+    // pick up anything from the history and wants to return all
+    // the way back to see the original input text.
+    originalUserValue: null,
   };
 }
 
 function history(state = getInitialState(), action, prefsState) {
   switch (action.type) {
     case APPEND_TO_HISTORY:
       return appendToHistory(state, prefsState, action.expression);
     case CLEAR_HISTORY:
       return clearHistory(state);
     case HISTORY_LOADED:
       return historyLoaded(state, action.entries);
-    case UPDATE_HISTORY_PLACEHOLDER:
-      return updatePlaceHolder(state, action.direction, action.expression);
+    case UPDATE_HISTORY_POSITION:
+      return updateHistoryPosition(state, action.direction, action.expression);
   }
   return state;
 }
 
 function appendToHistory(state, prefsState, expression) {
   // Clone state
   state = {...state};
   state.entries = [...state.entries];
 
   // Append new expression only if it isn't the same as
   // the one recently added.
-  // If it's the same don't forget to remove the current
-  // input value that has been appended in updatePlaceholder.
-  if (expression.trim() != state.entries[state.index - 1]) {
-    state.entries[state.index++] = expression;
-  } else if (state.index < state.entries.length) {
-    state.entries.pop();
+  if (expression.trim() != state.entries[state.entries.length - 1]) {
+    state.entries.push(expression);
   }
 
-  state.placeHolder = state.entries.length;
-
   // Remove entries if the limit is reached
   if (state.entries.length > prefsState.historyCount) {
     state.entries.splice(0, state.entries.length - prefsState.historyCount);
-    state.index = state.placeHolder = state.entries.length;
   }
 
+  state.position = state.entries.length;
+  state.originalUserValue = null;
+
   return state;
 }
 
 function clearHistory(state) {
   return getInitialState();
 }
 
 /**
@@ -86,49 +83,48 @@ function clearHistory(state) {
  * Loaded entries are appended before the ones that were
  * added to the state in this session.
  */
 function historyLoaded(state, entries) {
   const newEntries = [...entries, ...state.entries];
   return {
     ...state,
     entries: newEntries,
-    placeHolder: newEntries.length,
-    index: newEntries.length,
+    // Default position is at the end of the list
+    // (at the latest inserted item).
+    position: newEntries.length,
+    originalUserValue: null,
   };
 }
 
-function updatePlaceHolder(state, direction, expression) {
+function updateHistoryPosition(state, direction, expression) {
   // Handle UP arrow key => HISTORY_BACK
   // Handle DOWN arrow key => HISTORY_FORWARD
   if (direction == HISTORY_BACK) {
-    if (state.placeHolder <= 0) {
+    if (state.position <= 0) {
       return state;
     }
 
     // Clone state
     state = {...state};
 
-    // Save the current input value as the latest entry in history, only if
-    // the user is already at the last entry.
-    // Note: this code does not store changes to items that are already in
-    // history.
-    if (state.placeHolder == state.index) {
-      state.entries = [...state.entries];
-      state.entries[state.index] = expression || "";
+    // Store the current input value when the user starts
+    // browsing through the history.
+    if (state.position == state.entries.length) {
+      state.originalUserValue = expression || "";
     }
 
-    state.placeHolder--;
+    state.position--;
   } else if (direction == HISTORY_FORWARD) {
-    if (state.placeHolder >= (state.entries.length - 1)) {
+    if (state.position >= state.entries.length) {
       return state;
     }
 
     state = {
       ...state,
-      placeHolder: state.placeHolder + 1,
+      position: state.position + 1,
     };
   }
 
   return state;
 }
 
 exports.history = history;
--- a/devtools/client/webconsole/selectors/history.js
+++ b/devtools/client/webconsole/selectors/history.js
@@ -25,25 +25,28 @@ function getHistoryValue(state, directio
   }
   if (direction == HISTORY_FORWARD) {
     return getNextHistoryValue(state);
   }
   return null;
 }
 
 function getNextHistoryValue(state) {
-  if (state.history.placeHolder < (state.history.entries.length - 1)) {
-    return state.history.entries[state.history.placeHolder + 1];
+  if (state.history.position < (state.history.entries.length - 1)) {
+    return state.history.entries[state.history.position + 1];
   }
-  return null;
+
+  // The user didn't pick up anything from the history and returned
+  // back to the previous value (if any) that was in the input box.
+  return state.history.originalUserValue;
 }
 
 function getPreviousHistoryValue(state) {
-  if (state.history.placeHolder > 0) {
-    return state.history.entries[state.history.placeHolder - 1];
+  if (state.history.position > 0) {
+    return state.history.entries[state.history.position - 1];
   }
   return null;
 }
 
 module.exports = {
   getHistory,
   getHistoryEntries,
   getHistoryValue,
--- a/devtools/client/webconsole/test/mochitest/browser_jsterm_history_persist.js
+++ b/devtools/client/webconsole/test/mochitest/browser_jsterm_history_persist.js
@@ -49,18 +49,20 @@ async function testHistory() {
   let state2 = hud2.ui.consoleOutput.getStore().getState();
   is(JSON.stringify(getHistoryEntries(state2)),
      '["0","1","2","3","4","5","6","7","8","9"]',
      "Second tab has populated history");
   await testNavigatingHistoryInUI(hud2);
 
   state2 = hud2.ui.consoleOutput.getStore().getState();
   is(JSON.stringify(getHistoryEntries(state2)),
-     '["0","1","2","3","4","5","6","7","8","9",""]',
-     "An empty entry has been added in the second tab due to history perusal");
+    '["0","1","2","3","4","5","6","7","8","9"]',
+    "An empty entry has been added in the second tab due to history perusal");
+  is(state2.history.originalUserValue, "",
+     "An empty value has been stored as the current input value");
 
   // Third tab: Should have the same history as first tab, but if we run a
   // command, then the history of the first and second shouldn't be affected
   const hud3 = await openNewTabAndConsole(TEST_URI, false);
   let state3 = hud3.ui.consoleOutput.getStore().getState();
 
   is(JSON.stringify(getHistoryEntries(state3)),
      '["0","1","2","3","4","5","6","7","8","9"]',
@@ -73,18 +75,20 @@ async function testHistory() {
 
   state1 = hud1.ui.consoleOutput.getStore().getState();
   is(JSON.stringify(getHistoryEntries(state1)),
      '["0","1","2","3","4","5","6","7","8","9"]',
      "First tab history hasn't changed due to command in third tab");
 
   state2 = hud2.ui.consoleOutput.getStore().getState();
   is(JSON.stringify(getHistoryEntries(state2)),
-     '["0","1","2","3","4","5","6","7","8","9",""]',
-     "Second tab history hasn't changed due to command in third tab");
+    '["0","1","2","3","4","5","6","7","8","9"]',
+    "Second tab history hasn't changed due to command in third tab");
+  is(state2.history.originalUserValue, "",
+     "Current input value hasn't changed due to command in third tab");
 
   state3 = hud3.ui.consoleOutput.getStore().getState();
   is(JSON.stringify(getHistoryEntries(state3)),
      '["1","2","3","4","5","6","7","8","9","\\"hello from third tab\\""]',
      "Third tab has updated history (and purged the first result) after " +
      "running a command");
 
   // Fourth tab: Should have the latest command from the third tab, followed