Add chat history accessible with up/down arrows #1244

Argmaster · 2025-03-25T05:17:50Z

This pull request adds mechanism widely present in terminals which allows user to quickly access commands they typed before by using up and down arrows. Implementation has few assumptions:

When editing multiline message previous/next command should be accessed only from first and last line (respectively)
When modifying command/message from history original history entry has to be preserved
When moving "up" history it should be always possible to return "down" even if modified message
Modified messages are preserved when moving across history entries
Duplicated messages are not preserved

archbirdplus · 2025-03-25T07:52:11Z

src/gui/windows/chat.zig

 pub var input: *TextInput = undefined;
 var hideInput: bool = true;
+var upHistory: CircularBufferQueue([]const u8) = undefined;
+var downHistory: CircularBufferQueue([]const u8) = undefined;


Just wondering, is there an advantage to this double-queue system? Why not just have one "scratch history" buffer and an index into it?

As a result you are never directly modifying any history entry, so they are all preserved.

There is more subtle gain from it compared to single queue history. Assume you have history and move up twice to a command, then you modify it and by mistake move up again. With current system modified message will be inserted in the middle, between history entry you were modifying and history entry you moved to, so you can always go back.

So other options either don't preserve entries you modify or preserving modified entries is O(N).

If you think otherwise please provide more details, I am no data structure specialist, I do stuff so it works the way I want it rather than making it optimal from the beginning.

Oh, your model here is totally different from what I expected. I expected that you just had a list of old messages, and you could modify each one (or all) as you wished, and that scratch history would get replaced as soon as you entered something. Coincidentally like my favorite zsh.

What you have is that you can scroll through each message, and edit them, but when scrolling away from an edited message you first insert it into the history list. I mean, if that's intended behavior, your data structure is pretty good (though do you need to duplicate the string before immediately freeing it?). I get it now, but the user experience is quite surprising.

Oddities:

The first time you press up or down after editing a message, the message box "undoes" your message rather than actually scrolling to the message before/after the one you were just editing.

Message boxes get cloned every time you scroll up or down, and this can get quite cluttered if you move back and forth. I don't think there's a great reason to preserve the edit-histories of history like that. Generally this would be written into an invisible undo-buffer to be accessed with +Z.

Edits to messages get preserved after pressing enter. Combined with the previous issue, it means that the intermediate edits linger up in your history, while the final drafts ironically get removed.

Overall, I think this is way too conservative. I don't think chat messages are precious enough that every change must be remembered. Most software (besides vim, which has 2 dimensions of undo) would start dropping edits much earlier than this.

I just checked, and it turns out that shells also have different ways of dealing with history. My zsh keeps history edits when scrolling, then clears them when any command is run. Apparently my bash keeps history edits until that specific line is entered, and only resets one line at a time. So there is variation there.

Now I see what you're getting at, but I also think you're just trying to serialize a list of undo histories (a 2d array) into the main edit history. So I guess I would implement this as an array of doubly-linked lists (rather than your center-insertable circular buffer, which I do think is quite clever). This feels like a natural modern GUI generalization of shell-style edit histories. Thus you could scroll through the history above, have it not desync from the displayed history, and you can edit it at your leisure, and your can use standard UI conventions to recover from accidents.

Wow this is quite a long message, but there are a surprising number of subtleties going on here. Basically, your UI is rather idiosyncratic, which wouldn't help your PR or first-time users.

Hmm I don't find this this particularly "idiosyncratic", but this is expression of my personal preference - it pisses me of like hell when I navigate to long command I used before, edit it (possibly significantly) just to lose it cause one of my fingers hit up/down arrow by mistake or the original command goes away, especially if it was doing some clever thing like $(git log -1 --format=%H) and I have to type it again.

As for messages being precious, no, they are not, but commands with complex block patterns are and it is common to shuffle through many different patterns and mask expressions while building, so I find preserving them very valuable. In shell you can easily open a notepad, and use Ctrl+Z/fg to back and forth if you really need that high precision while modifying commands, but here there are no scripts, no subshells, only command line. If you want to preserve anything you need to copy it, open notepad as separate window and paste it there, that significantly increases complexity the command has to reach before it is worth the effort.

As for command history, problem is that history done per command can not diverge, if you change command, undo that change and change it again you inevitably lose first branch. As I explained it is common to experiment a lot when doing patterns, so it is inconvenient.

This behavior can be actually quite easy to summarize: "Every unique message (command) you submit is preserved" and "submit means that you either hit enter or move to different history entry with up/down arrows"

Well, instead of putting it above the command that's modified, you could just put it below it???

Well, it doesn't exactly work like that. See transferBetweenQueues, it doesn't place anything explicitly above or below, it moves current message (if it is different than top value of outQueue) and first inQueue entry to outQueue. Rest is emerging behavior of that implementation. While it is possible to do what you are asking for, it is not trivial.

Ok, I kind of figured that out, so now behavior is as follows:

type: A <Enter>
type: B <Enter>
type: C <Enter>

History:

A B C

textInput: ``

type: <upArrow>

textInput: C

type: <upArrow>

textInput: B

type: 12
textInput: B12

And now from that state:

1. Scenario

<upArrow>

textInput: B

History:

A B <-- we are here B12 C

2. Scenario

<downArrow>

textInput: C

A B B12 C <-- we are here

Rest should be more or less the same. I would appreciate if @archbirdplus and @IntegratedQuantum could try to break it cause I have no faith that it will work correctly.

There seems to be an issue when changing the last history entry:
If you press up after changing the oldest entry, it will still not navigate to the previous value of it.
But to be honest, with an 8192 history buffer and since this will likely be saved in the future too, I don't think it matters much.
So we can keep it bugged if that keeps the implementation simpler.

Yeah, that edge case would be hard to get rid of, since this is because up queue is empty when you reach last item.

src/gui/windows/chat.zig

src/gui/components/TextInput.zig

IntegratedQuantum · 2025-03-27T16:14:40Z

src/gui/windows/chat.zig

 pub var input: *TextInput = undefined;
 var hideInput: bool = true;
+var upHistory: CircularBufferQueue([]const u8) = undefined;
+var downHistory: CircularBufferQueue([]const u8) = undefined;


I also think it would be nicer if the message was inserted below the original and not above it. It also shouldn't be too much trouble to implement, right?

src/gui/windows/chat.zig

…tory

src/gui/components/TextInput.zig

src/gui/windows/chat.zig

src/utils.zig

Co-authored-by: IntegratedQuantum <43880493+IntegratedQuantum@users.noreply.github.com>

…tory

Argmaster · 2025-04-25T10:25:22Z

I have changed the code to use FixedSizeCircularBuffer.
Now there is a bit confusing edge case that if up queue is full and you move all the way up so all the messages are in down queue and after that you modify one of the messages, the latest message in down buffer will be discarded, since forceEnqueueFront does dequeueBack if queue is full. However considering the history has 8k entries, it is really unlikely people will navigate all the way up. There is no simple workaround, as player can generate infinitely many modified messages, It would require some kind of special case and imo it is not worth the effort.

Argmaster · 2025-04-25T10:36:19Z

Alright I also made it so it is possible to return to empty entry if you have navigated up, but it is not saved.

Argmaster · 2025-04-30T20:51:34Z

@archbirdplus alr so, I am too stupid to fully understand what you are saying, but I changed it again a bit, and I cant really reproduce the duplicated message behavior so for me it seems alr... Could you check again and pretty please provide simple step-by-step guide how to reproduce any undesired behaviors you find?
Not saving messages that already appear in history if they were selected from history was intentional but I changed it bcs it was counterintuitive. Blank entries should not appear in history, but blank entry at the beggining of the history is immortal due to how it is implemented and will stay there, no other blank entries will stay in history (like 99% sure).

archbirdplus · 2025-04-30T22:05:25Z

Sorry.
Issue 1:

Open chat. Press a Enter b Enter c Enter. Current textbox: . Press d. Current textbox: d. Press ↑. Current textbox: . Instead, it should be c. Press ↓ ↓. This can either be d or , up to you, but the blank higher up should be removed.

Issue 2:

Open chat. Press a Enter b Enter c Enter. Current textbox: . Press ↓ ↑. Current textbox: . Instead, it should be c. The appears to have been duplicated and you can scroll into it twice.

Issue 4:

Open chat. Press a Enter b Enter c Enter. Current textbox: . Press ↑. Current textbox: c. Press Delete b. Current textbox: b. Press ↑. Current textbox: b. Instead, it should be c. The history is a b c . Instead, the history should go a b c b because edits should go below their parents.
Open chat. Press a Enter b Enter c Enter. Current textbox: . Press ↑ ↑ ↑. Current textbox: a. Press Delete b. Current textbox: b. Press ↓. Current textbox: b. Although its a bit ambiguous, it should probably be c because you de-duplicate history. Currently the history reads a b b c . Instead, the history should go a b c and one of the b should be trimmed.

These are the setups that I am testing. Tell me if you disagree or can't replicate something.

Argmaster · 2025-05-01T14:24:05Z

Issue 1 - can't reproduce after last change. Can you confirm you still can reproduce?
Issue 2 - can't reproduce after last change. Can you confirm you still can reproduce?
Issue 4 a and b - The fact that you have manually typed contents of history entry doesn't make your message a history entry. Your message was not yet part of history, only after you moved down/up, it became part of history and since it was a duplicate it was removed. Harmless and arguably preferable approach. Jumping two positions if your message was a duplicate would be more surprising IMO. It should also be very rare in average use.

archbirdplus · 2025-05-01T19:57:32Z

Issue 1 is resolved. Issue 2 is resolved. Ok, great!
I agree that issue 4 is very rare, but it has the appearance of being an intentional feature, although slightly broken.

For 4a:
What I dont understand is why editing c to be b would make it de-duplicated. Isn't the logic supposed to be that edits come after the original message, therefore suggesting a b c b instead?

For 4b:

Jumping two positions if your message was a duplicate would be more surprising IMO

Alright, I'll take that. Then the 4b problem is slightly more reasonable, but there are still 2 consecutive b entries in history. They weren't removed. And if this message isn't removed, then I don't understand why the message in 4a is removed.

Argmaster · 2025-05-02T16:39:18Z

@archbirdplus Alr, I misunderstood issue 4, you are right, in 4a result should be a b c b, please check if this is the case now. As for 4b, its not trivial, I tried something, it might work, but It may cause duplication problems in different places. the problem is that for history cycling to work I need to check previous and next entry, but for that b to get deduplicated I would also have to check entry before previous. I am checking only values directly adjason, so to do that I need to sacrifice checking next value instead. What we achieved this way is that we still get b after doing ↓ but it is not duplicated. I think it is a valid outcome, you always go to previous history entry ↓ even if you created same entry manually.

archbirdplus · 2025-05-02T19:22:53Z

I thought I saw it again, but I can't reproduce 4. But I found one more- when you scroll up, exit chat, then press /, the chat gets replaced with / but you stay in the same place in history. I think its fine to keep the location of history after exiting, but not with the / shortcut. That should be all.

Argmaster · 2025-05-02T19:31:52Z

@archbirdplus Ehh that is a bad one, I already made tools to fix this in #1380 (onCharacter callback) but fixing conflicts caused by adding it here would suck, can we note this down as known limitation and fix this after #1380? Basically fix would be to flush history in onCharacter if all input contains is '/', just adding that callback now is not necessarily fun experience C:
I suppose I should ask @IntegratedQuantum the same C:

archbirdplus · 2025-05-02T20:04:12Z

As long as the bug won't be left hanging indefinitely, I approve this PR from the functionality perspective.

Argmaster · 2025-05-03T09:25:16Z

I will make PR for that today

IntegratedQuantum · 2025-05-03T12:54:17Z

src/utils.zig

 		}

+		pub fn dequeueFront(self: *Self) ?T {
+			return self.dequeue();


There is a mismatch in the naming conventions.
Below, in CircularBufferQueue, you delcared dequeue_back as self.dequeue

Yes, but I suppose we prefer camelCase rigth? That other one should be gone + it was done that way because of other methods being snake_case

I have removed changes from CircularBufferQueue

It's also about back vs front naming

Hmm, right, I'll swap those

src/items.zig

…tory

IntegratedQuantum

Now I'm still confused on the naming convention, I looked it up on wikipedia and consensus seems to be that back is at the end of the range, which kind of makes more sense.

But I don't want to keep this PR blocked on naming issues any longer, so please resolve this as a part of #1320, and please do it sooner rather than later.

* Add up down message history * Deduplicate messages when inserting into history * Add dedicated function for inserting strings into TextInput * Fix formatting issues * Change history behavior * Rename inputString to setString * Move clearing to setString * Apply suggestions from code review Co-authored-by: IntegratedQuantum <43880493+IntegratedQuantum@users.noreply.github.com> * Remove unused cursor capture * Use FixedSizeCircularBuffer for history * Restore large queue size * Allow navigation to empty entry * self.len must never be bigger than capacity * Move optional callbacks into struct * Use enum for moveCursorVertically return value * WA attempt #1 * Fix edge case from review * Update src/gui/windows/chat.zig Co-authored-by: IntegratedQuantum <43880493+IntegratedQuantum@users.noreply.github.com> * Remove isEmpty and isFull * Allow for empty history entry * Change empty message handling some more * Remove unused methods * Go to hell with all of those edge cases <3 * WA for 4b * Remove CircularBufferQueue methods * Fix queue thing --------- Co-authored-by: IntegratedQuantum <43880493+IntegratedQuantum@users.noreply.github.com>

Add up down message history

8608c52

Argmaster changed the title ~~Add up down message history~~ Add chat history accessible with up/down arrows Mar 25, 2025

archbirdplus reviewed Mar 25, 2025

View reviewed changes

Argmaster added 3 commits March 26, 2025 06:39

Deduplicate messages when inserting into history

3f6fbde

Add dedicated function for inserting strings into TextInput

eb064b1

Fix formatting issues

5c0c4d2

Argmaster marked this pull request as ready for review March 26, 2025 06:25

IntegratedQuantum requested changes Mar 27, 2025

View reviewed changes

Argmaster added 3 commits March 28, 2025 01:55

Merge remote-tracking branch 'origin/master' into feature/up-down-his…

bb1cc73

…tory

Change history behavior

14f7b81

Rename inputString to setString

ad154fc

Argmaster requested review from IntegratedQuantum and archbirdplus March 28, 2025 01:53

Move clearing to setString

93c014a

IntegratedQuantum requested changes Apr 19, 2025

View reviewed changes

Argmaster and others added 3 commits April 19, 2025 18:50

Apply suggestions from code review

a7325eb

Co-authored-by: IntegratedQuantum <43880493+IntegratedQuantum@users.noreply.github.com>

Merge remote-tracking branch 'origin/master' into feature/up-down-his…

ea373d5

…tory

Remove unused cursor capture

9d574a2

Argmaster marked this pull request as draft April 23, 2025 19:59

Argmaster added 2 commits April 25, 2025 12:17

Use FixedSizeCircularBuffer for history

fe5ce81

Restore large queue size

cf18507

Allow navigation to empty entry

a69839b

Argmaster marked this pull request as ready for review April 25, 2025 10:37

Argmaster requested a review from IntegratedQuantum April 25, 2025 10:37

Argmaster self-assigned this Apr 26, 2025

Argmaster added enhancement a new feature or improvement ui/ux labels Apr 26, 2025

Argmaster added this to the Short-Term Goals milestone Apr 26, 2025

Change empty message handling some more

bcb3743

Remove unused methods

2ff79af

Argmaster mentioned this pull request May 2, 2025

Add coniferous trees the way I like them #1377

Merged

Argmaster added 2 commits May 2, 2025 18:25

Go to hell with all of those edge cases <3

4d1cabe

WA for 4b

b965c0a

Argmaster mentioned this pull request May 2, 2025

Command name autocompletion #1380

Closed

Argmaster marked this pull request as ready for review May 2, 2025 21:59

Argmaster requested a review from IntegratedQuantum May 3, 2025 09:25

Argmaster mentioned this pull request May 3, 2025

Fix reset history on / #1386

Draft

IntegratedQuantum requested changes May 3, 2025

View reviewed changes

Argmaster added 2 commits May 3, 2025 16:32

Remove CircularBufferQueue methods

92db5e0

Merge remote-tracking branch 'origin/master' into feature/up-down-his…

75c4fcd

…tory

Argmaster requested a review from IntegratedQuantum May 3, 2025 14:42

Fix queue thing

a25593c

IntegratedQuantum approved these changes May 4, 2025

View reviewed changes

IntegratedQuantum merged commit 67135b4 into PixelGuys:master May 4, 2025
1 check passed

IntegratedQuantum mentioned this pull request May 9, 2025

Chat QoL (keybind to scroll through history like in a shell (arrows)) #428

Closed

Argmaster mentioned this pull request Jun 30, 2025

Chat reset on / should reset history #1645

Open

IntegratedQuantum modified the milestones: Short-Term Goals, Version 0.0.0 Oct 5, 2025

Add chat history accessible with up/down arrows #1244

Add chat history accessible with up/down arrows #1244

Uh oh!

Conversation

Argmaster commented Mar 25, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Argmaster Mar 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Argmaster Mar 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Argmaster Mar 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

1. Scenario

2. Scenario

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Argmaster commented Apr 25, 2025

Uh oh!

Argmaster commented Apr 25, 2025

Uh oh!

Argmaster commented Apr 30, 2025

Uh oh!

archbirdplus commented Apr 30, 2025

Uh oh!

Argmaster commented May 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

archbirdplus commented May 1, 2025

Uh oh!

Argmaster commented May 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

archbirdplus commented May 2, 2025

Uh oh!

Argmaster commented May 2, 2025

Uh oh!

archbirdplus commented May 2, 2025

Uh oh!

Argmaster commented May 3, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Argmaster May 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Argmaster Mar 26, 2025 •

edited

Loading

Argmaster Mar 27, 2025 •

edited

Loading

Argmaster Mar 28, 2025 •

edited

Loading

Argmaster commented May 1, 2025 •

edited

Loading

Argmaster commented May 2, 2025 •

edited

Loading

Argmaster May 3, 2025 •

edited

Loading