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

Implement "Goto last modification" command #1067

Merged
merged 1 commit into from
Nov 14, 2021

Conversation

ath3
Copy link
Contributor

@ath3 ath3 commented Nov 10, 2021

Another comand i use frequently: Goto last modification (Goto last buffer change in kakoune).
The key is g+; because g+. repeated last insertion.

@ath3 ath3 force-pushed the goto-last-mod branch 2 times, most recently from 9dd941a to 9e8f277 Compare November 11, 2021 00:08
book/src/keymap.md Outdated Show resolved Hide resolved
@@ -133,6 +133,18 @@ impl History {
Some(&self.revisions[last_child.get()].transaction)
}

pub fn last_edit_pos(&mut self) -> Option<usize> {
let current_revision = &self.revisions[self.current];
if let Some(op) = &current_revision.transaction.changes().changes.first() {
Copy link
Member

Choose a reason for hiding this comment

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

Hmm so this is why I didn't implement this yet, first change is not necessarily what we want.

Copy link
Member

Choose a reason for hiding this comment

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

Though I guess we do store the original selection for the revert. Use current_revision.inversion's primary_index to find the matching change (if current_revision.selection.len() == changes.len(), otherwise fallback onto .first)

helix-core/src/history.rs Outdated Show resolved Hide resolved
@@ -96,7 +96,7 @@ pub struct Document {
// It can be used as a cell where we will take it out to get some parts of the history and put
// it back as it separated from the edits. We could split out the parts manually but that will
// be more troublesome.
history: Cell<History>,
pub history: Cell<History>,
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 instead expose a history method that derefs the cell into &History. We don't want it to be externally mutatable

Copy link
Contributor

Choose a reason for hiding this comment

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

Cell can be mutated with an immutable reference.

@ath3 ath3 marked this pull request as draft November 11, 2021 15:47
@ath3 ath3 marked this pull request as ready for review November 11, 2021 16:21
@@ -782,6 +782,11 @@ impl Document {
success
}

// Get reference to history
pub fn history(&self) -> Option<&History> {
unsafe { self.history.as_ptr().as_ref() }
Copy link
Member

Choose a reason for hiding this comment

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

Ah I see, it's using a Cell, not RefCell so you can't return a reference here. Yeah let's avoid unsafe and use your original version with pub history instead 👍🏻

Comment on lines 140 to 141
if let Some(tsel) = current_revision.transaction.selection() {
if let Some(isel) = current_revision.inversion.selection() {
Copy link
Member

Choose a reason for hiding this comment

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

We only need the inversion here which will always have a selection (what the selection was in the document before the change was made).

pub fn last_edit_pos(&self) -> Option<usize> {
    let current_revision = &self.revisions[self.current];
    let primary_selection = current_revision.inversion.selection().expect("inversion always contains a selection").primary();
    let (from, _to, fragment) = current_revision.transaction
        .changes_iter()
        .find(|range| range.overlaps(primary_selection)) // find a change that matches the primary selection
        .or(current_revision.transaction.changes_iter()) // or use the first change
        .unwrap();
    let len = fragment.map(|text| text.chars.count()).unwrap_or(0);
    Some(from + len)
}

@ath3 ath3 force-pushed the goto-last-mod branch 2 times, most recently from a4d26a4 to d62297e Compare November 12, 2021 13:03
Copy link
Member

@archseer archseer left a comment

Choose a reason for hiding this comment

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

Just some minor tweaks, looks good otherwise 👍🏻

.transaction
.changes_iter()
// find a change that matches the primary selection
.find(|range| crate::Range::new(range.0, range.1).overlaps(&primary_selection))
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
.find(|range| crate::Range::new(range.0, range.1).overlaps(&primary_selection))
.find(|(from, to, _fragment)| crate::Range::new(from, to).overlaps(&primary_selection))

A bit clearer

let pos = current_revision
.transaction
.changes()
.map_pos(to, crate::Assoc::After);
Copy link
Member

Choose a reason for hiding this comment

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

Import Assoc at the top of the file

Copy link
Member

@archseer archseer left a comment

Choose a reason for hiding this comment

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

Great work! Tested locally

@archseer archseer merged commit 35c974c into helix-editor:master Nov 14, 2021
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.

3 participants