Closed Bug 1463676 Opened Last year Closed Last year

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

Categories

(DevTools :: Console, defect, P1)

defect

Tracking

(firefox63 fixed)

RESOLVED FIXED
Firefox 63
Tracking Status
firefox63 --- fixed

People

(Reporter: nchevobbe, Assigned: Honza)

References

(Blocks 1 open bug)

Details

(Whiteboard: [boogaloo-mvp])

Attachments

(1 file)

**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.
Blocks: 1458831
Priority: -- → P3
Priority: P3 → P2
Whiteboard: [boogaloo-mvp]
Assignee: nobody → odvarko
Status: NEW → ASSIGNED
Priority: P2 → P1
Product: Firefox → DevTools
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 ?
(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 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
Attachment #8990294 - Flags: review?(nchevobbe) → review+
(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
Pushed by jodvarko@mozilla.com:
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
https://hg.mozilla.org/mozilla-central/rev/478ac2a1618f
Status: ASSIGNED → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → Firefox 63
Duplicate of this bug: 1510205
You need to log in before you can comment on or make changes to this bug.