1463676 - Don't add an evaluated input to the history if it is the same as the previous evaluation

Status: RESOLVED FIXED

(DevTools :: Console, defect, P1)

Description

[Nicolas Chevobbe [:nchevobbe]]

**Steps to reproduce**
1. Open the console
2. Evaluate "hello"
3. Evaluate "world"
4. Hit the Up-arrow key to see "world" in the input again
5. Hit enter to evaluate it
6. Hit the Up-arrow twice


**Expected results**

I see "hello" in the input

**Actual results**

I see "world" in the input

---

That's because we always add the evaluated inputs the the history. We should check if the evaluated input, trimmed, is equal to the previous one, and if so, don't add it to the history.

Comment 1

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Attachment: Bug 1463676 - Don't add an evaluated input to the history if it is the same as the previous evaluation;

Review commit: https://reviewboard.mozilla.org/r/255328/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/255328/

Comment 2

[Nicolas Chevobbe [:nchevobbe]]

Comment on attachment 8990294 [details]
Bug 1463676 - Don't add an evaluated input to the history if it is the same as the previous evaluation;

https://reviewboard.mozilla.org/r/255328/#review262412

::: devtools/client/webconsole/reducers/history.js:52
(Diff revision 1)
>    // Clone state
>    state = {...state};
>    state.entries = [...state.entries];
>  
> -  // Append new expression
> +  // 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;
> +    state.entries[state.index++] = expression;
> +  } else if (state.index < state.entries.length) {
> +    state.entries.pop();
> +  }

I would expect that we return early if `expression.trim() === state.entries[state.index - 1]`, so we don't need to clone the state nor the entries.

Is there something that prevent us to do that ?

::: devtools/client/webconsole/test/mochitest/browser_jsterm_add_edited_input_to_history.js:57
(Diff revision 1)
> +  jsterm.setInputValue('"second item"');
> +  await jsterm.execute();

I think we can directly pass the input to execute `await jsterm.execute('"second item"')`.

We could also execute another input with spaces so we know that we do trim the input before doing the comparison with the last history entry

```js
await jsterm.execute('"second item"')
await jsterm.execute('  "second item"    ')
```

::: devtools/client/webconsole/test/mochitest/browser_jsterm_add_edited_input_to_history.js:62
(Diff revision 1)
> +  jsterm.setInputValue('"second item"');
> +  await jsterm.execute();
> +  EventUtils.synthesizeKey("KEY_ArrowUp");
> +  is(jsterm.getInputValue(), '"second item"', "test history up");
> +  EventUtils.synthesizeKey("KEY_ArrowUp");
> +  is(jsterm.getInputValue(), '"first item"', "test history up");

nit: say that a repeated input did not led to a duplicated history entry maybe ?

Comment 3

[Jan Honza Odvarko [:Honza] (always need-info? me)]

(In reply to Nicolas Chevobbe [:nchevobbe] from comment #2)
> Comment on attachment 8990294 [details]
> Bug 1463676 - Don't add an evaluated input to the history if it is the same
> as the previous evaluation;
> 
> https://reviewboard.mozilla.org/r/255328/#review262412
> 
> ::: devtools/client/webconsole/reducers/history.js:52
> (Diff revision 1)
> >    // Clone state
> >    state = {...state};
> >    state.entries = [...state.entries];
> >  
> > -  // Append new expression
> > +  // 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;
> > +    state.entries[state.index++] = expression;
> > +  } else if (state.index < state.entries.length) {
> > +    state.entries.pop();
> > +  }
> 
> I would expect that we return early if `expression.trim() ===
> state.entries[state.index - 1]`, so we don't need to clone the state nor the
> entries.
> 
> Is there something that prevent us to do that ?
Yes, we need to update the `placeHolder` field.

It's this line:
state.placeHolder = state.entries.length;


> :::
> devtools/client/webconsole/test/mochitest/
> browser_jsterm_add_edited_input_to_history.js:57
> (Diff revision 1)
> > +  jsterm.setInputValue('"second item"');
> > +  await jsterm.execute();
> 
> I think we can directly pass the input to execute `await
> jsterm.execute('"second item"')`.

> 
> We could also execute another input with spaces so we know that we do trim
> the input before doing the comparison with the last history entry
> 
> ```js
> await jsterm.execute('"second item"')
> await jsterm.execute('  "second item"    ')
> ```
Done

> :::
> devtools/client/webconsole/test/mochitest/
> browser_jsterm_add_edited_input_to_history.js:62
> (Diff revision 1)
> > +  jsterm.setInputValue('"second item"');
> > +  await jsterm.execute();
> > +  EventUtils.synthesizeKey("KEY_ArrowUp");
> > +  is(jsterm.getInputValue(), '"second item"', "test history up");
> > +  EventUtils.synthesizeKey("KEY_ArrowUp");
> > +  is(jsterm.getInputValue(), '"first item"', "test history up");
> 
> nit: say that a repeated input did not led to a duplicated history entry
> maybe ?
Done

Thanks Nicolas!
Honza

Comment 4

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Comment on attachment 8990294 [details]
Bug 1463676 - Don't add an evaluated input to the history if it is the same as the previous evaluation;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/255328/diff/1-2/

Comment 5

[Nicolas Chevobbe [:nchevobbe]]

Comment on attachment 8990294 [details]
Bug 1463676 - Don't add an evaluated input to the history if it is the same as the previous evaluation;

https://reviewboard.mozilla.org/r/255328/#review262450

Thanks Honza !

::: devtools/client/webconsole/test/mochitest/browser_jsterm_add_edited_input_to_history.js:57
(Diff revision 2)
>    EventUtils.synthesizeKey("KEY_ArrowDown");
>    is(jsterm.getInputValue(), '"editing input 2"',
>       "test history down restores new in-progress input again");
> +
> +  // Appending the same value again should not impact the history.
> +  // Let's also use same spaces around to check that the input value

nit: s/same spaces/some spaces

Comment 6

[Jan Honza Odvarko [:Honza] (always need-info? me)]

(In reply to Nicolas Chevobbe [:nchevobbe] from comment #5)
> Comment on attachment 8990294 [details]
> Bug 1463676 - Don't add an evaluated input to the history if it is the same
> as the previous evaluation;
> 
> https://reviewboard.mozilla.org/r/255328/#review262450
> 
> Thanks Honza !
> 
> :::
> devtools/client/webconsole/test/mochitest/
> browser_jsterm_add_edited_input_to_history.js:57
> (Diff revision 2)
> >    EventUtils.synthesizeKey("KEY_ArrowDown");
> >    is(jsterm.getInputValue(), '"editing input 2"',
> >       "test history down restores new in-progress input again");
> > +
> > +  // Appending the same value again should not impact the history.
> > +  // Let's also use same spaces around to check that the input value
> 
> nit: s/same spaces/some spaces
Fixed

Comment 7

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Comment on attachment 8990294 [details]
Bug 1463676 - Don't add an evaluated input to the history if it is the same as the previous evaluation;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/255328/diff/2-3/

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/478ac2a1618f
Don't add an evaluated input to the history if it is the same as the previous evaluation; r=nchevobbe

Comment 9

[Andrei Ciure[:aciure]]

https://hg.mozilla.org/mozilla-central/rev/478ac2a1618f