Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make undo-tree an optional dependency #1360

Merged
merged 8 commits into from
Oct 8, 2020
Merged

Make undo-tree an optional dependency #1360

merged 8 commits into from
Oct 8, 2020

Conversation

wasamasa
Copy link
Member

@wasamasa wasamasa commented Oct 1, 2020

I'd be thrilled if people tested this. Maybe even if a starter kit like Doom or Spacemacs enabled it. CC @hlissner @syl20bnr

@fbergroth
Copy link
Contributor

Using undo-redo, I get an error when evil-redo calls (undo-redo nil), i.e. when count is nil. It seems to work if I provide a digit argument.

@wasamasa
Copy link
Member Author

wasamasa commented Oct 7, 2020

@fbergroth Thanks, I've fixed this now.

@leungbk
Copy link
Contributor

leungbk commented Oct 7, 2020

Thanks for doing this. It's working well for me.

@leungbk
Copy link
Contributor

leungbk commented Oct 7, 2020

Since goto-chg.el is still a dependency, and since undo-tree is a dependency of goto-chg.el, evil still winds up depending on undo-tree. From a quick skim of undo-tree's code, it doesn't look like the advice and hooks it sets are particularly intrusive when global-undo-tree-mode is turned off, though.

@wasamasa
Copy link
Member Author

wasamasa commented Oct 8, 2020

Hm, that's a good point. The code of goto-chg.el does have a soft dependency on undo-tree.el, but it still does declare a hard dependency on it and errors out if it cannot be loaded up successfully.

@TheBB
Copy link
Member

TheBB commented Oct 8, 2020

Seems to work well for me, using the native Emacs (28+) functions.

evil-commands.el Outdated
@@ -1623,12 +1623,12 @@ of the block."

(evil-define-command evil-undo (count)
"Undo COUNT changes in buffer using `evil-undo-function'."
(interactive "<c>")
(interactive "p")
Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps (interactive "*p") (error if buffer is read-only)?

Copy link
Member Author

Choose a reason for hiding this comment

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

Hm, it doesn't seem to ever hit that case sinde read-only buffers tend to not have any undo data in the first place or allowing you to edit them to create it.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah ok. A user could potentially enable read-only-mode after having made some changes though. I just noticed that undo-redo used "*p".

Copy link
Member Author

Choose a reason for hiding this comment

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

Fair enough, fixed.

@wasamasa
Copy link
Member Author

wasamasa commented Oct 8, 2020

I've found that setting this to undo-tree only works if you've made sure to evaluate (global-undo-tree-mode) first. For some reason I haven't been successful with enabling it automatically for users, but eh, I can live with that.

@TheBB
Copy link
Member

TheBB commented Oct 8, 2020

Might be worth pointing that out somewhere, like the docstring of the evil-undo-system variable at least.

@wasamasa
Copy link
Member Author

wasamasa commented Oct 8, 2020

Alright, I've found out my issue and pushed a fix. Now you'll get a helpful error message if undo-tree-mode has not been enabled.

@TheBB TheBB self-requested a review October 8, 2020 11:04
@wasamasa
Copy link
Member Author

wasamasa commented Oct 8, 2020

Before merging this I'd like to try removing the hard dependency of goto-chg.el on undo-tree.el, otherwise the changes to the README claiming that Evil no longer requires undo-tree.el aren't true.

Edit: emacs-evil/goto-chg@5c057c8

@wasamasa
Copy link
Member Author

wasamasa commented Oct 8, 2020

I went through the issue tracker and noticed that this PR might end up closing the following issues as well:

@TheBB TheBB merged commit 373a57e into master Oct 8, 2020
@TheBB
Copy link
Member

TheBB commented Oct 8, 2020

Good work. Thanks!

@ColemanGariety
Copy link

I now get the error Customize 'evil-undo-system' for redo functionality. when redoing in evil-mode. Is this expected to break? What's the suggested way to get redo now?

@leungbk
Copy link
Contributor

leungbk commented Oct 8, 2020

I now get the error Customize 'evil-undo-system' for redo functionality. when redoing in evil-mode. Is this expected to break? What's the suggested way to get redo now?

(setq evil-undo-system PREFERRED-UNDO-SYSTEM)

The options are 'undo-tree (extension), 'undo-fu (extension), and (Emacs 28 only) 'undo-redo.

@TheBB
Copy link
Member

TheBB commented Oct 8, 2020

Yeah it's expected. Do as the message says and customize that variable.

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.

5 participants