Closed Bug 1465352 Opened 7 years ago Closed 7 years ago

Clean up history reducer state

Categories

(DevTools :: Console, enhancement, P1)

enhancement

Tracking

(firefox63 fixed)

RESOLVED FIXED
Firefox 63
Tracking Status
firefox63 --- fixed

People

(Reporter: Honza, Assigned: Honza, NeedInfo)

References

Details

(Keywords: good-first-bug, Whiteboard: [boogaloo-mvp] good-first-bug)

Attachments

(1 file)

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
Keywords: good-first-bug
Priority: -- → P3
Whiteboard: good-first-bug
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.
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
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.
Assignee: nobody → kaviarasi46
Status: NEW → ASSIGNED
Product: Firefox → DevTools
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
@kaviarasi: Are you still interested in this? Honza
Flags: needinfo?(kaviarasi46)
Assignee: kaviarasi46 → odvarko
Whiteboard: good-first-bug → good-first-bug, [boogaloo-mvp]
Priority: P3 → P1
Whiteboard: good-first-bug, [boogaloo-mvp] → [boogaloo-mvp] good-first-bug
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 ?
Attachment #8993335 - Flags: review?(nchevobbe) → review+
(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
Pushed by jodvarko@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/2aca3a665afe Clean up history reducer state; r=nchevobbe
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 63
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: