1465352 - Clean up history reducer state

Status: RESOLVED FIXED

(DevTools :: Console, enhancement, P1)

Description

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

This is follow up for bug 1462390

History reducer has three props:

- entries
- placeHolder
- index

It would be great if we can get rid of the 'index' property. If not it should be clearly described (and perhaps renamed) explaining what is it for.

Honza

Comment 1

[Kaviarasi]

I am a beginner who is yet to contribute to open source projects.
Shall I work on this bug ? If i can work on this bug, could anyone explain it more? 
I don't understand the issue completely.

Comment 2

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

Yes, you can!

Related code base is built on top of React/Redux, if you know these frameworks it's a big plus for fixing the issue. If not you have something new to learn ;-)

The issue is related to this state:
https://searchfox.org/mozilla-central/rev/292d295d6b084b43b70de26a42e68513bb7b36a3/devtools/client/webconsole/reducers/history.js#20-35

It stores entries executed through the command line in the Console panel. There are three props:
* entries
* placeHolder
* index

Try this STRs (steps to reproduce):

1) Load any page (e.g. google.com)
2) Open Firefox DevTools toolbox (F12 on Win)
3) Execute any expression on the command line, e.g. 1+1
4) Execute another expression e.g. 1+2
5) Press arrow up key to see previously executed expressions.

So, you can use keyboard up/down arrows to move in history of executed expressions and the state above stores necessary data for it.

Extended STRs 1:
6) Type something into the command line (do not execute)
7) Press arrow up -> you should see previous entry
8) Press arrow down -> you should see the non-executed expression

Extended STRs 2:
6) Make sure the command line is empty
7) Press arrow up -> you should see previous entry
8) Press arrow down -> the command line should be empty again

---

We think that the `index` property might be removed or (if not) nicely documented why it's there and/or/maybe renamed, so it corresponds more to its purpose.

The place that handles the movement in history (pressing up/down keys) is here:
https://searchfox.org/mozilla-central/rev/292d295d6b084b43b70de26a42e68513bb7b36a3/devtools/client/webconsole/components/JSTerm.js#871

But, mostly you need to analyze the reducer itself:
https://searchfox.org/mozilla-central/source/devtools/client/webconsole/reducers/history.js

You might use Browser Toolbox:
https://developer.mozilla.org/en-US/docs/Tools/Browser_Toolbox

Or you can use simple `console.log()` statements in the code and Browser Console to see what's happening in the code:
https://developer.mozilla.org/en-US/docs/Tools/Browser_Console

Should I assign the bug to you?

Please use the `Need more information from` field if you have any questions.

Thanks,
Honza

Comment 3

[Kaviarasi]

Thanks for the explanation Honza. will go through the materials and get back to you if I have any doubts.
Yeah you can assign this to me.

Comment 4

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

After working on bug 1463676 I think that the reducer state could be designed as follows:

// 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 the previous input value from the command line
// when the user goes backward.
curentInputValue: null,


In other words, do not append the `current input value` into the entries array, but store it into extra `curentInputValue` field. Than we don't need the `index` prop that make things hard to understand.

Honza

Comment 5

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

@kaviarasi: Are you still interested in this?

Honza

Comment 6

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

Attachment: Bug 1465352 - Clean up history reducer state;

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

Comment 7

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

Try push looks good:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=977e7fdb8172d22ae937d82b859ef1ac6fc1bdb3

Honza

Comment 8

[Nicolas Chevobbe [:nchevobbe]]

Comment on attachment 8993335 [details]
Bug 1465352 - Clean up history reducer state;

https://reviewboard.mozilla.org/r/258120/#review265148

I do have one question and a couple of comment for the test, but this is already a nice improvement and good to land

::: devtools/client/webconsole/reducers/history.js:33
(Diff revision 1)
> -    placeHolder: 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,
> +    position: undefined,
> +
> +    // Stores the current value in the input field. It's used again
> +    // if the user didn't pick up anything from the history and
> +    // returned all the way back to the actual current input value.
> +    currentInputValue: null,

nit: I find this could be a bit misleading, one could think it's the value you currently see in the input (which is not the case when you navigate).

Could we find a name that better conveys what this is, like userInputBackup, userInputStash, or something better than those 2 ?

::: devtools/client/webconsole/reducers/history.js:90
(Diff revision 1)
>  function historyLoaded(state, entries) {
>    const newEntries = [...entries, ...state.entries];
>    return {
>      ...state,
>      entries: newEntries,
> -    placeHolder: newEntries.length,
> +    position: newEntries.length,

it's still weird for me to think of the default state (i.e. when you opened the console), to be the Nth position.
Do you think it would make more sense to have the position be null/undefined until do navigate in the history ?

::: devtools/client/webconsole/test/mochitest/browser_jsterm_history_persist.js:55
(Diff revision 1)
>    state2 = hud2.ui.consoleOutput.getStore().getState();
> -  is(JSON.stringify(getHistoryEntries(state2)),
> -     '["0","1","2","3","4","5","6","7","8","9",""]',
> +  is(state2.history.currentInputValue, "",
> +     "An empty value has been stored as the current input value");
> -     "An empty entry has been added in the second tab due to history perusal");

maybe we could check that the entries weren't modified ?

::: devtools/client/webconsole/test/mochitest/browser_jsterm_history_persist.js:79
(Diff revision 1)
> -  is(JSON.stringify(getHistoryEntries(state2)),
> -     '["0","1","2","3","4","5","6","7","8","9",""]',
> +  is(state2.history.currentInputValue, "",
> +     "Current input value hasn't changed due to command in third tab");
> -     "Second tab history hasn't changed due to command in third tab");

same here, maybe we could check that the entries weren't modified ?

Comment 9

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

Comment on attachment 8993335 [details]
Bug 1465352 - Clean up history reducer state;

https://reviewboard.mozilla.org/r/258120/#review265340

Comment 10

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

Comment on attachment 8993335 [details]
Bug 1465352 - Clean up history reducer state;

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

Comment 11

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

(In reply to Nicolas Chevobbe [:nchevobbe] from comment #8)

Thanks for the review Nicolas!

> ::: devtools/client/webconsole/reducers/history.js:33
> nit: I find this could be a bit misleading, one could think it's the value
> you currently see in the input (which is not the case when you navigate).
I renamed the prop to `originalUserValue` and updated couple of comments.

> it's still weird for me to think of the default state (i.e. when you opened
> the console), to be the Nth position.
> Do you think it would make more sense to have the position be null/undefined
> until do navigate in the history ?
I kept this as is in the end and added a comment explaining it.
It seems to be the simplest to make sure that the data are
properly initialized since the beginning. 

> :::
> devtools/client/webconsole/test/mochitest/browser_jsterm_history_persist.js:
> 55
> (Diff revision 1)
> >    state2 = hud2.ui.consoleOutput.getStore().getState();
> > -  is(JSON.stringify(getHistoryEntries(state2)),
> > -     '["0","1","2","3","4","5","6","7","8","9",""]',
> > +  is(state2.history.currentInputValue, "",
> > +     "An empty value has been stored as the current input value");
> > -     "An empty entry has been added in the second tab due to history perusal");
> 
> maybe we could check that the entries weren't modified ?
Done

> :::
> devtools/client/webconsole/test/mochitest/browser_jsterm_history_persist.js:
> 79
> (Diff revision 1)
> > -  is(JSON.stringify(getHistoryEntries(state2)),
> > -     '["0","1","2","3","4","5","6","7","8","9",""]',
> > +  is(state2.history.currentInputValue, "",
> > +     "Current input value hasn't changed due to command in third tab");
> > -     "Second tab history hasn't changed due to command in third tab");
> 
> same here, maybe we could check that the entries weren't modified ?
Done

Honza

Comment 12

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

Try looks good
https://treeherder.mozilla.org/#/jobs?repo=try&revision=7bda8f314ec1d9183dd27fcbac7be0bee2489a6d

Honza

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2aca3a665afe
Clean up history reducer state; r=nchevobbe

Comment 14

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/2aca3a665afe