-
Notifications
You must be signed in to change notification settings - Fork 780
Add StyledText type #10122
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 StyledText type #10122
Conversation
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.
Looks good, thanks for splitting!
I'm thinking this should still be gated as experimental.
Alternative is that, if we really intend to have this feature ready in the next release, we just remove all gating now.
(And if we decide we want to release anyway without it, we re-add the gating before the release)
| register.insert_type(Type::Angle); | ||
| register.insert_type(Type::Brush); | ||
| register.insert_type(Type::Rem); | ||
| register.insert_type(Type::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.
Should we keep it experimental by removing it from the builtin() function (like we do for Type::ComponentFactory) (or not adding it in the first place by adding a parametter to this function to only add it if experimental)
internal/core/api/styled_text.rs
Outdated
| // SPDX-License-Identifier: GPL-3.0-only OR LicenseRef-Slint-Royalty-free-2.0 OR LicenseRef-Slint-Software-3.0 | ||
|
|
||
| #[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.
I think the warning is valid and if we commit this as is, we risk to forget to document.
I suggest making it
#![cfg_attr(feature="experimental-rich-text", allow(missing_docs))]
that way, we would get the warning when we remove the experimental of it.
Or we just add the documentation now
| use alloc::string::String; | ||
|
|
||
| mod styled_text; | ||
| pub use styled_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.
The whole module can be gated on #[cfg(feature = "experimental-rich-text")]
internal/core/api/styled_text.rs
Outdated
|
|
||
| #[derive(Clone, Debug, PartialEq)] | ||
| #[allow(missing_docs)] | ||
| pub struct FormattedSpan { |
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.
Does these types actually need to be in the public API?
Maybe it's better to have StyledText as an opaque struct?
I'm in favour of the alternative. |
|
I've removed the feature gating around the styled text stuff while keeping it around for the things in shared-parley for the time being |
1751cc1 to
235cc25
Compare
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.
Looks good to me appart from the fact that this adds a lot of public API which i think shouldn't be public API.
Split off from #10060 as that PR was getting too large