Repository: helix-editor/helix
File: helix-view/src/view.rs (Rust)

Code before:
            self.current += count;
            self.jumps.get(self.current)
        } else {
            None
        }
    }

    // Taking view and doc to prevent unnecessary cloning when jump is not required.
    pub fn backward(&mut self, view_id: ViewId, doc: &mut Document, count: usize) -> Option<&Jump> {
        if let Some(mut current) = self.current.checked_sub(count) {
            let cur_jump = (doc.id(), doc.selection(view_id).clone());
            if self.current == self.jumps.len() {
                let num_removed = self.push(cur_jump.clone());
                current = current.saturating_sub(num_removed);
            }
            self.current = current;
            self.jumps.get(self.current).and_then(|prev_jump| {
                // If a jumplist position is saved and then `jump_backward` is immediately invoked
                // without moving the cursor, jump back one more step to prevent a no-op
                if prev_jump == &cur_jump {
                    self.current = self.current.saturating_sub(1);
                    self.jumps.get(self.current)
                } else {
                    Some(prev_jump)
                }
            })
        } else {
            None
        }
    }

    pub fn remove(&mut self, doc_id: &DocumentId) {
        self.jumps.retain(|(other_id, _)| other_id != doc_id);
    }

    pub fn iter(&self) -> impl DoubleEndedIterator<Item = &Jump> {
        self.jumps.iter()
    }

    /// Applies a [`Transaction`] of changes to the jumplist.
    /// This is necessary to ensure that changes to documents do not leave jump-list
    /// selections pointing to parts of the text which no longer exist.
    fn apply(&mut self, transaction: &Transaction, doc: &Document) {
        let text = doc.text().slice(..);

        for (doc_id, selection) in &mut self.jumps {
            if doc.id() == *doc_id {
                *selection = selection
                    .clone()
                    .map(transaction.changes())
                    .ensure_invariants(text);

Review comment (suggestion):
Changing the behavior here sounds good to me but the `and_then` can be a little tough to read. Let's use the `?` operator to clean it up and also avoid making another clone of the selection to check equality (i.e. not binding `cur_jump` above):

```rust
// Avoid jumping to the current location.
let jump @ (doc_id, selection) = self.jumps.get(self.current)?;
if doc.id() == *doc_id && doc.selection(view_id) == selection {
    self.current = self.current.checked_sub(1)?;
    self.jumps.get(self.current)
} else {
    Some(jump)
}
```

Code after:

    pub fn forward(&mut self, count: usize) -> Option<&Jump> {
        if self.current + count < self.jumps.len() {
            self.current += count;
            self.jumps.get(self.current)
        } else {
            None
        }
    }

    // Taking view and doc to prevent unnecessary cloning when jump is not required.
    pub fn backward(&mut self, view_id: ViewId, doc: &mut Document, count: usize) -> Option<&Jump> {
        if let Some(mut current) = self.current.checked_sub(count) {
            if self.current == self.jumps.len() {
                let jump = (doc.id(), doc.selection(view_id).clone());
                let num_removed = self.push_impl(jump);
                current = current.saturating_sub(num_removed);
            }
            self.current = current;

            // Avoid jumping to the current location.
            let jump @ (doc_id, selection) = self.jumps.get(self.current)?;
            if doc.id() == *doc_id && doc.selection(view_id) == selection {
                self.current = self.current.checked_sub(1)?;
                self.jumps.get(self.current)
            } else {
                Some(jump)
            }
        } else {
            None
        }
    }

    pub fn remove(&mut self, doc_id: &DocumentId) {
        self.jumps.retain(|(other_id, _)| other_id != doc_id);
    }

    pub fn iter(&self) -> impl DoubleEndedIterator<Item = &Jump> {
        self.jumps.iter()
    }

    /// Applies a [`Transaction`] of changes to the jumplist.
    /// This is necessary to ensure that changes to documents do not leave jump-list
    /// selections pointing to parts of the text which no longer exist.
    fn apply(&mut self, transaction: &Transaction, doc: &Document) {
        let text = doc.text().slice(..);

        for (doc_id, selection) in &mut self.jumps {
            if doc.id() == *doc_id {
                *selection = selection
                    .clone()