Skip to content

Feat/undo redo#391

Open
elijahthis wants to merge 4 commits into
antonmedv:masterfrom
elijahthis:feat/undo-redo
Open

Feat/undo redo#391
elijahthis wants to merge 4 commits into
antonmedv:masterfrom
elijahthis:feat/undo-redo

Conversation

@elijahthis

@elijahthis elijahthis commented Jan 31, 2026

Copy link
Copy Markdown

Undo/Redo for Node Deletion

Fixes #370

New Keybindings (vim-like):

  • u: Undo last deletion.
  • ctrl+r: Redo last undone deletion.

Testing Performed

  • Deleting and undoing single values.
  • Deleting/Undoing entire objects and arrays.
  • Verified that array indices ([0], [1], etc.) update correctly in both directions.
  • Verified that trailing commas are added/removed correctly when undoing the deletion of the last element in a container.

Comment thread main.go Outdated
}
}

func (m *model) createTombstone(at *Node) Tombstone {

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like this method does not uses any model fields. Lets move it to josnx package?

Comment thread main.go Outdated
}

// if it was the first child
if t.Parent != nil && t.Parent.Next == t.Next {

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The logic on undo will be nice to put to jsonx package as well.

@antonmedv

Copy link
Copy Markdown
Owner

Implementation is nice, but lets move all model independent parts to nodes.

And lets add tests! JSON -> delete node -> assert JSON -> redo node -> assert JSON

@elijahthis

Copy link
Copy Markdown
Author

thanks for the comments / review @antonmedv. really solid points.

on it!

@elijahthis

elijahthis commented Jan 31, 2026

Copy link
Copy Markdown
Author

hi @antonmedv
all done. just added testing. had to update .golden files as well to reflect changes to testdata 🫠

@antonmedv

Copy link
Copy Markdown
Owner

ctrl+r: Redo last undone deletion.

But do we really need redo?

@elijahthis

Copy link
Copy Markdown
Author

But do we really need redo?

hmm.
well it's worth having in case a user spams the "undo" button. accidentally. Plus users familiar with vim might expect there to be a redo if there's an undo.

@elijahthis

Copy link
Copy Markdown
Author

Plus since we already implemented Tombstone, the redo is a relatively straightforward implementation.

@antonmedv

Copy link
Copy Markdown
Owner

what about instead of key binding, use command :u

@elijahthis

Copy link
Copy Markdown
Author

what about instead of key binding, use command :u

might feel slightly off for vim users. could work though. they'll just have to read the docs.

also, when you say command+u, do you mean cmd/alt+u for redo? or ctrl+u for redo?

@antonmedv

Copy link
Copy Markdown
Owner

I see. Ok, lets keep u.

Please rebase!

@elijahthis

Copy link
Copy Markdown
Author

Please rebase!

Just rebased. No conflicts with master!

@elijahthis

Copy link
Copy Markdown
Author

Hi @antonmedv any updates on this?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add undo u and redo

2 participants