Bug 1465352 - Clean up history reducer state; r=nchevobbe
authorJan Odvarko <odvarko@gmail.com>
Thu, 19 Jul 2018 14:26:34 +0200
changeset 427458 2aca3a665afe56e1480c2358c8b9d3047cbbdc32
parent 427457 ab6c2651f8eff6b2846ee89732089c9ca62f6a13
child 427459 8cce0e7ad6f3f7e91afe13e97e04fc0f0ed400c6
push id34306
push usercsabou@mozilla.com
push dateFri, 20 Jul 2018 21:41:18 +0000
treeherdermozilla-central@d6a5e8aea651 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersnchevobbe
bugs1465352
milestone63.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
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