Remove the select_* methods from PlainEditorDriver

[ickshonpe]

PlainEditorDriver has about a dozen select_* functions that have matching move_* functions that are identical except that they call set_selection (or whatever) with a true value for its extend parameter. When handling keyboard inputs you check the state of the modifier keys and call the appropriate function:

                    Key::Named(NamedKey::ArrowLeft) => {
                        if action_mod {
                            if shift {
                                drv.select_word_left();
                            } else {
                                drv.move_word_left();
                            }
                        } else if shift {
                            drv.select_left();
                        } else {
                            drv.move_left();
                        }
                    }

This PR removes select_* functions and add an extend parameter to its matching move_* functions instead, which allows the keyboard handling logic to be simplified like so:

                    Key::Named(NamedKey::ArrowLeft) => {
                        if action_mod {
                            drv.move_word_left(shift);
                        } else {
                            drv.move_left(shift);
                        }
                    }

[nicoburns]

This change makes a lot of sense to me. However:

• We should document what the extend parameter does on each of the functions that now has one.
• We will need a changelog entry

[DJMcNab]

This does seem like a reasonable thing to want. My concern is that it does make code using this functionality harder to follow and understand, due to the standard issues around boolean parameters. That doesn't block this change, but it warrants care.

I wonder if there's a better name than move_... which would make this clearer, now that the semantics have changed?

[ickshonpe]

This does seem like a reasonable thing to want. My concern is that it does make code using this functionality harder to follow and understand, due to the standard issues around boolean parameters. That doesn't block this change, but it warrants care.

Yes this bothers me a bit too. I think overall this makes things easier to understand probably though, just because of the smaller API footprint and less branching in user code.

I wonder if there's a better name than move_... which would make this clearer, now that the semantics have changed?

Some half-baked thoughts:

• The parameter could be renamed from extend to extend_selection or with_select or something.
• Enum to replace the bool, obviously. drv.move_left(MovementMode::Select) etc
• Modal selection API. Call drv.start_selection() to clear the current selection and start a new selection block at the current position. Downside: opens a new space for synchronisation bugs.
• Make the API more self-descriptive by giving each of the functions a _with_select postfix. So users would write something like:

                 Key::Named(NamedKey::ArrowLeft) => {
                     if action_mod {
                         drv.move_word_left_with_select(shift);
                     } else {
                         drv.move_left_with_select(shift);
                     }
                 }

• Have a single navigation function that takes a struct with named fields:

                 Key::Named(NamedKey::ArrowLeft) => {
                     if action_mod {
                         drv.move_to(Motion { target: Target::WordLeft, with_select: bool });
                     } else {
                         drv.move_to(Motion { target: Target::Left, with_select: bool });
                     }
                 }

Probably the MovementMode (or whatever name) enum is the least fuss?

[nicoburns]

I like the idea of an enum. Perhaps:

enum SelectionMode {
     /// Updates the current selection by moving the focus point to the specified location
     Update,
     /// Replaces the current selection with a new collapsed selection at the specified location
     Replace, // or Move or Collapse
}

Perhaps then the methods should be select_*?

[ickshonpe]

Perhaps then the methods should be select_*?

Yeah I like that, it carries the right implication, that the cursor's position without a selection is still represented by an empty selection range.

[DJMcNab]

I quite like Collapse; we should likely also have a SelectionMode::update_if(bool)