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.

Sign up for GitHub

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

De-traitify text module #515

Merged
merged 6 commits into from
Sep 23, 2024
Merged

De-traitify text module #515

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
9220226
De-traitify text module
PoignardAzur Aug 15, 2024
d5e5dab
Apply suggestions from code review
PoignardAzur Sep 19, 2024
a5082f7
Run cargo fmt
PoignardAzur Sep 19, 2024
7764a70
Fix clippy lint
PoignardAzur Sep 19, 2024
d122341
Update VariableLabel
PoignardAzur Sep 19, 2024
771dab5
Tweaks to address code review
PoignardAzur Sep 23, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 13 additions & 7 deletions masonry/src/text/backspace.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,14 +5,14 @@

use xi_unicode::*;

use super::{EditableTextCursor, Selectable};
use crate::text::StringCursor;

/// Logic adapted from Android and
/// <https://github.com/xi-editor/xi-editor/pull/837>
/// See links present in that PR for upstream Android Source
/// Matches Android Logic as at 2024-05-10
#[allow(clippy::cognitive_complexity)]
fn backspace_offset(text: &impl Selectable, start: usize) -> usize {
fn backspace_offset(text: &str, start: usize) -> usize {
#[derive(PartialEq)]
enum State {
Start,
Expand All @@ -34,9 +34,15 @@ fn backspace_offset(text: &impl Selectable, start: usize) -> usize {

let mut delete_code_point_count = 0;
let mut last_seen_vs_code_point_count = 0;
let mut cursor = text
.cursor(start)
.expect("Backspace must begin at a valid codepoint boundary.");

let mut cursor = StringCursor {
text,
position: start,
};
assert!(
cursor.is_boundary(),
"Backspace must begin at a valid codepoint boundary."
);

while state != State::Finished && cursor.pos() > 0 {
let code_point = cursor.prev_codepoint().unwrap_or('0');
Expand Down Expand Up @@ -190,8 +196,8 @@ fn backspace_offset(text: &impl Selectable, start: usize) -> usize {
/// This involves complicated logic to handle various special cases that
/// are unique to backspace.
#[allow(clippy::trivially_copy_pass_by_ref)]
pub fn offset_for_delete_backwards(caret_position: usize, text: &impl Selectable) -> usize {
backspace_offset(text, caret_position)
pub fn offset_for_delete_backwards(caret_position: usize, text: &impl AsRef<str>) -> usize {
backspace_offset(text.as_ref(), caret_position)
}
Comment on lines +199 to 201
Copy link
Member

Choose a reason for hiding this comment

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

I'd probably just delete this function (to be clear, this same comment applies to the old version as well...)


#[cfg(test)]
Expand Down
110 changes: 35 additions & 75 deletions masonry/src/text/edit.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,47 +23,15 @@ use super::{
Selectable, TextBrush, TextWithSelection,
};

/// Text which can be edited
pub trait EditableText: Selectable {
/// Replace range with new text.
/// Can panic if supplied an invalid range.
// TODO: make this generic over Self
fn edit(&mut self, range: Range<usize>, new: impl Into<String>);
/// Create a value of this struct
fn from_str(s: &str) -> Self;
}

impl EditableText for String {
fn edit(&mut self, range: Range<usize>, new: impl Into<String>) {
self.replace_range(range, &new.into());
}
fn from_str(s: &str) -> Self {
s.to_string()
}
}

// TODO: What advantage does this actually have?
// impl EditableText for Arc<String> {
// fn edit(&mut self, range: Range<usize>, new: impl Into<String>) {
// let new = new.into();
// if !range.is_empty() || !new.is_empty() {
// Arc::make_mut(self).edit(range, new)
// }
// }
// fn from_str(s: &str) -> Self {
// Arc::new(s.to_owned())
// }
// }

/// A region of text which can support editing operations
pub struct TextEditor<T: EditableText> {
inner: TextWithSelection<T>,
pub struct TextEditor {
inner: TextWithSelection<String>,
/// The range of the preedit region in the text
preedit_range: Option<Range<usize>>,
}

impl<T: EditableText> TextEditor<T> {
pub fn new(text: T, text_size: f32) -> Self {
impl TextEditor {
pub fn new(text: String, text_size: f32) -> Self {
Self {
inner: TextWithSelection::new(text, text_size),
preedit_range: None,
Expand Down Expand Up @@ -122,11 +90,11 @@ impl<T: EditableText> TextEditor<T> {
Key::Named(NamedKey::Backspace) => {
if let Some(selection) = self.inner.selection {
if !selection.is_caret() {
self.text_mut().edit(selection.range(), "");
self.text_mut().replace_range(selection.range(), "");
self.inner.selection =
Some(Selection::caret(selection.min(), Affinity::Upstream));

let contents = self.text().as_str().to_string();
let contents = self.text().clone();
ctx.submit_action(Action::TextChanged(contents));
} else {
// TODO: more specific behavior may sometimes be warranted here
Expand All @@ -136,11 +104,11 @@ impl<T: EditableText> TextEditor<T> {
let text = self.text_mut();
let offset =
offset_for_delete_backwards(selection.active, text);
self.text_mut().edit(offset..selection.active, "");
self.text_mut().replace_range(offset..selection.active, "");
self.inner.selection =
Some(Selection::caret(offset, selection.active_affinity));

let contents = self.text().as_str().to_string();
let contents = self.text().clone();
ctx.submit_action(Action::TextChanged(contents));
}
Handled::Yes
Expand All @@ -151,24 +119,24 @@ impl<T: EditableText> TextEditor<T> {
Key::Named(NamedKey::Delete) => {
if let Some(selection) = self.inner.selection {
if !selection.is_caret() {
self.text_mut().edit(selection.range(), "");
self.text_mut().replace_range(selection.range(), "");
self.inner.selection = Some(Selection::caret(
selection.min(),
Affinity::Downstream,
));

let contents = self.text().as_str().to_string();
let contents = self.text().clone();
ctx.submit_action(Action::TextChanged(contents));
} else if let Some(offset) =
self.text().next_grapheme_offset(selection.active)
{
self.text_mut().edit(selection.min()..offset, "");
self.text_mut().replace_range(selection.min()..offset, "");
self.inner.selection = Some(Selection::caret(
selection.min(),
selection.active_affinity,
));

let contents = self.text().as_str().to_string();
let contents = self.text().clone();
ctx.submit_action(Action::TextChanged(contents));
}
Handled::Yes
Expand All @@ -184,18 +152,19 @@ impl<T: EditableText> TextEditor<T> {
h_pos: None,
});
let c = ' ';
self.text_mut().edit(selection.range(), c);
self.text_mut()
.replace_range(selection.range(), &c.to_string());
self.inner.selection = Some(Selection::caret(
selection.min() + c.len_utf8(),
// We have just added this character, so we are "affined" with it
Affinity::Downstream,
));
let contents = self.text().as_str().to_string();
let contents = self.text().clone();
ctx.submit_action(Action::TextChanged(contents));
Handled::Yes
}
Key::Named(NamedKey::Enter) => {
let contents = self.text().as_str().to_string();
let contents = self.text().clone();
ctx.submit_action(Action::TextEntered(contents));
Handled::Yes
}
Expand All @@ -219,17 +188,17 @@ impl<T: EditableText> TextEditor<T> {
Key::Named(NamedKey::Backspace) => {
if let Some(selection) = self.inner.selection {
if !selection.is_caret() {
self.text_mut().edit(selection.range(), "");
self.text_mut().replace_range(selection.range(), "");
self.inner.selection =
Some(Selection::caret(selection.min(), Affinity::Upstream));
}
let offset =
self.text().prev_word_offset(selection.active).unwrap_or(0);
self.text_mut().edit(offset..selection.active, "");
self.text_mut().replace_range(offset..selection.active, "");
self.inner.selection =
Some(Selection::caret(offset, Affinity::Upstream));

let contents = self.text().as_str().to_string();
let contents = self.text().clone();
ctx.submit_action(Action::TextChanged(contents));
Handled::Yes
} else {
Expand All @@ -239,19 +208,19 @@ impl<T: EditableText> TextEditor<T> {
Key::Named(NamedKey::Delete) => {
if let Some(selection) = self.inner.selection {
if !selection.is_caret() {
self.text_mut().edit(selection.range(), "");
self.text_mut().replace_range(selection.range(), "");
self.inner.selection = Some(Selection::caret(
selection.min(),
Affinity::Downstream,
));
} else if let Some(offset) =
self.text().next_word_offset(selection.active)
{
self.text_mut().edit(selection.active..offset, "");
self.text_mut().replace_range(selection.active..offset, "");
self.inner.selection =
Some(Selection::caret(selection.min(), Affinity::Upstream));
}
let contents = self.text().as_str().to_string();
let contents = self.text().clone();
ctx.submit_action(Action::TextChanged(contents));
Handled::Yes
} else {
Expand All @@ -268,20 +237,21 @@ impl<T: EditableText> TextEditor<T> {
TextEvent::Ime(ime) => match ime {
Ime::Commit(text) => {
if let Some(selection_range) = self.selection.map(|x| x.range()) {
self.text_mut().edit(selection_range.clone(), text);
self.text_mut().replace_range(selection_range.clone(), text);
self.selection = Some(Selection::caret(
selection_range.start + text.len(),
Affinity::Upstream,
));
}
let contents = self.text().as_str().to_string();
let contents = self.text().clone();
ctx.submit_action(Action::TextChanged(contents));
Handled::Yes
}
Ime::Preedit(preedit_string, preedit_sel) => {
if let Some(preedit) = self.preedit_range.clone() {
// TODO: Handle the case where this is the same value, to avoid some potential infinite loops
self.text_mut().edit(preedit.clone(), preedit_string);
self.text_mut()
.replace_range(preedit.clone(), preedit_string);
let np = preedit.start..(preedit.start + preedit_string.len());
self.preedit_range = if preedit_string.is_empty() {
None
Expand All @@ -308,7 +278,7 @@ impl<T: EditableText> TextEditor<T> {
return Handled::Yes;
}
let sr = self.selection.map(|x| x.range()).unwrap_or(0..0);
self.text_mut().edit(sr.clone(), preedit_string);
self.text_mut().replace_range(sr.clone(), preedit_string);
let np = sr.start..(sr.start + preedit_string.len());
self.preedit_range = if preedit_string.is_empty() {
None
Expand All @@ -330,7 +300,7 @@ impl<T: EditableText> TextEditor<T> {
Ime::Enabled => {
// Generally this shouldn't happen, but I can't prove it won't.
if let Some(preedit) = self.preedit_range.clone() {
self.text_mut().edit(preedit.clone(), "");
self.text_mut().replace_range(preedit.clone(), "");
self.selection = Some(
self.selection
.unwrap_or(Selection::caret(0, Affinity::Upstream)),
Expand All @@ -341,7 +311,7 @@ impl<T: EditableText> TextEditor<T> {
}
Ime::Disabled => {
if let Some(preedit) = self.preedit_range.clone() {
self.text_mut().edit(preedit.clone(), "");
self.text_mut().replace_range(preedit.clone(), "");
self.preedit_range = None;
let sm = self.selection.map(|x| x.min()).unwrap_or(0);
if preedit.contains(&sm) {
Expand All @@ -364,49 +334,39 @@ impl<T: EditableText> TextEditor<T> {
active_affinity: Affinity::Downstream,
h_pos: None,
});
self.text_mut().edit(selection.range(), &**c);
self.text_mut().replace_range(selection.range(), c);
self.inner.selection = Some(Selection::caret(
selection.min() + c.len(),
// We have just added this character, so we are "affined" with it
Affinity::Downstream,
));
let contents = self.text().as_str().to_string();
let contents = self.text().clone();
ctx.submit_action(Action::TextChanged(contents));
Handled::Yes
}
}

impl<T: EditableText> Deref for TextEditor<T> {
type Target = TextWithSelection<T>;
impl Deref for TextEditor {
type Target = TextWithSelection<String>;
Copy link
Member

Choose a reason for hiding this comment

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

I suspect we might want to remove this Deref at some point - it's a bit messy. Not a job for this PR


fn deref(&self) -> &Self::Target {
&self.inner
}
}

// TODO: Being able to call `Self::Target::rebuild` (and `draw`) isn't great.
impl<T: EditableText> DerefMut for TextEditor<T> {
impl DerefMut for TextEditor {
fn deref_mut(&mut self) -> &mut Self::Target {
&mut self.inner
}
}

#[cfg(test)]
mod tests {
use super::EditableText;

// #[test]
// fn arcstring_empty_edit() {
// let a = Arc::new("hello".to_owned());
// let mut b = a.clone();
// b.edit(5..5, "");
// assert!(Arc::ptr_eq(&a, &b));
// }

#[test]
fn replace() {
let mut a = String::from("hello world");
a.edit(1..9, "era");
a.replace_range(1..9, "era");
assert_eq!("herald", a);
}
}
Loading