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

Add :earlier and :later commands that can be used to navigate the full edit history. #194

Merged
merged 30 commits into from
Jun 11, 2021

Conversation

jbaa
Copy link
Contributor

@jbaa jbaa commented Jun 9, 2021

This is a possible implementation of the functionality described in #60. The new :earlier and :later commands can be used to move to previous history revisions. They accept either an integer, e.g. 3 that denotes the number of steps to move back or forward, or a time period e.g. 1s, 5 minutes that denotes how far in time from the currently viewed revision the command should jump.

To enable this change, I modified the change history representation. In its current shape it's similar to kakoune's. It's a vector of revisions, kept in the commit order, each of which keeps a pointer to its parent, forming a tree. Additionally, to support the traditional history, the revisions also keep their "last child" which is the revision that we need to move to in order to execute a redo.

Moves through history using the new commands use a simple algorithm on the history tree. First, the lowest common ancestor of the original and the destination revision is determined. Then we apply the inverted transactions needed to reach that common ancestor from the original revision (equivalent to a series of regular undos using u), and finally we descend to the destination applying the necessary (non-inverted) transactions.

To find the right revision to jump to given a time period, we add/subtract the time period from the commit time of the current revision, and use a binary search on the revision vector to find the revision closest to the destination time. We can do that because we maintain the revision vector is sorted by the commit timestamp.

I tested the feature manually. I think the changes could do with some early feedback. At least some unit tests would be good to add before submitting.

Jakub Bartodziej and others added 11 commits June 3, 2021 08:51
the closest history state to the commit of the current time minus that
interval. Current time is now by default, or the commit time if :before
has just been used.
the edit history and retrieve changes hidded by undoing
and commiting new changes. The commands accept a number
of steps or a time period relative to the currrent change.
Cargo.lock Outdated Show resolved Hide resolved
helix-core/src/lib.rs Outdated Show resolved Hide resolved
helix-term/src/commands.rs Outdated Show resolved Hide resolved
helix-core/src/history.rs Outdated Show resolved Hide resolved
helix-core/src/history.rs Outdated Show resolved Hide resolved
helix-core/src/history.rs Outdated Show resolved Hide resolved
helix-core/src/history.rs Outdated Show resolved Hide resolved
helix-term/src/commands.rs Outdated Show resolved Hide resolved
helix-term/src/commands.rs Outdated Show resolved Hide resolved
helix-core/src/history.rs Outdated Show resolved Hide resolved
helix-term/src/commands.rs Outdated Show resolved Hide resolved
helix-term/src/commands.rs Outdated Show resolved Hide resolved
@pickfire
Copy link
Contributor

pickfire commented Jun 9, 2021

Looks good to me. @jbaa Thanks a lot. Can you please resolve some of my comments, I will follow up on the parsing and the keymap.

@jbaa
Copy link
Contributor Author

jbaa commented Jun 10, 2021

Everything should be resolved now.

helix-core/src/history.rs Outdated Show resolved Hide resolved
helix-core/src/history.rs Outdated Show resolved Hide resolved
helix-core/src/history.rs Outdated Show resolved Hide resolved
helix-core/src/history.rs Outdated Show resolved Hide resolved
helix-core/src/history.rs Outdated Show resolved Hide resolved
@pickfire
Copy link
Contributor

I tested it locally, looks good and works like a charm. The code is good but I left some comments on how it can be improved.

@pickfire
Copy link
Contributor

By the way, any idea what we should use for the keys? Vim have g+, g- and kakoune have alt-u, alt-U, maybe we can follow kakoune since this most of the time we rarely need to do this?

@jbaa
Copy link
Contributor Author

jbaa commented Jun 10, 2021

I think alt-u and alt-U is reasonable (although it's a slight annoyance that both alt and shift need to be pressed at the same time). I don't like g+ and g-, because you always need two strokes (three to input plus) whereas with the kakoune-style keybindings you can hold alt (and maybe shift) pressed and keep hitting u until you reach your destination.

helix-core/src/history.rs Outdated Show resolved Hide resolved
helix-core/src/history.rs Outdated Show resolved Hide resolved
Co-authored-by: Ivan Tham <pickfire@riseup.net>
Comment on lines +278 to +283
let time_unit = TIME_UNITS
.iter()
.enumerate()
.find(|(_, (forms, _, _))| forms.iter().any(|f| f == &unit_str));

if let Some((i, (_, unit, mul))) = time_unit {
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't quite like this i thing and the additional enumerate, it makes it looks a bit ugly. Maybe we can put 0b0001..0b1000 for the time units so we can keep a single usize to check if they are used. We can do like if specified & time_unit.i and specified |= time_unit.i. Then we can remove the enumerate and specified[i] and just do a find.

What do you think?

Copy link
Member

Choose a reason for hiding this comment

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

Let's just keep it as is for the time being, If we go with binary patterns we'd probably have to use something like bitflags to make it more readable

@pickfire
Copy link
Contributor

Thanks a lot but just one last thing. I was a bit tired when I reviewed yesterday I guess I missed that part out.

@archseer archseer merged commit 69fe46a into helix-editor:master Jun 11, 2021
@archseer
Copy link
Member

Thanks @jbaa! 🎉

@archseer archseer mentioned this pull request Jun 11, 2021
@pickfire
Copy link
Contributor

Thanks a lot @jbaa.

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.

None yet

4 participants