Skip to content

Check the presence of glyphs when selecting font #113

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.

Sign up for GitHub

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

Merged
swift-kim merged 10 commits into from
Jun 22, 2021
Merged

Check the presence of glyphs when selecting font #113

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
21741dc
Check the presence of glyphs when selecting font
bwikbs Jun 9, 2021
caf01dd
Change default font family name
bwikbs Jun 9, 2021
1b3125e
Fix coding style
bwikbs Jun 9, 2021
5135217
Cache the glyphs and fonts used when matching fonts
bwikbs Jun 10, 2021
139fab6
Apply review's comment
bwikbs Jun 11, 2021
5713f1e
When matching fonts, if the existing character is a special character…
bwikbs Jun 11, 2021
5ed9fd7
Apply review's comment
bwikbs Jun 11, 2021
1b2bbf0
Fixed sorting to make emoji fonts go backwards
bwikbs Jun 14, 2021
bfe940f
Apply review's comment
bwikbs Jun 17, 2021
16e3397
Apply review's comment
bwikbs Jun 18, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 5 additions & 3 deletions third_party/txt/src/minikin/FontCollection.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -509,7 +509,9 @@ void FontCollection::itemize(const uint16_t* string,
if (!shouldContinueRun) {
const std::shared_ptr<FontFamily>& family = getFamilyForChar(
ch, isVariationSelector(nextCh) ? nextCh : 0, langListId, variant);
if (utf16Pos == 0 || family.get() != lastFamily) {
if (utf16Pos == 0 || family.get() != lastFamily ||
(!(U_GET_GC_MASK(prevCh) & U_GC_L_MASK) &&
(U_GET_GC_MASK(ch) & U_GC_L_MASK))) {
size_t start = utf16Pos;
// Workaround for combining marks and emoji modifiers until we implement
// per-cluster font selection: if a combining mark or an emoji modifier
Expand All @@ -528,8 +530,8 @@ void FontCollection::itemize(const uint16_t* string,
}
start -= prevChLength;
}
result->push_back(
{family->getClosestMatch(style), static_cast<int>(start), 0});
result->push_back({family->getClosestMatch(style, ch, variant),
static_cast<int>(start), 0});
run = &result->back();
lastFamily = family.get();
}
Expand Down
23 changes: 18 additions & 5 deletions third_party/txt/src/minikin/FontFamily.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -146,15 +146,28 @@ static FontFakery computeFakery(FontStyle wanted, FontStyle actual) {
return FontFakery(isFakeBold, isFakeItalic);
}

FakedFont FontFamily::getClosestMatch(FontStyle style) const {
FakedFont FontFamily::getClosestMatch(
FontStyle style,
uint32_t codepoint /* = 0 */,
uint32_t variationSelector /* = 0 */) const {
const Font* bestFont = nullptr;
int bestMatch = 0;
int bestMatch = INT_MAX;
for (size_t i = 0; i < mFonts.size(); i++) {
const Font& font = mFonts[i];
int match = computeMatch(font.style, style);
if (i == 0 || match < bestMatch) {
bestFont = &font;
bestMatch = match;
bool result = false;
if (codepoint != 0) {
hb_font_t* hbFont = getHbFontLocked(font.typeface.get());
uint32_t unusedGlyph = 0;
result =
hb_font_get_glyph(hbFont, codepoint, variationSelector, &unusedGlyph);
hb_font_destroy(hbFont);
}
if (codepoint == 0 || (codepoint != 0 && result)) {
if (match < bestMatch) {
bestFont = &font;
bestMatch = match;
}
}
}
if (bestFont != nullptr) {
Expand Down
4 changes: 3 additions & 1 deletion third_party/txt/src/minikin/FontFamily.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -140,7 +140,9 @@ class FontFamily {
static bool analyzeStyle(const std::shared_ptr<MinikinFont>& typeface,
int* weight,
bool* italic);
FakedFont getClosestMatch(FontStyle style) const;
FakedFont getClosestMatch(FontStyle style,
uint32_t codepoint = 0,
uint32_t variationSelector = 0) const;

uint32_t langId() const { return mLangId; }
int variant() const { return mVariant; }
Expand Down
22 changes: 22 additions & 0 deletions third_party/txt/src/txt/font_collection.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -244,6 +244,28 @@ void FontCollection::SortSkTypefaces(
std::sort(
sk_typefaces.begin(), sk_typefaces.end(),
[](const sk_sp<SkTypeface>& a, const sk_sp<SkTypeface>& b) {
{
// A workaround to prevent emoji fonts being selected for normal text
// when normal and emojis are mixed at same font family.

bool a_isEmojiFont = false;
bool b_isEmojiFont = false;
SkString postScriptName;
a->getPostScriptName(&postScriptName);
if (postScriptName.contains("Emoji")) {
a_isEmojiFont = true;
}
b->getPostScriptName(&postScriptName);
if (postScriptName.contains("Emoji")) {
b_isEmojiFont = true;
}
if (a_isEmojiFont && !b_isEmojiFont) {
return false;
} else if (!a_isEmojiFont && b_isEmojiFont) {
return true;
}
}

SkFontStyle a_style = a->fontStyle();
SkFontStyle b_style = b->fontStyle();

Expand Down
6 changes: 5 additions & 1 deletion third_party/txt/src/txt/platform_linux.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,9 @@
namespace txt {

std::vector<std::string> GetDefaultFontFamilies() {
#ifdef FLUTTER_USE_FONTCONFIG
return {"TizenDefaultFont"};
#else
Copy link
Member

@swift-kim swift-kim Jun 17, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need this change (caf01dd) if we're not to re-enable fontconfig?

Copy link
Member Author

@bwikbs bwikbs Jun 17, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, TizenDefaultFont is not an actual font. That's an alias in fontconfig

Copy link
Member

@swift-kim swift-kim Jun 17, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Then you might revert the commit.

Copy link
Member Author

@bwikbs bwikbs Jun 18, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm sorry but I don't get the point. Because..?

Copy link
Member

@swift-kim swift-kim Jun 18, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The commit in this context is caf01dd.

Copy link
Member Author

@bwikbs bwikbs Jun 18, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm still not sure if I understood your intentions...

This PR is base on fontconfig. And PR that uses font config again and revert unnecessary our modification will be prepared another PR (Perhaps @bbrto21 Am I right boram? 😄 ).
The reason for doing that is because it would be good to track the related fontconfig in the future.(Just my opinion)

I'm sorry for the lack of explanation of this.

Copy link
Member

@swift-kim swift-kim Jun 18, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#106 (review) by @bbrto21:

I think you need to re-enable fontconfig in azure-pipelines.

#106 (comment) by @bwikbs:

Reverting existing code and applying fontconfig seem to be a little different topic, so I would like to talk about it in another PR.

If this PR depends on fontconfig, why not re-enable the fontconfig option in azure-pipelines.yml? The change inside the #ifdef FLUTTER_USE_FONTCONFIG block doesn't affect anything without the option enabled, so I thought you could safely revert the change. I'm sorry but I still don't get your point.

Copy link
Member Author

@bwikbs bwikbs Jun 18, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@swift-kim Well... You're right. I just wanted to collect the related re-enable font config contents in one PR.
There is no great reason...(To be honest, I didn't want to revert and clean-up the existing ones. And because it seems that @bbrto21 has already done that work, I was going to ask for him)
I didn't think much about it.. If it's too weird, I'll enable the font config right away.

Copy link
Member

@swift-kim swift-kim Jun 18, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, if there will be a follow-up PR, you may just briefly mention what changes will be made in that PR and how TizenDefaultFont is related to those changes.

Copy link
Member Author

@bwikbs bwikbs Jun 18, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've updated PR description!

return {
"SamsungOneUI",
"SamsungOneUIArabic",
Expand Down Expand Up @@ -72,11 +75,12 @@ std::vector<std::string> GetDefaultFontFamilies() {
"BreezeSansFallback",
"BreezeColorEmoji",
};
#endif
}

sk_sp<SkFontMgr> GetDefaultFontManager() {
#ifdef FLUTTER_USE_FONTCONFIG
return SkFontMgr_New_FontConfig(nullptr);
return SkFontMgr::RefDefault();
#else
return SkFontMgr_New_Custom_Directory("/usr/share/fonts");
#endif
Expand Down