Closed jdtsmith closed 6 months ago
Thanks, looks nice! But please make sure only indent with spaces, I'm seeing tabs again. Apart from the comments above, I also see terminal color codes in the terminal. I know you are using Emacs facility for diffing, is there anything we can do? Or at least teach users how to resolve it in the docs.
Thanks. Indent fixed. For your color codes in terminal, maybe try (setq diff-switches "-u --color=never")
?
The one other feature I considered overnight is just replacing those terrible tmp file names with the Current/Marked (with TS). But it sounds a bit fragile, so maybe not worth it for the improved cleanliness.
I have added a sanity check wrapper prior to the moves, and pushed some README/NEWS changes for your consideration. I'm still playing with the formatting in the diff buffer, will wrap that up soon.
I made a few more improvements today:
vundo-diff
displays timestamps independent of whether the user has green circles on (vundo-highlight-saved-nodes
), we need last-saved-idx
to always be saved (further separating display/other logic). last-saved-idx
timestamp for the node, when it is just the most recent saved buffer state — it gets a green circle, after all. (New function vundo--any-timestamp
). vundo-diff
results buffer to remove the ugly tmp file names and substitute vundo-specific info. (New function vundo-diff--cleanup-diff-buffer
).vundo-diff-mode
derived from diff-mode
, to fontify said file header info.declare-function
's to silence compiler. This PR is pretty well complete IMO, pending any further review.
Looks nice!
Turns out that diff can be called async, and if so, I need to update/cleanup the diff buffer in the process sentinel.
Why use the async version? IMO the sync version is perfectly fine and should be simpler to use, right?
And I wonder if we can use a single key to mark/unmark nodes, instead of using both m
and u
. You know, make it vundo-toggle-mark
or something.
Turns out that diff can be called async, and if so, I need to update/cleanup the diff buffer in the process sentinel.
Why use the async version? IMO the sync version is perfectly fine and should be simpler to use, right?
Since it's the default, people might be used to that behavior of diff. The cost to support async is not high: just a lambda for the process sentinel. And sometimes tmp files are across a tramp link. [Update: since all the undo data are in process, I don't buy my own argument. I've switched to a sync-call for simplicity. ]
And I wonder if we can use a single key to mark/unmark nodes, instead of using both
m
andu
. You know, make itvundo-toggle-mark
or something.
~Good idea. Maybe just m
.~ OK, just tried and I don't love it. The problem is this: I often cruise around a few nearby nodes, and set a few different marks in a row, and re-diff looking for "something specific". This would double the number of m
's I have to hit.
We could make it so that m
on an unmarked node sets is, m
on a marked node unsets. But that's getting fancy/non-intuitive. I think m
+ u
is pretty simple and clean, and leverages dired
knowledge.
And if you don't mind taking a look up at my question about vundo-refresh-buffer
, I'm still a bit confused about the need for that to be there twice.
We could make it so that m on an unmarked node sets is, m on a marked node unsets. But that's getting fancy/non-intuitive. I think m + u is pretty simple and clean, and leverages dired knowledge.
Ok, I trust your judgement here.
And if you don't mind taking a look up at my question about vundo-refresh-buffer, I'm still a bit confused about the need for that to be there twice.
Yeah, moving to a node pushes new undo entries onto undo-list, and we need to process those with vundo-refresh-buffer
(update mod-list, etc).
Thanks for the feedback.
And if you don't mind taking a look up at my question about vundo-refresh-buffer, I'm still a bit confused about the need for that to be there twice.
Yeah, moving to a node pushes new undo entries onto undo-list, and we need to process those with
vundo-refresh-buffer
(update mod-list, etc).
I think what confuses me is this note from the vundo--trim-undo-list
doc:
We can call
vundo--move-to-node
multiple times between twovundo--refresh-buffer
.
That doesn't seem to be the case here. I.e. you must call refresh-buffer
after each of the pair of moves (there and back) or you get no route
. Probably not important.
Found one more little doc tweak I pushed, so this is ready to go.
In terms of version bump, I do have a couple of small things I'd like to send in as a small additional PR once this is merged:
l
key move to a prior saved node, if already on one (so you can l
, l
, l
, to go back through saved states), and have it report the saved time too.That doesn't seem to be the case here. I.e. you must call refresh-buffer after each of the pair of moves (there and back) or you get no route.
Oh, I would trust the me that wrote the comment much more than I trust the me right now. If the comment says so, we should be able to run move-to-node
multiple times.
I think I know why you'll get "no route". One example: in an undo list [1 2 3 4 5], you'd have mod list [1 2 3 4 5], and if you go back to node 3, your undo list becomes [1 2 3 4 5 4' 3'], but without calling refresh
, the mod list remains [1 2 3 4 5]. Now, if you want to go back to 5, there's no route available in the mod list (you can think of it as you can only go left in the list, so from 5 to 1-4, but not the other way around). Once you call refresh
, the mod list becomes [1 2 3 4 5 4' 3'], now you have a route from 3 to 5 (in the form of 3' to 5, which is going left).
Good call though, I never thought about this before. I should add a comment somewhere calling this out.
So the branch is ready for a final review, right? It'd be great if you can squash everything and write a commit message. Then I'll review and merge it.
I think I know why you'll get "no route".
Right, you can't change direction between refresh
's.
I'll review and merge it.
Great thanks, commits squashed, ready for review. Once merged I'll submit my small PR for timestamp stuff (I'm already loving multiple l
).
Ok, some final nits:
vundo--orig-buffer
and vundo--last-saved-idx
in their original place, and just place (defvar vundo--last-saved-idx)
in the timestamp section (to suppress compiler errors)?(setq diff-switches "-u --color=never")
somewhere in the documentation?OK should be all set, thanks for the review. I also threw in a docstring escape of single quotes to silence the compiler. Once this is merged I have a TS-related PR I'll post.
Merged, thanks!
This provides
vundo-diff
support with the following capabilities:vundo-diff-mode
global minor mode~ (now builtin)(m)ark
,(u)nmark
, and(d)iff