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

readline style insert mode #1039

Merged
merged 11 commits into from
Nov 15, 2021
Merged

readline style insert mode #1039

merged 11 commits into from
Nov 15, 2021

Conversation

QiBaobin
Copy link
Contributor

@QiBaobin QiBaobin commented Nov 9, 2021

Add readline style maps into insert mode

@@ -563,6 +567,16 @@ fn extend_to_line_start(cx: &mut Context) {
goto_line_start_impl(view, doc, Movement::Extend)
}

fn kill_to_line_start(cx: &mut Context) {
extend_to_line_start(cx);
delete_selection(cx);
Copy link
Member

Choose a reason for hiding this comment

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

You probably want to use delete_selection_impl for these:

fn delete_selection_impl(reg: &mut Register, doc: &mut Document, view_id: ViewId) {
let text = doc.text().slice(..);
let selection = doc.selection(view_id);
// first yank the selection
let values: Vec<String> = selection.fragments(text).map(Cow::into_owned).collect();
reg.write(values);
// then delete
let transaction = Transaction::change_by_selection(doc.text(), selection, |range| {
(range.from(), range.to(), None)
});
doc.apply(&transaction, view_id);
}
fn delete_selection(cx: &mut Context) {
let reg_name = cx.register.unwrap_or('"');
let (view, doc) = current!(cx.editor);
let registers = &mut cx.editor.registers;
let reg = registers.get_mut(reg_name);
delete_selection_impl(reg, doc, view.id);
doc.append_changes_to_history(view.id);
// exit select mode, if currently in select mode
exit_select_mode(cx);
}

delete_selection will also store all the changes into history, but in insert mode we don't want to do that until insert mode is exited.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is it OK if I add a flag in delete_selection as it contains some common logic? Or it's better that I wrote a wrapper like current delete_selection?

fn delete_selection(cx: &mut Context) {
    delete_selection_and_append_history(cx, true);
}

fn delete_selection_and_append_history(cx: &mut Context, append_change_to_history: bool) {
    let reg_name = cx.register.unwrap_or('"');
    let (view, doc) = current!(cx.editor);
    let registers = &mut cx.editor.registers;
    let reg = registers.get_mut(reg_name);
    delete_selection_impl(reg, doc, view.id);

    if append_change_to_history {
        doc.append_changes_to_history(view.id);
    }

    // exit select mode, if currently in select mode
    exit_select_mode(cx);
}

Copy link
Member

@archseer archseer Nov 10, 2021

Choose a reason for hiding this comment

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

We don't want the extra register logic though:

If I have: hello [world] and I press c, we get hello | with " set to world. Now if you use kill_to_line_start that would change the register. But we want to keep world.

Copy link
Member

Choose a reason for hiding this comment

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

So using only delete_selection_impl should be enough

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done, thanks for the finding, please take another look.

doc.apply(&transaction, view_id);

// exit select mode, if currently in select mode
exit_select_mode(cx);
Copy link
Member

Choose a reason for hiding this comment

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

Please just use delete_selection_impl. In insert mode we don't want to do any logic related to exiting select mode.

Copy link
Contributor Author

@QiBaobin QiBaobin Nov 10, 2021

Choose a reason for hiding this comment

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

The delete_selection_impl would yank the text to a register, is there a _ register for this? And I think we need create a selection to use delete_selection_impl too, does it mean that we already touch selection data?

fn delete_selection_impl(reg: &mut Register, doc: &mut Document, view_id: ViewId) {
    let text = doc.text().slice(..);
    let selection = doc.selection(view_id);

    // first yank the selection
    let values: Vec<String> = selection.fragments(text).map(Cow::into_owned).collect();
    reg.write(values);

    // then delete
    let transaction = Transaction::change_by_selection(doc.text(), selection, |range| {
        (range.from(), range.to(), None)
    });
    doc.apply(&transaction, view_id);
}

Copy link
Member

Choose a reason for hiding this comment

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

Hmm, maybe use

    let transaction = Transaction::change_by_selection(doc.text(), selection, |range| {
        (range.from(), range.to(), None)
    });
    doc.apply(&transaction, view_id);

directly in your implementations then?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's still need to make a selection to pass in change_by_selection, right? So I just need remove the exit_select_mode(cx);?

Comment on lines 3872 to 3877
let selection = doc
.selection(view.id)
.clone()
.transform(|range| movement::move_next_word_start(text, range, count));
doc.set_selection(view.id, selection);
delete_selection_insert_mode(cx)
Copy link
Member

Choose a reason for hiding this comment

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

Yeah you still need to build a selection, but rather than use set_selection you can just use it directly:

Suggested change
let selection = doc
.selection(view.id)
.clone()
.transform(|range| movement::move_next_word_start(text, range, count));
doc.set_selection(view.id, selection);
delete_selection_insert_mode(cx)
let selection = doc
.selection(view.id)
.clone()
.transform(|range| movement::move_next_word_start(text, range, count));
// then delete
let transaction = Transaction::change_by_selection(doc.text(), selection, |range| {
(range.from(), range.to(), None)
});
doc.apply(&transaction, view_id);

You can just inline the delete_selection_insert_mode code.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the suggestions, updated the code.

Comment on lines 1577 to 1587
fn delete_selection_insert_mode(cx: &mut Context, selection: &Selection) {
let (view, doc) = current!(cx.editor);
let view_id = view.id;

// then delete
let transaction = Transaction::change_by_selection(doc.text(), selection, |range| {
(range.from(), range.to(), None)
});
doc.apply(&transaction, view_id);
}

Copy link
Member

Choose a reason for hiding this comment

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

I would just inline this function everywhere, but if you want to avoid that then maybe use delete_selection_insert_mode(doc: &mut Document, view: &View, selection: &Selection), that way we avoid looking up current! again.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point, updated accordingly.

@archseer archseer merged commit 46d9ae2 into helix-editor:master Nov 15, 2021
@QiBaobin QiBaobin deleted the readline branch November 15, 2021 23:50
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

2 participants