@meganrogge We add verification-needed on feature requests that don't end up on an endgame test plan. @mjbvz can clarify that a little bit since we're currently in endgame.

@joaomoreno There are a few issues with the new approach:

1. If we only save the current value when the arrows are pressed, then on window reload,
   text typed but not yet committed will not be saved (unless we use the saveCurrent method).

2. I haven't found a way using the historyNavigator to replace / delete a value. I need this so that the current element can be replaced with a new value and so that there isn't a backlog of uncommitted messages.

For example, let's say I have 3 messages successfully committed.

message 1
message 2
message 3

and I can navigate between them. so i'm typing message 4 when I want to reference
message 2. I go up to message 2, return to message 4, successfully commit it.

then what will be in the history at that time is:

message 1
message 2
message 3
message 4 (before looking up at message 2)
message 4 completed

or I return to message 4 and change it completely to hello.

what i'll see in the history if I go back through it is:

message 1
message 2
message 3
message 4
hello

even though message 4 was not a successful commit

I'm not sure how to solve this. what I'd want to see is:

message 1
message 2
message 3
hello

do you agree this would be the expectation?

also, on the first pass through it's doing a strange thing with the order. on the way back down it's resolved and appears to store them in the correct order when debugged.

If we only save the current value when the arrows are pressed, then on window reload,
text typed but not yet committed will not be saved (unless we use the saveCurrent method).

We'd save the value when (1) the inbox box changes in interesting ways (the fromKeyboard story), (2) when navigating back in history and (3) when the workbench unloads. Here's how you can do 3.

do you agree this would be the expectation?

Absolutely! You can always change the HistoryNavigator itself to support your use case. How about adding a remove() method? That way you can remove something at will.

@joaomoreno this is working now except:

1. The ContextKey so that the cursor must be either on the first line (for up arrow) or last line (for down arrow) of the input box. I'm not sure how to do this despite looking at the links you sent me.
2. Regarding save on window reload, is the intended functionality that the state is preserved only when I've typed something and haven't yet committed it? or should it also preserve where you are in the history navigation? if the former, that is working. if the latter, then I will need to somehow store the actual historyNavigator itself since otherwise its state gets reset in the constructor when it's defined with the stored history values on window reload. this.historyNavigator = new HistoryNavigator(JSON.parse(history), 50);

@meganrogge

1. Yeah this isn't so clear to grasp at first. I've added a commit which sets these context keys. All you have to do now it hook those context keys into the when clauses of the keybindings.
2. When the workbench comes up, the user should always be pointing to the last element in the history. Right now I don't really see that happening, following these steps: (1) type something, (2) press Up to see the previous message in history and (3) reload - the value I typed is gone. I expected to see the value I typed in step (1), as it is the last item in history.

A few other issues I see:

• (1) Open the workbench, box is empty, (2) press Up to see the last message, (3) press Down to go back to empty -> 🐛 it doesn't
• (2) Open the workbench, box is empty, (2) type anything, (3) press Up -> 🐛 nothing happens, it only works if I press Up twice

Idea: why not allowing the history to store the empty string as a value? Raising that restriction will let you fix issue (1) above and will let you just store strings, instead of storing strings and booleans in history.

@joaomoreno
Re issue 1, I had thought we wanted to model the search editor behavior. But after discussing this with Kai, I agree it should behave as you describe.
Both issues have been addressed.
The reason we need the boolean is because otherwise we don't know which element to delete

@meganrogge First off: this was a hard work item. Even I failed to grasp the complexity of the issue. As I reviewed it this time I decided to go ahead and finalize it by making some changes. You did a good job, but ultimately were hitting issues of the current HistoryNavigator. Let me explain.

The current HistoryNavigator is weird. It lets the navigation go out of its bound, which is really nasty and misaligns with the fact that we want to store the current editing value as the last of the history. This is the reason why you couldn't simply store strings. It was also the source of bugs like having to press Up twice or going beyond the history array bounds accidentally.

I took the time to write a new HistoryNavigator2 which never lets the cursor go outside a history. It's implemented as a linked list, so capping its size is cheap. The caveat with it is that it doesn't remove duplicates, since I didn't think that was too important. But feel free to add that capability to HistoryNavigator2 if you think it is, should be pretty easy. Ultimately, it lets us just store strings, while keeping our code much smaller and easy to reason about. Please give it a look, try it out and let me know what you think.

Some notes on the work:

• There were both instances of bad formatting and eslint errors like using == instead of ===. We currently have a precommit rule which prevents you from committing mistakes like this. How were you able to create these commits?
• It's usually good practice to wrap JSON.parse in a try catch

As you test this, remember to try it in a new workspace, since the storage in your test workspace contains the array of tuples, so the code will break, since it expects an array of strings.

Overall great work, you really pushed this forward! 👏

We currently have a precommit rule which prevents you from committing mistakes like this. How were you able to create these commits?

probably by using the --no-verify flag on occasion. I won't do that going forward.

@joaomoreno it's now working as expected. I'll wait for your final sign-off to merge it.