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

Add basic support for text highlighting #1276

Closed
wants to merge 7 commits into from

Conversation

BigWingBeat
Copy link

Adds support for changing the background colour of various parts of text, to enable things like highlighting search results, marking outstanding regions of text, etc.

You could achieve a similar effect before this PR by splitting your text into multiple Text structs and nesting the parts you want to highlight in a Container with a background colour set, but this is not a viable solution as doing so interferes with text layouting, as demonstrated in the below videos.

Text highlighting using the aforementioned containers-based method (Notice how the text fidgets around as the highlighting changes):

/* Snip */
let splitter = &self.highlight_input_value;
                            
let mut row = Row::new();
let mut split = s.split(splitter);
                            
if let Some(first) = split.next() {
    row = row.push(Text::new(first));
}
                            
for s in split {
    row = row
        .push(
            Container::new(Text::new(splitter))
                .style(ContainerStyle::red()),
        )
        .push(Text::new(s));
}
/* Snip */
before.mp4

Text highlighting using the highlight method introduced by this PR:

/* Snip */
let mut text = Text::new(s);
for (index, _) in s.match_indices(&self.highlight_input_value) {
    text = text.highlight(
        index,
        index + self.highlight_input_value.len(),
        Color::from_rgb8(0xff, 0xc0, 0xcb),
    );
}
/* Snip */
after.mp4

let quad = renderer::Quad {
bounds: Rectangle {
x: x + width_before_start,
y,
Copy link
Member

Choose a reason for hiding this comment

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

It seems highlighting will not work with a Text of multiple lines?

Copy link
Author

Choose a reason for hiding this comment

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

Yeah, I should clarify that this a very basic and naive implementation thus far; there's a lot of trivial cases that break it.

@hecrj
Copy link
Member

hecrj commented Mar 6, 2022

I think this is related to #156.

We are not only interested in highlighthing support, but also changing color, font, size, and other text properties without affecting layout.

@BigWingBeat BigWingBeat changed the title Add support for text highlighting Add basic support for text highlighting Mar 7, 2022
renderer.measure(above_lines, size, font.clone(), Size::INFINITY);

// If the highlight crosses over multiple lines, draw a seperate rect on each line
// BUG: This ignores single `\r` but Iced's text layouting does not (See above)
Copy link
Author

Choose a reason for hiding this comment

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

Rust only considers \n and \r\n as line endings, but Iced's text layouting also considers a standalone \r as a line ending.

This isn't exactly a hard blocker, but I'm unaware of the consensus of if this behaviour should be kept or not.

// If the highlight crosses over multiple lines, draw a seperate rect on each line
// BUG: This ignores single `\r` but Iced's text layouting does not (See above)
// BUG #2: Text wrapping caused by the text not being given wide enough bounds is not handled at all
// (And furthermore it currently _can't_ be handled because there's no way to get information about it)
Copy link
Author

Choose a reason for hiding this comment

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

If there is a way to handle this, please correct me, but if not, this would be a blocker on ensuring correct behaviour.

@BigWingBeat
Copy link
Author

It's worth considering what the API for this should look like. If Iced ever gets full rich text support, this will likely be integrated with that, but until then this will need its own API.

Currently the API accepts grapheme indexes; graphemes are the most intuitive way to deal with partitioning rendered text, and indeed, there is no intuitive answer as to what a highlight should look like if it were to be specified by byte or char index instead, and ended up cutting a grapheme in half.

The issue with this is the std library provides no way to observe strings via their graphemes (Iced itself uses this crate), instead dealing with bytes and chars . This is a problem as I think it would be reasonable to say that for most use cases of text highlighting, the region of text to highlight would be calculated programmatically, and thus would likely be represented (on the user's end) as bytes or chars, rather than graphemes (Unless the user specifically uses a grapheme processing crate).

The API could be changed to accept bytes or char`s, but that would either require extra input sanitisation to handle inputs that don't conform to a grapheme boundary (Do we silently ignore, return an error or panic?), or somehow interpreting what highlights would look like if they start or end in the middle of a grapheme.

On the other hand, keeping the API as-is would pass these burdens on to the user, requiring them to convert their data into grapheme indexes. We could expose helper methods to aid with this, but doing so could be scope creep.

I am certainly not acquainted enough with Iced's architecture and design philosophies to make any kind of informed decision on my own (Not only about API but the other topics mentioned in my above comments), so I'm willing to delegate to the opinion of someone more familiar with Iced.

@cryptoquick
Copy link

@Pixelstormer What do you think of this crate?
https://crates.io/crates/unicode-segmentation

The .unicode_words() method seems good enough in the case of highlighting, don't you think?

@hecrj
Copy link
Member

hecrj commented Oct 3, 2022

I think there is still a lot of discussion that needs to happen before we can start thinking about code here.

I have some text rendering improvements coming soon in the pipeline. I am aware I am the main bottleneck here! Once those improvements land, we can review this approach and evaluate if it's suitable or whether we can reuse any parts or not.

@hecrj hecrj closed this Oct 3, 2022
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.

3 participants