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

De-traitify text module #515

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

De-traitify text module #515

wants to merge 2 commits into from

Conversation

PoignardAzur
Copy link
Contributor

No description provided.

Copy link
Member

@DJMcNab DJMcNab left a comment

Choose a reason for hiding this comment

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

I agree with this change. Not approving as PR is draft

@@ -675,27 +616,43 @@ impl<Str: Deref<Target = str> + TextStorage> Selectable for Str {
}
}

/// A cursor type that implements `EditableTextCursor` for string types
/// A cursor type with helper methods for working with strings.
#[derive(Debug)]
pub struct StringCursor<'a> {
Copy link
Member

Choose a reason for hiding this comment

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

Why is this still pub? It has no public API except for construction.

Comment on lines +653 to +654
/// Move cursor to previous codepoint boundary, if it exists.
/// Returns previous codepoint as usize offset.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
/// Move cursor to previous codepoint boundary, if it exists.
/// Returns previous codepoint as usize offset.
/// Move cursor to previous codepoint boundary, if it exists.
/// Returns previous codepoint as usize offset, or `None` if this cursor was already at the first boundary.

Comment on lines 228 to 235
// TODO: This is *definitely* empty
#[cfg(FALSE)]
if !self.text_layout.text().links().is_empty() {
tracing::warn!("Links present in text, but not yet integrated");
}
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// TODO: This is *definitely* empty
#[cfg(FALSE)]
if !self.text_layout.text().links().is_empty() {
tracing::warn!("Links present in text, but not yet integrated");
}
// When we add links to `Prose`, they will probably need to be handled here.

Comment on lines +208 to 211
#[cfg(FALSE)]
if !self.text_layout.text().links().is_empty() {
tracing::warn!("Links present in text, but not yet integrated");
}
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
#[cfg(FALSE)]
if !self.text_layout.text().links().is_empty() {
tracing::warn!("Links present in text, but not yet integrated");
}

Label will never support links

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