-
Notifications
You must be signed in to change notification settings - Fork 34
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
[TEST] Text rendering comparison #13
Conversation
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.
Just a couple minor things
@@ -26,7 +26,7 @@ intellij { | |||
// pluginName.set("Compose support for IJ UI development") | |||
version.set("LATEST-EAP-SNAPSHOT") | |||
plugins.set(listOf("org.jetbrains.kotlin", "org.jetbrains.compose.desktop.ide:${libs.versions.composeDesktop.get()}")) | |||
version.set("2021.2.4") | |||
version.set("2021.3.3") |
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 had to use 21.2 because 21.3+ would crash, happy to see that's fixed now ✨
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.
Oh I didn't know that. I did try 22.1 and that crashed. Did you test this change locally?
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.
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! Reproduced it.
@@ -157,8 +159,24 @@ suspend fun CurrentIntelliJThemeDefinition(): IntelliJThemeDefinition { | |||
) | |||
) | |||
|
|||
val baseTextStyle = retrieveFont("Label.font", palette.text) | |||
|
|||
fun TextStyle.scaleFont(amount: Float): TextStyle { |
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.
Just a nit, but I'd expect a function called scale*
to take in a float scale factor, not an offset amount
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.
That's fair. I think given text units in Compose use SP
we get scaling for free, unlike these utility functions (biggerOn, lessOn) in JBUI. That is what I modeled it after with some simplification.
@igordmn any clue? |
About loading SF issue. How it is loaded, from file, or by family name? |
@jimgoog will get someone from the text team on the Google side to help out with the text-specific issues here |
@c5inco I've taken the liberty to push a fix so that the text demo now shows up with the right background, and removed some redundant |
cc @objcode |
@c5inco can you the font code in a minimal repro, may be a framework issue but hard to tell from this CL |
@jimgoog sure, it starts in There's a significant difference in the font rendering that is quite apparent (Windows, darcula): Looks like our definition of bold and Swing's are pretty different. Swing's bold looks ExtraBold, or even Black. I tried setting our weights to ExtraBold and Black but that has no effect. Is this a matter of the weights not being included in the fontfamily on the Skia side? Not sure. |
@rock3r did you make these changes in the IJ platform as well? |
@c5inco no I've updated our typography to match the existing platform stuff |
@igordmn It seems font loading isn't working in the Jewel bridge. But it seems it might not be Jewel and it's more on the Compose Desktop side? |
@igordmn why is the default Helvetica Neue in Compose when the default in macOS generally appears to be SF? |
@c5inco the bridge only gets the font from the IJ theme (it's an AWT font), and tries to find it by name in the available fonts list as provided by Skiko (see these lines) You can follow through the call to Skiko's |
I don't remember the exact reason. Maybe it is because in Skia itself we have it by default. Chrome also has Helvetica as a default font (not Helvetica Neue though). But we'll investigate whether we should change it to San Francisco.
I am confused :). Does font loading work or doesn't work? |
@igordmn thanks on the SF check - I think we should change it if possible since Helvetica is mainly used by older macOS apps. And for font loading, no loading doesn't seem to be working when using the name pulled from the AWT theme - this is the error seen in the log from Jewel:
Sebastiano's previous comment has more links to the code where Jewel is trying to load the font. |
JetBrains/compose-multiplatform-core#296 San Francisco by default |
@igordmn what stable version of CfD will this land in? |
This is already in 1.2.0-beta01. 1.2.0 will be released on October, 10. Also, will try to look on other font issues |
Awesome great to hear. Yes I'll test it after that release (update this PR) and see if there are more issues. |
@c5inco This is already in |
The change of changing the default font was primarily for usual Compose applications. It probably should not affect Jewel, because we should always use the font defined in the IDEA settings, not the system default font. And to use this font, we should define a separate component, something like |
It seems this isn't working yet in @igordmn do you have the sample code that generated the screenshots here (JetBrains/compose-multiplatform-core#296)? |
You mean, the default font isn't San Francisco? That was the only change, and there were no bug fixes. The code is usual:
I haven't looked on this issue yet |
Yes the default is still Helvetica Neue it seems. Used the same "Rat's" content in Button to compare as well as digits.
Yeah, I'm getting the same issue in the logs output on |
I'm not really sure what's going on, haven't had time to test it on Mac yet. I'll take a look today, hopefully 🤞 |
Could you write here your macOs version? |
I have checked and this happens for me as well — the Skiko AwtFontManager doesn't find the font. I debugged it and it's simply not in the map: I suppose this is happening because the fonts starting with If you look at what Qt does, for example, on macOS they'll be scanning for aliases using CoreFont. I don't really think we have much of a workaround for this, we'll need Skiko to do some serious JNI work; then it'll know about all the fonts, and this will be fixed. |
66919d6
to
adbc958
Compare
PS: I have just pushed a rebase on top of master, should get in some improvements/fixes (and up-to-date dependencies) |
It seems so. We need to implement more complex font searching. Not sure yet if we need native API for that |
This is beyond obsolete by now; I'll close it |
Branch to show and discuss text rendering differences between Swing and Compose for Desktop.
Font styles are based on the current typography spec for IntelliJ. You can also run an internal action to see the Swing demo with Tools > Internal Actions > UI > Demos > Test Label Sizes (this is also where I pulled the Swing code for comparison).