-
Notifications
You must be signed in to change notification settings - Fork 247
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
Text Style #106
Text Style #106
Conversation
9d2e127
to
90c7d21
Compare
2410f1c
to
9e931dd
Compare
add style module: Style, StyledStr add Printer::print_styled() (proof of concept, should probably be cleaned up) impl Default for Effect, derive Debug for ColorStyle
Printer::print() now returns the unicode-width of the text printed
/// # let text_styled: &[StyledString] = &[].as_ref(); | ||
/// let text_plain = text_styled.to_plain(); | ||
/// let spans = StyledLinesIterator::make_spans(text_styled); | ||
/// // You can now keep `text_plain` and `spans` alive while using `iter`. |
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.
I don't really like this but couldn't figure out a way to avoid it.
We need to keep the string that the LinesIterator
uses alive since it is not owned by the StyledLinesIterator
.
Because the parsed StyledStrings
use Cows
now and not simply slices, the user also needs to join them together into text_plain
at some point so he can use StyledSpan
s against it later.
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.
The problem is that we mix direct indices in a string (StyledSpan
) and &str
(StyledString
), and it's not easy to move from one to the other.
Since we can now print content which is not directly present in the input (because of escaping), the indices from StyledSpan
are no longer a great solution. Keeping a Cow
in the StyledSpan
sounds easier.
Actually, I think we could merge StyledSpan
and StyledString
. Both refer to a "styled" section from the input text, potentially "rewritten" to account for escaping (hence the Cow
).
Then, we'd have:
fn parse(input: &'a str) -> Vec<StyledSpan<'a>>
fn compute_rows(spans: &'b [StyledSpan<'a>]) -> Vec<StyledSpan<'a>>
For the compute_rows
, I'm thinking of updating the existing LinesIterator
rather than wrapping it. I'm not a big fan of StyledLinesIterator
taking both the plain text and the spans, and hoping that they'll match. Especially since it doesn't need the plain text.
I'll try to work on updating LinesIterator
for that this week if I find some time.
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.
(*) EDIT: It seems I forgot that to find line breaks it uses LinesIterator
which needs the plain string to index into. That busts my idea... Rust can be tough.
Excuse me, I have a little trouble understanding your idea.
Since we can now print content which is not directly present in the input (because of escaping), the indices from StyledSpan are no longer a great solution. Keeping a Cow in the StyledSpan sounds easier.
Actually, I think we could merge StyledSpan and StyledString. Both refer to a "styled" section from the input text, potentially "rewritten" to account for escaping (hence the Cow).
If the indices are removed, StyledSpan
contains only the style
and width
. Since StyledString
already contains both of them (width
via styled_string.0.as_ref().width()
) merging them would just mean that StyledSpan
vanishes.
compute_rows
is basically equivalent to styled_lines_iter.collect()
, it just solves the ownership problem of StyledLinesIterator
by removing the ability to lazily iterate, keeping ownership in the parent scope. Also, how does it know the width constraint?
However I also don't like the potential for misuse that StyledLinesIterator
poses.
Therefore I would suggest that we do perform the merge of StyledSpan
and StyledString
, in effect just removing StyledSpan
, and make StyledLinesIterator
split StyledString
s into two on line breaks. (*) Since it now uses Cow
that should be possible. We gain:
- no more potential misuse of
StyledLinesIterator
- no more numerical indices tricking the borrow checker
- no
text_plain
to be kept alive by the user, it now "lives" in theCow
s of a[StyledString]
(*)
I'll try this so we can see how it compares with your approach. Maybe I missed something, especially since I'm not sure exactly what you mean.
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.
While updating LinesIterator
, I found one of the reasons I was using indices: when inserting text in a TextArea
, it tries not to recompute rows for the entire text, and just "offset" the indices in most situations.
Now, this is mostly an optimisation (so removing it should be slower, but safe), and TextArea
may end up using an entirely different structure anyway.
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.
Regarding the compute_rows
signature I posted, it is indeed overly simplistic, and was just meant to show an example of input/output flow.
Though thinking about it again, we may need 2 structures after all: the width only needs to be computed when creating the rows, so the input for the updated LineIterator
could be a list of
struct StyledStr<'a> {
content: Cow<'a, str>,
style: Style
}
and the output would use:
struct StyledSpan<'a> {
content: Cow<'a, str>,
style: Style,
width: usize,
}
We'll need the width multiple times and don't want to compute it each time, but we don't need it (and should not compute it) before computing the rows. Having a single type would be a bit ugly, since the width
field would just be left empty by the parser, and would be ignored by the LineIterator
, so having two types makes sense.
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.
Actually, we don't need Cow
at all for StyledSpan
, since it always points to the StyledStr
.
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.
Also, owning_ref doesn't look like it would be enough, be refstruct-rs looks very promising.
I need very strong this TextView functionality. When it can be implemented? |
I'm currently a bit busy with work & family. Should have some more time in october ; mouse support and styled text will be the two focus points then. |
Styled text is now implemented. A basic markdown parser exist, and more may come, but the core issue is resolved. |
Good news! Thanks a lot! |
The functionality is there but we still lack any widgets using styled text and a possible markup language.
I'd suggest not to merge this PR yet but use it to track progress on this front. Next I'll try to implement styled text for
TextView
as per #90.