Skip to content

fix: selection in emacs/viins mode #911

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

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from
Draft

Conversation

blindFS
Copy link
Contributor

@blindFS blindFS commented May 23, 2025

fixes #893

A not-so-elegant solution coded by AI agent, just works according to my test.

Copy link
Member

@sholderbach sholderbach left a comment

Choose a reason for hiding this comment

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

Thanks for trying to tackle this (and not just doing the revert game) Haven't run it yet so I can't tell if the AI is doing slop here.

If it works I don't mind the use of it but maybe you could go through the tests and check which are truly necessary to cover the relevant bits and be explanatory, but we don't need too much redundant prose in the form of tests.

Comment on lines +629 to +630
// Default to Vi-style behavior for backward compatibility
self.get_selection_with_mode(&PromptEditMode::Vi(PromptViMode::Normal))
Copy link
Member

Choose a reason for hiding this comment

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

Is there an explicit need to be backwards compatible here?

Maybe the correct thing would be to split it into true selection (which covers the characters in a consistent way with a range using indices used by the edit ops), and visual selection which is mode aware.

It seems the bug prone part was the visual selection, if there are no other users in reedline itself we should consider what the right behavior would be for a public user of get_selection (and if that is unlikely we can make things private.

@blindFS blindFS marked this pull request as draft May 24, 2025 09:45
@collinmurch
Copy link

#926

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.

Line editor selects two characters instead of one
3 participants