Add basic undo in project panel

[marcocondrache]

Initial part for #5039

Continues work from #45008. The author is no longer assigned to the issue, so I’m assuming it’s unclaimed. Please close this if that’s not correct.

• Undo rename
• Undo move
• Undo paste
• Undo drag
• Enough tests

Release Notes:

• Added basic undo for file creation, renaming, moving in project panel

[cla-bot]

We require contributors to sign our Contributor License Agreement, and we don't have @HactarCE on file. You can sign our CLA at https://zed.dev/cla. Once you've signed, post a comment here that says '@cla-bot check'.

[HactarCE]

I no longer work at Zed, but I'm glad someone picked this up. Thank you so much for continuing this!

[HactarCE]

@cla-bot check

[cla-bot]

The cla-bot has been summoned, and re-checked this pull request!

[dinocosta]

Hey @marcocondrache 👋

Thanks for opening this. I was looking at #5039 last week to pick it up and see if we can ship it, so it’s great to see you continuing the work from #45008 !

I paired briefly with @cole-miller today, and we’re planning to update Byron/trash-rs to centralize all trashing and restoring logic there. That crate already provides restoration logic, and both Byron/trash-rs#109 and Byron/trash-rs#128 explore returning the actual item that was trashed, for both Linux and macOS, so that it can be restored. With those changes, it seems we’d only be missing restoration support for macOS as well as returning trashed items on Windows.

If that goes well, we may update these PR changes accordingly, assuming you’re comfortable with that. Let me know if you’re up for some pairing time 🙂

[marcocondrache]

Hey @dinocosta, thanks for checking this - I had exactly the same idea.

Instead of copying to a temporary folder like in #45008, we can reuse the OS trash and avoid doing the work twice. The current state of the PR already does this (at least on macOS), and I’ve been exploring how to implement it on Linux and Windows as well.

I looked at trash-rs when starting the PR, but I didn’t really like the implementation. It requires passing around handles to trashed items, which gets inconvenient, especially for remote worktrees since they’d need proto serialization. I’m currently working on a separate trash crate that abstracts the platform implementation like trash-rs does and allows restoration by only knowing the old path (I can explain how this works).

I love pairing sessions, so we can arrange a meeting.

[dinocosta]

Quick update for anyone following along 👋

Just chatted with Marco today and we’ve decided that, while we’re waiting for the filesystem‑related changes that allow tracking trashed files and directories to be reviewed and approved (zed-industries/trash-rs#1, zed-industries/trash-rs#2, as well as the changes in https://github.com/zed-industries/zed/tree/5039-fs-update and https://github.com/zed-industries/zed/tree/5039-fs-restore), we’ll use this pull request to nail down the undo/redo system and add support for undoing and redoing the following operations:

• Moving
• Renaming
• Creating

We may eventually even ship these first and, once the filesystem‑related changes are reviewed and shipped, open a new pull request adding support for undoing and redoing file and directory trashing 🙂⁠

[dinocosta]

Hey @marcocondrache 👋

Thank you so much for continuing the work on this! I've had a chance to review the current state of these changes and leave some comments in the code, where appropriate.

Some feedback isn't particular to the file or code, so I'll leave it here instead:

• We'll probably need to think a little bit more on how we undo the ProjectPanelOperation::Batch operation. The current implementation seems to simply pop the operation off the stack and then attempts to undo all the operations within it. However, it seems that, if one of the operations within the batch fails to undo, we halt execution and silently "drop" the remaining operations. I wonder if we'll want to, at least, notify the user of which operations failed.
• We still need to explore how we're going to deal with conflicts. For example, right now, if I have a src/main.rs file and, through the Project Panel, rename it to src/bananas.rs but then, outside of Zed, create a new src/main.rs file, attempting to undo the rename will silently fail, while also popping the operation from the undo_stack. For the scope of this Pull Request, I believe we only need to care about displaying an error message to the user when undoing an operation fails, possibly with concrete information on what the operation was trying to do, for example, in the case shared above we'd likely want to tell the user that the rename failed because the target file already exists. In a future Pull Request, we might want to explore how to actually provide conflict resolution steps to the user, maybe something as simple as allowing the user to skip the operation or keeping track of the failed operations so we can retry, after the user manually resolves the conflict.
• Having an "Undo" option option to the right-click menu used in the project panel might also make it easier to discover that this feature exists.
  • Should be disable in case undo_stack.is_empty() is true
  • I might also, in the future, discuss with our design team if there's any icon/indicator/button we want to add for this, to make it easier for folks to know it exists
• I feel like maybe creating a new crates/project_panel/src/undo.rs module might be a good idea, so we can centralize all this undo/redo system logic and implementation there, especially since crates/project_panel/src/project_panel.rs is already ~6500 lines long.

Hope this helps! Let me know if you have any question or suggestion regarding any of the points or if there's something you'd prefer we pair on. Thanks! 🙂

[dinocosta]

It seems there's multiple places where we are performing this check, so we could likely extract this logic and avoid calling unwrap on the popped operation, even though we're checking the vector's length .

Maybe even updating record_operation to record_operations(&mut self, operations: Vec<ProjectPanelOperation>)? We can then match on the vector's length and also match on Vec::pop instead of unwrapping 🧹

[marcocondrache]

Yes right, I've extracted the logic so this is handled better now

[dinocosta]

I wonder if we actually need the cx.spawn here. Can't we just call detach_and_log_err(cx) on the result of self.revert_operation(operation, cx)? 🤔

[marcocondrache]

Right, it was a leftover from the previous implementation

[dinocosta]

Seems like we actually end up not using the is_directory field in the current implementation, so we can probably remove it altogether. We can always add it back later, if necessary 🙂

[marcocondrache]

Should be fixed

[dinocosta]

I'm now thinking that we might want to already have a limit to the number of ProjectPanelOperations we keep in the undo stack, otherwise this could lead to an unbounded growth of the stack, for example, if an user keeps Zed open for a very long time and performs a lot of operations in the Project Panel 🤔

[marcocondrache]

I've added a limit of 10_000 operations which with the current enum (56 bytes) should use around 0.56MB of heap

[dinocosta]

Not sure if strictly necessary, but we might want to call cx.notify() here? I believe the filesystem watcher we setup eventually picks up the file changes from undoing an operation, but it might be worth explicitly notifying the UI to update 🤔

[marcocondrache]

I've added it :)

[marcocondrache]

@dinocosta thanks a lot for reviewing this!

• I extracted the logic into undo.rs to make it easier to manage and reason about. I've got inspired by https://developer.apple.com/documentation/foundation/undomanager
• I looked at how VSCode handles conflicts during undo operations. It simply shows a toast notification with the raw error message, I think we can do something similar.
• I updated the revert logic for ProjectPanelOperation::Batch so it attempts to undo all operations and then collects any errors that occur. We still need to decide how we want to surface or handle errors in this particular case.