-
Notifications
You must be signed in to change notification settings - Fork 780
Styled Text: add a macro and functions for parsing markdown #10060
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: master
Are you sure you want to change the base?
Conversation
|
Currently getting this error: @ogoffart where should I be registering this? |
internal/core/lib.rs
Outdated
| out | ||
| } | ||
|
|
||
| pub fn parse_markdown(text: &str) -> std::string::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.
The idea is that this function should return a slint::StyledText
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 know
This is supposed to be registered in
|
Ohh, right, thanks. Part of the confusion here is that we have both |
Indeed. It is possible to have the item have a different struct name. For example |
4e9e4bc to
b6d3eb8
Compare
a7a2ff7 to
ff8b450
Compare
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.
Looking overall good! :). I hope it's not too early to provide comments, but I've added some inline. I think besides those comments the main thing I see missing are tests (in tests/cases). I wonder if it's possible to do for example a link-click test with the testing backend.
Edit: I guess C++ is missing as well, but that's follow-up material perhaps?
| p.expect(SyntaxKind::RParent); | ||
| } | ||
|
|
||
| fn parse_markdown(p: &mut impl Parser) { |
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.
Please add some basic tests to this function. You can see with parse_tr() right above that there's a framework for that. That way we can catch any future refactoring mistakes earlier.
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.
And also test errors in similar in a syntax test, similar to internal/compiler/tests/syntax/basic/tr.slint and tr2.slint
https://github.com/slint-ui/slint/blob/master/docs/testing.md#syntax-tests
internal/core/api.rs
Outdated
| } | ||
|
|
||
| #[derive(Clone, Debug, PartialEq)] | ||
| #[allow(missing_docs)] |
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.
To be addressed in scope of this PR or afterwards? :)
internal/core/api.rs
Outdated
| ClosingTagMismatch(&'a str, std::string::String), | ||
| } | ||
|
|
||
| /// Internal styled text type |
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.
By this (and the other types) being placed here and being pub, they're not internal anymore but public API.
I sugest to make StyledText public, but its fields as well as the other associated types pub(crate), so that the public API surface is basically
#[non_exhaustive]
pub enum StyledTextError { ... }
pub struct StyledText { .. }
impl StyledText {
pub fn from_markdown(markdown: &str) -> Result<Self, StyledTextError> { ... }
pub fn from_plaintext(plaintext: &str) -> Self { ... }
}@expenses @ogoffart what do you think? (We can also have a call to discuss the API surface, if it's more than that)
internal/core/api.rs
Outdated
| #[cfg(feature = "experimental-rich-text")] | ||
| #[derive(Debug, thiserror::Error)] | ||
| #[allow(missing_docs)] | ||
| pub enum StyledTextError<'a> { |
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.
This enum should be non_exhaustive, I think.
ogoffart
left a comment
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.
Note that this PR is becoming big and i wonder if it wouldn't be beneficial to split some part like renaming "MarkDownText" or adding "styled-text" type in their own PR. (But only if it is easy to do and make things faster, not if it makes things slower)
| p.expect(SyntaxKind::RParent); | ||
| } | ||
|
|
||
| fn parse_markdown(p: &mut impl Parser) { |
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.
And also test errors in similar in a syntax test, similar to internal/compiler/tests/syntax/basic/tr.slint and tr2.slint
https://github.com/slint-ui/slint/blob/master/docs/testing.md#syntax-tests
| register.types.remove("DropEvent").unwrap(); // Also removed in xtask/src/slintdocs.rs | ||
|
|
||
| register.elements.remove("MarkdownText").unwrap(); | ||
| register.elements.remove("StyledText").unwrap(); |
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.
If we want to keep the feature internal, we also need to remove the "styled-text" from register.types
internal/core/api.rs
Outdated
| #[derive(Debug, PartialEq, Clone, Default)] | ||
| #[allow(missing_docs)] | ||
| pub struct StyledText { | ||
| #[cfg(feature = "experimental-rich-text")] |
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.
imho the full struct should be gated.
internal/core/api.rs
Outdated
| |ctx| ctx.set_xdg_app_id(app_id.into()), | ||
| ) | ||
| } | ||
|
|
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.
Perhaps this whole thing should be in its own module.
api.rs is already quite big.
it should either go to core/styled_text.rs or core/api/styled_text.rs
And everything should be gated with experimental-rich-text for now.
| debug_log!("Error opening url {}: {}", url, err); | ||
| } | ||
| } | ||
|
|
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.
And this should also go to the styled_text module
| } | ||
| } | ||
|
|
||
| pub fn escape_markdown(text: &str) -> alloc::string::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.
Note that this depends on where in markdown this goes.
If you have something like:
```
{}
```then you need another way to escape.
I wonder if that kind of escaping make sense at all, and maybe it would be best to do the interpolation on our internal data structure after the parsing.
| } | ||
| (_, true, PlainOrStyledText::Styled(_)) | ||
| | (fonts::Font::PixelFont(_), _, PlainOrStyledText::Styled(_)) => { | ||
| panic!("Unable to get text size of styled text without parley") |
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.
We have to discuss as well how to handle this. For example for Path, we just ignore it and not draw it, same for transformation. So I guess we have to do the same, or just draw the text without style.
229a0c2 to
b004797
Compare
No description provided.