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

Expose selection_start #81

Merged
merged 3 commits into from
Aug 8, 2024
Merged

Conversation

achristmascarl
Copy link
Contributor

for #80; it is useful information to have access to when the direction of the selection matters

/// ```
pub fn selection_start(&self) -> Option<(usize, usize)> {
self.selection_start
}
Copy link
Owner

@rhysd rhysd Aug 7, 2024

Choose a reason for hiding this comment

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

I feel the following method is a better interface because the word start is a bit confusing when the cursor is before the start position. I think that users want to obtain the range of the selected text rather than a single (row, col) position.

pub fn selection_range(&self) -> Option<((usize, usize), (usize, usize))> {
    // Return (row, col) pair of selection range
}

What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think that makes sense!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

actually, @rhysd selection_range() conflicts with the private fn which has a different behavior:

tui-textarea/src/textarea.rs

Lines 1433 to 1444 in 93cfabd

fn selection_range(&self) -> Option<(Pos, Pos)> {
let (sr, sc) = self.selection_start?;
let (er, ec) = self.cursor;
let (so, eo) = (self.line_offset(sr, sc), self.line_offset(er, ec));
let s = Pos::new(sr, sc, so);
let e = Pos::new(er, ec, eo);
match (sr, so).cmp(&(er, eo)) {
Ordering::Less => Some((s, e)),
Ordering::Equal => None,
Ordering::Greater => Some((e, s)),
}
}

i guess we could call the public fn something like unordered_selection_range() or selection_range_raw()? although those are a bit unwieldy

Copy link
Owner

@rhysd rhysd Aug 7, 2024

Choose a reason for hiding this comment

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

Oops, please just rename the private method to selection_range_ (or selection_range_impl if clippy gets unhappy). Then I will rename it to more proper name after merging this PR.

Copy link
Owner

@rhysd rhysd left a comment

Choose a reason for hiding this comment

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

Thanks!

@rhysd rhysd merged commit b7a7e3d into rhysd:main Aug 8, 2024
5 checks passed
@achristmascarl achristmascarl deleted the expose-selection-start branch August 8, 2024 01:12
@rhysd
Copy link
Owner

rhysd commented Aug 8, 2024

@achristmascarl I modified the API to always make start <= end where let (start, end) = textarea.selection_range() because it is natural to put a smaller value at the left side of the range like a..b.

For your use case, please do the following trick to get the selection start.

let (start, end) = textarea.selection_range();
let selection_start = if start == textarea.cursor() { end } else { start };

@achristmascarl
Copy link
Contributor Author

ok, that makes sense 👍

@rhysd
Copy link
Owner

rhysd commented Aug 8, 2024

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.

2 participants