-
Notifications
You must be signed in to change notification settings - Fork 659
Introduce Platform.style-name
and Platform.os
#8202
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
I tried this manually, but I'm curious about suggestions how to test this. I could "expose" |
0895f51
to
7e66928
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.
Thanks. Looks good to me.
But I don't think we should have Platform without the other stuff like os and stuff. and it's probably not a good idea to rush that in 1.11 so i'd favor waiting.
For the test, maybe you can have something like
out property <bool> test: Platform.style-name != "" && (Platform.os == "macOs" ? platform.style-name == "cupertino" : Platform.os == "windows" ? platform.style-name == "fluent" : true );
or out property <string> style-name <=> Platform.style-name
(i wonder if <=>
is allowed actually, otherwise just use :
) and test that with #[cfg(...)]
in the rust test.
For the name, should it be style-name
or just style
?
@@ -840,7 +840,7 @@ impl Snapshotter { | |||
pub struct TypeLoader { | |||
pub global_type_registry: Rc<RefCell<TypeRegister>>, | |||
pub compiler_config: CompilerConfiguration, | |||
style: String, | |||
pub resolved_style: 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.
I would have left the name as is. But can't hurt.
Maybe a doc comment explaining what is is exactly would be good.
That sounds good to me. I'll add those "on top" in this PR then.
Absolutely. I don't intend to propose this for 1.11. As discussed in the API review, I just wanted to do both in one, the change for 1.11 as well as the follow-up, so that it's clearer to see where this goes.
Ah yes, great, with
I prefer |
Marking this as draft, to make it clearer that I don't intend this for 1.11. |
Oops, I see that this doesn't work because we go through lengths to make sure that tests are run with the fluent style for consistent results :-). But I'll add this to the widget tests and check at least that the value changes. |
fb87cb5
to
affe667
Compare
This replaces the previously hidden `StyleMetrics.style-name` that was only accessible for internal use.
9cb1f8b
to
534e935
Compare
This replaces the previously hidden
StyleMetrics.style-name
that was only accessible for internal use, introduced in #8200.Additionally, this introduces
Platform.os
.