-
Notifications
You must be signed in to change notification settings - Fork 10
Adding text truncation logic to support ellipsis. #36
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
base: main
Are you sure you want to change the base?
Conversation
Reviewer's Guide by SourceryThis pull request implements text truncation logic with ellipsis support in the RIL (Rust Image Library) project. It introduces a new File-Level Changes
Tips
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @tillderoquefeuil - I've reviewed your changes - here's some feedback:
Overall Comments:
- Consider adding unit tests for edge cases in the truncation logic, such as very small max heights or extremely long words.
- The comment for the
with_max_height
method is incomplete. Please finish the sentence for better documentation.
Here's what I looked at during the review
- 🟡 General issues: 3 issues found
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟡 Complexity: 1 issue found
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment to tell me if it was helpful.
@@ -232,6 +235,13 @@ impl<'a, P: Pixel> TextSegment<'a, P> { | |||
self | |||
} | |||
|
|||
/// Sets the maximum height of the text segment. If this is set, the text will be ellipsized if. It only works in a [`TextLayout`]. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion: Improve documentation for with_max_height method
The documentation for with_max_height should be more precise. Consider rewording it to clearly state that the ellipsization only occurs when used within a TextLayout.
/// Sets the maximum height of the text segment. If this is set, the text will be ellipsized if. It only works in a [`TextLayout`]. | |
/// Sets the maximum height of the text segment. | |
/// When used within a [`TextLayout`], the text will be ellipsized if it exceeds this height. | |
/// Note: This setting only takes effect when the text is rendered in a [`TextLayout`]. |
/// Adds a text segment to the text layout. | ||
pub fn push_segment(&mut self, segment: &TextSegment<'a, P>) { | ||
let truncated_text = self.truncate_text_with_ellipsis(segment); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion: Add comment explaining truncated_text usage
Consider adding a brief comment explaining why truncated_text is used instead of segment.text directly. This will help future maintainers quickly understand the reasoning behind this change.
// Truncate text if it exceeds layout bounds, adding ellipsis if needed
let truncated_text = self.truncate_text_with_ellipsis(segment);
use ril::prelude::*; | ||
|
||
fn main() -> ril::Result<()> { | ||
let font = Font::open("./examples/assets/Arial.ttf", 12.0)?; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion: Use a more portable way to specify font path
The hardcoded font path might not work on all systems. Consider using a system font or provide instructions on how to make the example work with different font locations to improve portability.
let font_path = std::env::var("FONT_PATH").unwrap_or_else(|_| "./examples/assets/Arial.ttf".to_string());
let font = Font::open(&font_path, 12.0)?;
@@ -495,13 +505,70 @@ impl<'a, P: Pixel> TextLayout<'a, P> { | |||
self | |||
} | |||
|
|||
fn truncate_text_with_ellipsis(&self, segment: &TextSegment<'a, P>) -> String { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
issue (complexity): Consider extracting the binary search logic into a separate function for improved modularity.
The truncate_text_with_ellipsis
function introduces a clever binary search approach to efficiently handle text truncation with a height limit. While the algorithm is efficient, we can improve its readability and maintainability:
- Extract the binary search logic into a separate function:
fn find_truncation_point(text: &str, max_height: f32, font: &Font, size: f32, ellipsis: &str) -> usize {
let mut left = 0;
let mut right = text.len();
while left + 1 < right {
let mid = (left + right) / 2;
let truncated = format!("{}{}", &text[..mid], ellipsis);
let mut layout = Layout::new(CoordinateSystem::PositiveYDown);
layout.reset(&self.settings);
layout.append(&[font.inner()], &TextStyle::new(&truncated, size, 0));
if layout.height() <= max_height {
left = mid;
} else {
right = mid;
}
}
left
}
- Simplify the main function using the extracted helper:
fn truncate_text_with_ellipsis(&self, segment: &TextSegment<'a, P>) -> String {
let Some(max_height) = segment.max_height else {
return segment.text.clone();
};
let ellipsis = "...";
let mut layout = Layout::new(CoordinateSystem::PositiveYDown);
layout.reset(&self.settings);
layout.append(&[segment.font.inner()], &TextStyle::new(&segment.text, segment.size, 0));
if layout.height() <= max_height as f32 {
return segment.text.clone();
}
let truncation_point = find_truncation_point(&segment.text, max_height as f32, segment.font, segment.size, ellipsis);
if truncation_point == 0 {
ellipsis.to_string()
} else {
let trimmed_text = segment.text[..truncation_point].trim_end();
format!("{trimmed_text}{ellipsis}")
}
}
- Add a comment explaining the binary search approach:
// We use a binary search to efficiently find the optimal truncation point.
// This approach is particularly effective for longer text strings, as it
// logarithmically reduces the number of layout calculations needed.
let truncation_point = find_truncation_point(&segment.text, max_height as f32, segment.font, segment.size, ellipsis);
These changes maintain the efficient binary search algorithm while improving code readability and maintainability. The extracted function can be easily unit tested, and the main function is now more focused and easier to understand.
This commit introduces a new method
with_max_height
in theTextSegment
struct, which allows setting the max height of the text. This method works only if the segment is in aTextLayout
. The commit also includes an example usage of thewith_max_height
method in theellipsis.rs
file.All tests still work, the code is formatted and clippy does not show error on added code.
Summary by Sourcery
Add text truncation logic to the
TextSegment
struct by introducing awith_max_height
method, allowing text to be truncated with an ellipsis if it exceeds the specified height. Implement the truncation logic in theTextLayout
and provide an example usage inellipsis.rs
.New Features:
with_max_height
in theTextSegment
struct to allow setting a maximum height for text, enabling text truncation with ellipsis when the text exceeds the specified height.Enhancements:
truncate_text_with_ellipsis
function in theTextLayout
to handle text truncation when the maximum height is set, ensuring text is truncated with an ellipsis if it exceeds the specified height.Tests:
with_max_height
method in theellipsis.rs
file to demonstrate the new text truncation feature.