vundo saved status: add highlights for saved buffer states

[jdtsmith]

I often find myself hitting return to check the "saved" status of a given state along vundo's history. This PR adds highlights to indicate which buffer states have been saved, including a special mark for the most recent.

Details

buffer-undo-list includes timestamp records like (t 25613 2538 435008 282000) whenever a given modification alters a buffer which has been created or saved to file. This PR adds a (green) highlight face to those buffer states which were saved, and a solid green overlay for the most recent.

Note that the "saved" status inferred from the undo-list timestamps is only visible after that buffer is further modified. So if you go back to some state, save it, and then immediately leave it via vundo navigation, that save is simply not recorded on undo-list. We therefore also save and mark the "most recent saved state" specially. This also has the nice additional benefit of indicating which is the most recent save when you've saved states along several branches in the tree (rightmost is not always most recent!).

This is achieved by saving the (nil-trimmed) buffer-undo-list via an after-save-hook in the original buffer. We can then use vundo's nice hash table to map it back to the node [1].

One thing to consider is that the after-save-hook is only setup after vundo is invoked the first time. So the solid green indicator will only show thereafter. I think this isn't such a problem, but the trivial after-save-hook could be setup on vundo load or init time instead, so that the last save prior to invoking vundo the first time in a buffer will be correctly indicated.

[1] It's slightly more complicated: due to vundo's clever "trimming" of the excess state repetitions from looping undo-lists, we must update the saved vundo--undo-list-at-last-save using the master equivalent node's undo-list value.

[jdtsmith]

For people reading along, here's how it looks. Open green circles are all the saved states, solid is the most recently saved.

[casouri]

Looks cool! While I read your PR, have you signed the copyright assignment for Emacs? Because this package is on ELPA and ELPA is part of Emacs, technically you are contributing to Emacs. So you need to sign the assignment which essentially shares your copyright on all of your Emacs contribution with FSF. If you haven't, you can send an email to emacs-devel@gnu.org asking for it. Someone will send you the assignment.

[jdtsmith]

Yes I have, long ago. I added a couple small cleanup tweaks.

[casouri]

Fantastic!

[casouri]

I've looked at the code, it's pretty clever! I made some minor stylistic changes to your first commit, and added a commit to fix one corner case I encountered. You can see the commits on branch last-saved-working.

The corner case happens when the saved node is on the tip of a branch, try this:

1. open a file
2. insert "a\n"
3. insert "b", save the buffer
4. undo, now there should only be "a\n" in the buffer
5. insert "c"
6. open vundo, you'd expect to see the node representing the "a\nb" state to be colored in green, because we saved it, but it isn't.

You can read more in the commit message, fortunately the fix is pretty simple.

If you are fine with it, I'll merge my minor fix into your first commit and add some commit messages, and push it with my fix commit. (Plus some documentation.)

OTOH, I'm hesitant about the "most recently saved" feature. I don't know what many users would feel about vundo quietly adding a hook to after-save-hook and never removing it; plus the "most recently saved" wouldn't be correct before user invoked vundo for the first time in a buffer (adding to a hook globally at load time is way beyond the line IMO); plus it doesn't feel as essential: one can easily tell that the most recently saved state is the saved node that's closes to the current state, or the current state itself.

IOW, if it works perfectly and cleanly without adding hooks, I'd happily add it; but in it's current state I'm hesitant.

[jdtsmith]

Thanks for taking a look. That's a good find RE child vs. "next node" in the mod-list; makes sense in hindsight. Of course it still doesn't fix the problem of "no additional mods" leaving a saved node unmarked.

I'm hesitant about the "most recently saved" feature

I figured this might be a bit more to consider ;), which is why I kept the commits separate. I do worry that not marking the last save will lead to non-intuitive results, when you save a state and make no further changes, either at the top of a branch, or having moved "back in time". For example, the recent rebinding of save-buffer presumably will often result in this state (saved along history, did not yet mod from there).

wouldn't be correct before user invoked vundo for the first time in a buffer

Right. In that case you get no filled green circle, so it's not misleading at least. I do agree about not adding an after save hook en masse in every buffer, and I can see how some may not like any save-hook at all.

if it works perfectly and cleanly without adding hooks, I'd happily add it;

I had one other thought to try; give me a bit to explore it and I'll update this branch incorporating your tweaks.

[jdtsmith]

OK I merged in your last-saved-working branch, and rolled back the after-save-hook. Instead, as a compromise position, this now saves the timestamp itself (not just a flag) in each node with a (t ...) among its records. We then accumulate the latest seen timestamp among all new mods, marking its prior node specially.

So this solves 1/2 of the problem: that the "rightmost" saved state is not always the most recent. You can now clearly see which save is the latest. It also eliminates the after-save-hook entirely. Then I suppose all that's needed is some documentation to make clear that green nodes are those which are "saved and then modified". Nodes which are "saved but never modified" are simply not indicated.

Let me know what you think. Feel free to push changes directly to this PR.

[jdtsmith]

Should we also consider new key binding(s)?

l:  to go back to the previous saved node along this branch
L:  to go to the latest saved node

[casouri]

I think binding l to "go to latest saved node" is good enough.

Could you introduce a defcustom that toggle display for saved nodes, and use bold face instead of a solid circle for the latest saved node?

Could you track timestamps in vundo--draw-tree? IIRC vundo--mod-list-from doesn't have any side effect and it'll be better to keep it so. That way we wouldn't need vundo--latest-saved-ts either, I think.

More over, is comparing timestamp necessary? The last mode in the mod list which is saved should naturally be the latest save node, right?

[jdtsmith]

The reason I had compared TS and not index is incremental updates and GC may invalidate indices, but "timestamps are forever". But accumulating in draw instead of mod-list-from renders this moot. I've update to record a vundo--last-saved-idx, which is accumulated during draw. I apply a face vundo-last-saved, which I've setup to bold. This is a bit subtle for my eye, but it's easy to change. I still need to implement the key binding but thought I'd give you a chance to look.

[casouri]

Thanks, I added some comments. Also, could you squash all the commits into one and force push it?

And for the case when there isn't any modification after the last saved node, you can check whether the original buffer is saved (buffer-modified-p), if it is, then the last node must be the last saved node.

[jdtsmith]

for the case when there isn't any modification after the last saved node, you can check whether the original buffer is saved (buffer-modified-p), if it is, then the last node must be the last saved node. In new file, "a\nb"

OK this is a good thought. It's not the last, but the current node that has this logic. For the last node it's probably fine just to mark it last-saved, even though no timestamp has been seen. But an intermediate node will be "ephemeral".

But... this spurred a new discovery: (t ..) timestamp records ARE inserted on the buffer-undo-list even when you simply save then "undo away" from some state! I.e. simply looping back on the undo-list does insert timestamps when appropriate (when undoing/redoing away from a newly saved state). The problem (I think) is that these subsequently get trimmed away by vundo to control the "useless loopback" mod explosion.

Conclusion: checking that the undo-list pointer matches again some prior value (via the mod-hash) doesn't really tell the whole story; the buffer-undo-list must be getting insertions not just pushes to head. So this inserted data is being lost. Any ideas on how to save it?

[jdtsmith]

This ALSO explain one small issue I have long noticed with vundo: when you navigate to and then save an older buffer state, and then undo/redo away from and back to it, the buffer is no longer marked "unmodified". But if you do the exact same operations with with plain undo/redo, the buffer is marked unmodified. This is in fact the main use of the timestamps:

(t . time-flag) ... primitive-undo uses these values to determine whether to mark the buffer as unmodified once again; it does so only if the file’s status matches that of time-flag.

I had the vague sense that the file was being modified somehow, but I now understand the real reason: vundo has trimmed away and tossed those "loopback timestamps". I think we should figure out how to preserve them.

[jdtsmith]

Using your (lovely) drawings to make it clear what I mean:

[jdtsmith]

One maybe dumb idea: save the timestamp on the node itself, not the next node where it actually occurred. When trimming the useless tail, first process any timestamps among the to-be-trimmed records, and update the TS of the lowest index equivalent node (No. 1 in the image above).

I get fuzzy when it comes to efficiency impact, GC issues, etc. This would also (if it worked) make vundo happy, but not plain undo itself, as plain undo expects those "save amidst a series of undos" to be tagged so it can set buffer-modified-p accordingly.

[jdtsmith]

..this isn't a fix for the lost timestamps issue, just a hack to highlight current node as last-saved if buffer is unmodified. As soon as you visit another node, it loses its "last saved" status.

[jdtsmith]

I squashed this PR as-is, with the quasi-fix (mark current node saved if buffer is unmodified).

I have what may be a solution, but I think it would be too much for this PR. I'll open a separate issue with my idea (see #66).

[casouri]

As soon as you visit another node, it loses its "last saved" status.

You mean the last-saved highlight is removed as soon as we move to another node? Why?

Could you write vundo--node-saved-ts in plain let and and? That isn't a very good use of and-let IMO.

Why do we need to change vundo--highlight-node?

Also, please, fix indentation and trailing whitespace before committing. And I'll be happier if all the comments starts with a capital and ends with a period.

[casouri]

Also, why highlight the late saved node in vundo--refresh-buffer rather than in vundo--draw-tree?

[jdtsmith]

Also, why highlight the late saved node in vundo--refresh-buffer rather than in vundo--draw-tree?

Mostly for symmetry with the highlighting of the current node, and because we are (for now) special casing the current node when it is unmodified. I could make a helper function vundo--highlight-last-saved-node.

[jdtsmith]

Sorry, missed this.

As soon as you visit another node, it loses its "last saved" status.

You mean the last-saved highlight is removed as soon as we move to another node? Why?

No, but when you hit return on a different node, then re-enter vundo, the node you navigated to and saved is no longer recognizable as saved, because vundo has trimmed off the timestamp that undo added to indicate this. There is a potential solution though: see #66. This will also help undo correctly mark a saved buffer state as unmodified (no ** in the modeline) when returning to it.

Could you write vundo--node-saved-ts in plain let and and? That isn't a very good use of and-let IMO.

Sure, done.

Why do we need to change vundo--highlight-node?

Technically we don't; it works identically. I had re-written earlier when it also highlighted the current node with an overlay, and on stripping that part out my version looked cleaner/clearer for the reader. Let me know your preference.

Also, please, fix indentation and trailing whitespace before committing. And I'll be happier if all the comments starts with a capital and ends with a period.

My indentation is always good on this end (lispy enforces that). Do you perhaps use indent-tabs-mode=nil? Assuming that's the case I have untabified and pushed a change.

[jdtsmith]

I found one other little wibble. I should highlight last saved only once, when drawing. Otherwise you can end up with two different "last saved" highlights as you mouse around.

[casouri]

Sorry, missed this.

I took a break too :-) And please allow me to focus on this PR before dealing with the other ones, this is the most important one, and the other PR's seem to warrant complexity not worth the outcome. Anyway, let's first get this done, and we can look at the rest one-by-one.

No, but when you hit return on a different node, then re-enter vundo, the node you navigated to and saved is no longer recognizable as saved, because vundo has trimmed off the timestamp that undo added to indicate this. There is a potential solution though: see https://github.com/casouri/vundo/issues/66. This will also help undo correctly mark a saved buffer state as unmodified (no ** in the modeline) when returning to it.

I agree that we should fix it, but if the solution is too complex or require some hack, it might not worth it. Let's finish this PR and then look at #66.

Technically we don't; it works identically. I had re-written earlier when it also highlighted the current node with an overlay, and on stripping that part out my version looked cleaner/clearer for the reader. Let me know your preference.

Your version is probably cleaner. Still, I'd prefer we don't make stylistic change that doesn't relate to the main change.

My indentation is always good on this end (lispy enforces that). Do you perhaps use indent-tabs-mode=nil? Assuming that's the case I have untabified and pushed a change.

Yeah that must be it. I always set indent-tabs-mode to nil. I also recommend ws-butler for auto whitespace removal, works like a charm.

Also, why highlight the late saved node in vundo--refresh-buffer rather than in vundo--draw-tree?

Mostly for symmetry with the highlighting of the current node, and because we are (for now) special casing the current node when it is unmodified. I could make a helper function vundo--highlight-last-saved-node.

IMO, last saved node is conceptually the same as other saved nodes and should be highlighted in vundo--draw-tree. It is an integral part of the tree like saved nodes, ie, it doesn't change as long as the tree doesn't change. OTOH, current node is not an integral part of the tree and changes all the time, that's why it's highlighted in vundo--refresh-buffer.

[jdtsmith]

And please allow me to focus on this PR before dealing with the other ones, this is the most important one, and the other PR's seem to warrant complexity not worth the outcome

No rush at all. I had time to add/fix a few things over the course of last week, so I submitted them together, but did not expect a parallel review. vundo is such a clean and performant tool, the main criteria as bugs are addressed and features considered is keeping it so. Please take your time to get this right.

I'd prefer we don't make stylistic change that doesn't relate to the main change.

Understood; reversed.

it doesn't change as long as the tree doesn't change.

If we don't implement a fix like #66, this isn't strictly true, since last-saved is a special-casing for buffer-modified=nil, which can change even if the tree doesn't change. Since we have bound vundo-save, this could in fact happen for the same drawn tree (we should probably redraw the tree in vundo-save?).

But I do take your point. I have moved the latest-saved highlighting to vundo--draw-tree. It fits quite nicely there, and naturally solves the "double-highlight" issue I mentioned last evening. On the other hand, since vundo--orig-buffer is not yet set when draw is first called, it now requires a second ORIG-BUFFER argument.

BTW, I'll be traveling and in more sporadic contact over the next couple of weeks, so please don't take slower response as a sign of waning interest.

[jdtsmith]

One other small point to consider: I find the default last-saved-node=bold face to be insufficiently distinct for my font/size. I am using (set-face-foreground 'vundo-last-saved "cyan") and it is much clearer. If you agree, perhaps we should make some more distinct colors by default?

[casouri]

If we don't implement a fix like https://github.com/casouri/vundo/issues/66, this isn't strictly true, since last-saved is a special-casing for buffer-modified=nil, which can change even if the tree doesn't change. Since we have bound vundo-save, this could in fact happen for the same drawn tree (we should probably redraw the tree in vundo-save?).

Gah, that's true. My suggestion is to create a new functionvundo--highlight-last-saved that runs in both vundo--draw-tree and vundo--save. The function would find a remove existing last-saved highlight (using text-property-search-forward) and apply the highlight to the current last saved node.

One other small point to consider: I find the default last-saved-node=bold face to be insufficiently distinct for my font/size. I am using (set-face-foreground 'vundo-last-saved "cyan") and it is much clearer. If you agree, perhaps we should make some more distinct colors by default?

I'd prefer we keep it simple by default. If someone comes to complain (I doubt it), we can think of making it more distinct.

[jdtsmith]

create a new function vundo--highlight-last-saved that runs in both

OK done. I used an overlay instead of searching for and moving the text property. The one peculiarity is that saved nodes, unless they were already recognized as saved when the tree was drawn, do not retain their saved status; they go back to being regular nodes. This is "fair" in the sense that (without something like #66) they will never be recognized as having been saved. If we do something to correct that, we could consider "updating" the saved status of a node while the drawn tree is live.

[casouri]

Thanks! Now, could you squash and force push again, so I can have a look at the patch as a whole?

[jdtsmith]

Anything else in need of attention?

[casouri]

My bad! I didn't see that you pushed. I'll check it out tomorrow!

[casouri]

Merged! I made some minor edits here and there (can't resist the urge...). Thanks for your patience.

[jdtsmith]

This seems to be working fine. Are you planning a new release, or to wait for other feature development?

[casouri]

Yeah I'll make a release soon.