wxOSX Translate Courier and Times font names (PR #22355)
Stefan Csomor
under macOS 12 Monterey Courier and Times are no more, now always use the new versions (existing since macOS 10.7). and fix native font info toString roundtrip problems
You can view, comment on, or merge this pull request online at:
https://github.com/wxWidgets/wxWidgets/pull/22355
Commit Summary
- a64b2c6 always switching to ‚new‘ variants of Times and Courier on macOS, fixing roundtrip test
File Changes
(1 file)
- M src/osx/carbon/font.cpp (31)
Patch Links:
- https://github.com/wxWidgets/wxWidgets/pull/22355.patch
- https://github.com/wxWidgets/wxWidgets/pull/22355.diff
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
VZ
@vadz commented on this pull request.
Thanks!
It looks like this ought to be split in 2 different commits as the 2 changes don't seem to be dependent on each other (unless I'm missing something), do you mind if I commit them as 2 commits to facilitate exploring the history later?
I'd also like to change FamilyToFaceName() in the same file to use the new font names -- even if the old ones get translated, it still seems better to use the actual font names directly to make things more clear. Would you mind if I did this?
And, as mentioned below, I'd appreciate if you could please explain the rationale for v3 font info format, either by pushing an additional commit to this branch or just leaving it in a comment directly here.
TIA!
> @@ -1171,8 +1194,10 @@ wxString wxNativeFontInfo::ToString() const
xml.Replace("\t",wxEmptyString,true);
xml = xml.Mid(xml.Find("<plist"));
- s.Printf("%d;%d;%d;%d;%s",
- 2, // version
+ s.Printf("%d;%d;%d;%d;%d;%d;%s",
+ 3, // version
The comment above needs to be updated to explain the v3 format rather than v2 which is not used any longer. In particular, I'd really like to know why do we include style but not weight into it now (and while I have some conjectures about the family, I'd appreciate having an explanation for it too).
> @@ -927,6 +927,11 @@ void wxNativeFontInfo::CreateCTFontDescriptor()
if ( fontname.empty() )
fontname = FamilyToFaceName(m_family);
+ if ( fontname == "Courier")
I think we need some explanation here as it doesn't seem to be that obvious, e.g.
⬇️ Suggested change- if ( fontname == "Courier") + // Courier and Times fonts used to be available everywhere and so are commonly + // hard-coded in the applications (even though they shouldn't be, and "teletype" + // or "roman" font family should be used instead), so make sure we still support + // them even if macOS > 12 doesn't have any fonts with these names any more. + if ( fontname == "Courier")
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Stefan Csomor
@csomor pushed 1 commit.
- 0412cc2 Update src/osx/carbon/font.cpp
—
View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.
Stefan Csomor
@csomor commented on this pull request.
> @@ -1171,8 +1194,10 @@ wxString wxNativeFontInfo::ToString() const
xml.Replace("\t",wxEmptyString,true);
xml = xml.Mid(xml.Find("<plist"));
- s.Printf("%d;%d;%d;%d;%s",
- 2, // version
+ s.Printf("%d;%d;%d;%d;%d;%d;%s",
+ 3, // version
The font tests showed that a font created from a font info string was not getting family nor italic round-trip correct. I was exporting a string from the native font descriptor as xml and had to add what was missing in front (v2), now the tests showed that two other properties were not roundtrip correct.
Eg a font could have a slant variant natively (thus expressed in the descriptor) or emulated by wx (normal font in font descriptor, slant matrix applied when constructing the font itself, 3rd param in CTFontCreateWithFontDescriptor).
Deducing the font family does not work the way I thought it would, apparently the stylistic font class information is not always available. At least not from the descriptor alone. So while I try to find out whether I can make this work natively, exposing the family like this makes test pass correctly.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
VZ
@vadz commented on this pull request.
> @@ -1171,8 +1194,10 @@ wxString wxNativeFontInfo::ToString() const
xml.Replace("\t",wxEmptyString,true);
xml = xml.Mid(xml.Find("<plist"));
- s.Printf("%d;%d;%d;%d;%s",
- 2, // version
+ s.Printf("%d;%d;%d;%d;%d;%d;%s",
+ 3, // version
I don't think making the tests pass under Mac is the highest priority -- it would be great if they passed, of course, but what we really need is for things to work correctly and I'm not totally sure if these changes actually achieve this. I.e. if it's just a temporary hack to make the tests pass, I'd rather avoid changing the format and add Mac-specific checks to the tests instead.
In response to the last question, I think wxNativeFontInfo should only be available when the corresponding native font is available, otherwise what's the point of using it? The rationale for it is to be able to exactly preserve, without any loss due to round tripping between it and wx font description, any native font. As long as we don't have any native font, we shouldn't need wxNativeFontInfo at all.
Because of this I'm also not sure at all if storing wx family in the serialized representation really makes sense...
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
VZ
It looks like this ought to be split in 2 different commits as the 2 changes don't seem to be dependent on each other (unless I'm missing something), do you mind if I commit them as 2 commits to facilitate exploring the history later?
I'd like to apply the first commit ("Use actually existing fonts") already, do you mind if I do this?
I'd also like to change
FamilyToFaceName()in the same file to use the new font names -- even if the old ones get translated, it still seems better to use the actual font names directly to make things more clear. Would you mind if I did this?
I'd also still like to do this too.
And, as mentioned below, I'd appreciate if you could please explain the rationale for v3 font info format, either by pushing an additional commit to this branch or just leaving it in a comment directly here.
AFAICS this change is less critical and so could wait and, also, unless you're absolutely sure it's correct and the v3 format is here to stay, I'd rather not change the format now only to possibly change it again very soon.
So, to summarize, I'd like to partially apply this a.s.a.p. but leave the serialization format-related changes for later unless you're definitely certain that we should apply them now. Please let me know what do you think, TIA!
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Stefan Csomor
@vadz I'm fine with merging the font names part, so go ahead :-)
and if there is no need for a native font info to be preserved if the font it wants to express is not available, I'll store the descriptor back after a font resolution, thus the font family should work, I hope at least ...
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Stefan Csomor
@csomor commented on this pull request.
> @@ -1171,8 +1194,10 @@ wxString wxNativeFontInfo::ToString() const
xml.Replace("\t",wxEmptyString,true);
xml = xml.Mid(xml.Find("<plist"));
- s.Printf("%d;%d;%d;%d;%s",
- 2, // version
+ s.Printf("%d;%d;%d;%d;%d;%d;%s",
+ 3, // version
No the reason would not be to make the tests work, but to make things work as expected. So the style must be working correctly, this will be needed, either by the style flag or by adding a matrix transform, the first seemed to be easier, so this will be remain, but regarding the family, there is another option, see below ...
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
VZ
OK, done now, thanks!
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
VZ
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Stefan Csomor
@vadz what is the best way to continue with the remaining part, a new PR or reopening this ?
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
VZ
I think the best would be to create a new PR, as this one was just about the font names and this part got applied. TIA!
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Stefan Csomor
ok, done in #22373
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.