From 7fdbd67e73303cf090b19578d6aa678162656b2f Mon Sep 17 00:00:00 2001 From: Xiao Yu Date: Mon, 17 Aug 2020 02:34:52 -0700 Subject: [PATCH 1/7] Fix font sorting problem due to iOS 14 fonts being broader --- third_party/txt/src/txt/font_collection.cc | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/third_party/txt/src/txt/font_collection.cc b/third_party/txt/src/txt/font_collection.cc index c35c10a95f7e5..f8850e5557d93 100644 --- a/third_party/txt/src/txt/font_collection.cc +++ b/third_party/txt/src/txt/font_collection.cc @@ -227,7 +227,16 @@ std::shared_ptr FontCollection::CreateMinikinFontFamily( [](const sk_sp& a, const sk_sp& b) { SkFontStyle a_style = a->fontStyle(); SkFontStyle b_style = b->fontStyle(); - return (a_style.weight() != b_style.weight()) + return (a_style.width() != b_style.width()) + // If a family name query is so generic it ends up bringing + // in fonts of multiple widths (e.g. condensed, expanded), + // opt to be conservative and select the most standard width. + + // If a specific width is desired, it should be be narrowed + // down via the family name. + ? std::abs(a_style.width() - SkFontStyle::kNormal_Width) < + std::abs(b_style.width() - SkFontStyle::kNormal_Width) + : (a_style.weight() != b_style.weight()) ? a_style.weight() < b_style.weight() : a_style.slant() < b_style.slant(); }); From 24f0b827818664914248b263eaf15cbccd16702f Mon Sep 17 00:00:00 2001 From: Xiao Yu Date: Mon, 17 Aug 2020 02:41:23 -0700 Subject: [PATCH 2/7] autoformat --- third_party/txt/src/txt/font_collection.cc | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/third_party/txt/src/txt/font_collection.cc b/third_party/txt/src/txt/font_collection.cc index f8850e5557d93..b1c2cf26cf4a6 100644 --- a/third_party/txt/src/txt/font_collection.cc +++ b/third_party/txt/src/txt/font_collection.cc @@ -223,11 +223,12 @@ std::shared_ptr FontCollection::CreateMinikinFontFamily( } } - std::sort(skia_typefaces.begin(), skia_typefaces.end(), - [](const sk_sp& a, const sk_sp& b) { - SkFontStyle a_style = a->fontStyle(); - SkFontStyle b_style = b->fontStyle(); - return (a_style.width() != b_style.width()) + std::sort( + skia_typefaces.begin(), skia_typefaces.end(), + [](const sk_sp& a, const sk_sp& b) { + SkFontStyle a_style = a->fontStyle(); + SkFontStyle b_style = b->fontStyle(); + return (a_style.width() != b_style.width()) // If a family name query is so generic it ends up bringing // in fonts of multiple widths (e.g. condensed, expanded), // opt to be conservative and select the most standard width. @@ -239,7 +240,7 @@ std::shared_ptr FontCollection::CreateMinikinFontFamily( : (a_style.weight() != b_style.weight()) ? a_style.weight() < b_style.weight() : a_style.slant() < b_style.slant(); - }); + }); std::vector minikin_fonts; for (const sk_sp& skia_typeface : skia_typefaces) { From 9a814a0aa8f2058a2cef54cda451d2c8229415f2 Mon Sep 17 00:00:00 2001 From: Xiao Yu Date: Mon, 17 Aug 2020 10:50:02 -0700 Subject: [PATCH 3/7] add a bit more comment --- third_party/txt/src/txt/font_collection.cc | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/third_party/txt/src/txt/font_collection.cc b/third_party/txt/src/txt/font_collection.cc index b1c2cf26cf4a6..bc8205c9994a1 100644 --- a/third_party/txt/src/txt/font_collection.cc +++ b/third_party/txt/src/txt/font_collection.cc @@ -232,9 +232,14 @@ std::shared_ptr FontCollection::CreateMinikinFontFamily( // If a family name query is so generic it ends up bringing // in fonts of multiple widths (e.g. condensed, expanded), // opt to be conservative and select the most standard width. - + // // If a specific width is desired, it should be be narrowed // down via the family name. + // + // The font weights are also sorted lightest to heaviest + // but Flutter APIs have the weight specified to narrow it + // down later. The width ordering here is more consequential + // since TextStyle doesn't have letter width APIs. ? std::abs(a_style.width() - SkFontStyle::kNormal_Width) < std::abs(b_style.width() - SkFontStyle::kNormal_Width) : (a_style.weight() != b_style.weight()) From 71ccc8e1769bc7e147d59c8f8e3d3c7d9ea43b0e Mon Sep 17 00:00:00 2001 From: Xiao Yu Date: Wed, 19 Aug 2020 00:42:24 -0700 Subject: [PATCH 4/7] trying various things to test this --- third_party/txt/src/txt/font_collection.cc | 53 +++++---- third_party/txt/src/txt/font_collection.h | 5 + .../txt/tests/font_collection_unittests.cc | 105 ++++++++++++++++++ 3 files changed, 140 insertions(+), 23 deletions(-) diff --git a/third_party/txt/src/txt/font_collection.cc b/third_party/txt/src/txt/font_collection.cc index bc8205c9994a1..a0bacb05a0d8a 100644 --- a/third_party/txt/src/txt/font_collection.cc +++ b/third_party/txt/src/txt/font_collection.cc @@ -202,6 +202,35 @@ std::shared_ptr FontCollection::FindFontFamilyInManagers( return nullptr; } +void FontCollection::SortSkTypefaces( + std::vector> sk_typefaces) { + std::sort(sk_typefaces.begin(), sk_typefaces.end(), + [](const sk_sp& a, const sk_sp& b) { + SkFontStyle a_style = a->fontStyle(); + SkFontStyle b_style = b->fontStyle(); + + if (a_style.width() != b_style.width()) { + // If a family name query is so generic it ends up bringing in + // fonts of multiple widths (e.g. condensed, expanded), opt to + // be conservative and select the most standard width. + // + // If a specific width is desired, it should be be narrowed down + // via the family name. + // + // The font weights are also sorted lightest to heaviest but + // Flutter APIs have the weight specified to narrow it down + // later. The width ordering here is more consequential since + // TextStyle doesn't have letter width APIs. + return std::abs(a_style.width() - SkFontStyle::kNormal_Width) < + std::abs(b_style.width() - SkFontStyle::kNormal_Width); + } else { + return (a_style.weight() != b_style.weight()) + ? a_style.weight() < b_style.weight() + : a_style.slant() < b_style.slant(); + } + }); +} + std::shared_ptr FontCollection::CreateMinikinFontFamily( const sk_sp& manager, const std::string& family_name) { @@ -223,29 +252,7 @@ std::shared_ptr FontCollection::CreateMinikinFontFamily( } } - std::sort( - skia_typefaces.begin(), skia_typefaces.end(), - [](const sk_sp& a, const sk_sp& b) { - SkFontStyle a_style = a->fontStyle(); - SkFontStyle b_style = b->fontStyle(); - return (a_style.width() != b_style.width()) - // If a family name query is so generic it ends up bringing - // in fonts of multiple widths (e.g. condensed, expanded), - // opt to be conservative and select the most standard width. - // - // If a specific width is desired, it should be be narrowed - // down via the family name. - // - // The font weights are also sorted lightest to heaviest - // but Flutter APIs have the weight specified to narrow it - // down later. The width ordering here is more consequential - // since TextStyle doesn't have letter width APIs. - ? std::abs(a_style.width() - SkFontStyle::kNormal_Width) < - std::abs(b_style.width() - SkFontStyle::kNormal_Width) - : (a_style.weight() != b_style.weight()) - ? a_style.weight() < b_style.weight() - : a_style.slant() < b_style.slant(); - }); + SortSkTypefaces(skia_typefaces); std::vector minikin_fonts; for (const sk_sp& skia_typeface : skia_typefaces) { diff --git a/third_party/txt/src/txt/font_collection.h b/third_party/txt/src/txt/font_collection.h index c81a342557dad..a3ea612c2967a 100644 --- a/third_party/txt/src/txt/font_collection.h +++ b/third_party/txt/src/txt/font_collection.h @@ -127,6 +127,11 @@ class FontCollection : public std::enable_shared_from_this { const sk_sp& manager, const std::string& family_name); + // Sorts in-place a group of SkTypeface from an SkTypefaceSet into a + // reasonable order for future queries. + FRIEND_TEST(FontCollectionTest, CheckSkTypefacesSorting); + static void SortSkTypefaces(std::vector> sk_typefaces); + const std::shared_ptr& GetFallbackFontFamily( const sk_sp& manager, const std::string& family_name); diff --git a/third_party/txt/tests/font_collection_unittests.cc b/third_party/txt/tests/font_collection_unittests.cc index 6b935423d7c20..7f8426e87626b 100644 --- a/third_party/txt/tests/font_collection_unittests.cc +++ b/third_party/txt/tests/font_collection_unittests.cc @@ -16,12 +16,117 @@ #include "flutter/fml/command_line.h" #include "flutter/fml/logging.h" +#include "gmock/gmock.h" #include "gtest/gtest.h" +#include "third_party/skia/src/core/SkAdvancedTypefaceMetrics.h" +#include "third_party/skia/src/core/SkScalerContext.h" #include "txt/font_collection.h" #include "txt_test_utils.h" namespace txt { +// We don't really need a fixture but a class in a namespace is needed for +// the FRIEND_TEST macro. +class FontCollectionTest : public ::testing::Test {}; + +class MockSkTypeface : public SkTypeface { + public: + MockSkTypeface(); + MOCK_CONST_METHOD1(onMakeClone, sk_sp(const SkFontArguments&)); + MOCK_CONST_METHOD2(onCreateScalerContext, + SkScalerContext*(const SkScalerContextEffects&, + const SkDescriptor*)); + MOCK_CONST_METHOD1(onFilterRec, void(SkScalerContextRec*)); + MOCK_CONST_METHOD0(onGetAdvancedMetrics, + std::unique_ptr()); + MOCK_CONST_METHOD1(getPostScriptGlyphNames, void(SkString*)); + MOCK_CONST_METHOD1(getGlyphToUnicodeMap, void(SkUnichar* dstArray)); + MOCK_CONST_METHOD1(onOpenStream, + std::unique_ptr(int* ttcIndex)); + MOCK_CONST_METHOD2( + onGetVariationDesignPosition, + int(SkFontArguments::VariationPosition::Coordinate coordinates[], + int coordinateCount)); + MOCK_CONST_METHOD2(onGetVariationDesignParameters, + int(SkFontParameters::Variation::Axis parameters[], + int parameterCount)); + MOCK_CONST_METHOD2(onGetFontDescriptor, + void(SkFontDescriptor*, bool* isLocal)); + MOCK_CONST_METHOD3(onCharsToGlyphs, + void(const SkUnichar* chars, + int count, + SkGlyphID glyphs[])); + MOCK_CONST_METHOD0(onCountGlyphs, int()); + MOCK_CONST_METHOD0(onGetUPEM, int()); + MOCK_CONST_METHOD3(onGetKerningPairAdjustments, + bool(const SkGlyphID glyphs[], + int count, + int32_t adjustments[])); + MOCK_CONST_METHOD1(onGetFamilyName, void(SkString* familyName)); + MOCK_CONST_METHOD0(onCreateFamilyNameIterator, SkTypeface::LocalizedStrings*()); + MOCK_CONST_METHOD1(onGetTableTags, int(SkFontTableTag tags[])); + MOCK_CONST_METHOD4( + onGetTableData, + size_t(SkFontTableTag, size_t offset, size_t length, void* data)); + MOCK_CONST_METHOD1(onCopyTableData, sk_sp(SkFontTableTag)); + MOCK_CONST_METHOD1(onComputeBounds, bool(SkRect*)); + MOCK_CONST_METHOD0(onGetCTFontRef, void*()); +}; + +TEST(FontCollectionTest, CheckSkTypefacesSorting) { + MockSkTypeface mockTypeface1; + + sk_sp typeface1{SkTypeface::MakeFromName( + "Arial", + SkFontStyle(SkFontStyle::kThin_Weight, SkFontStyle::kExpanded_Width, + SkFontStyle::kItalic_Slant))}; + sk_sp typeface2{SkTypeface::MakeFromName( + "Arial", + SkFontStyle(SkFontStyle::kLight_Weight, SkFontStyle::kNormal_Width, + SkFontStyle::kUpright_Slant))}; + sk_sp typeface3{SkTypeface::MakeFromName( + "Arial", + SkFontStyle(SkFontStyle::kNormal_Weight, SkFontStyle::kNormal_Width, + SkFontStyle::kUpright_Slant))}; + std::vector> candidateTypefaces{typeface1, typeface2, + typeface3}; + + ASSERT_EQ(candidateTypefaces[0]->fontStyle().weight(), + SkFontStyle::kThin_Weight); + ASSERT_EQ(candidateTypefaces[0]->fontStyle().width(), + SkFontStyle::kExpanded_Width); + + ASSERT_EQ(candidateTypefaces[1]->fontStyle().weight(), + SkFontStyle::kLight_Weight); + ASSERT_EQ(candidateTypefaces[1]->fontStyle().width(), + SkFontStyle::kNormal_Width); + + ASSERT_EQ(candidateTypefaces[2]->fontStyle().weight(), + SkFontStyle::kNormal_Weight); + ASSERT_EQ(candidateTypefaces[2]->fontStyle().width(), + SkFontStyle::kNormal_Width); + + // This sorts the vector in-place. + txt::FontCollection::SortSkTypefaces(candidateTypefaces); + ASSERT_EQ(candidateTypefaces[0].get(), typeface1.get()); + ASSERT_EQ(candidateTypefaces[1].get(), typeface2.get()); + + // ASSERT_EQ(candidateTypefaces[0]->fontStyle().weight(), + // SkFontStyle::kLight_Weight); + ASSERT_EQ(candidateTypefaces[0]->fontStyle().width(), + SkFontStyle::kNormal_Width); + + ASSERT_EQ(candidateTypefaces[1]->fontStyle().weight(), + SkFontStyle::kNormal_Weight); + ASSERT_EQ(candidateTypefaces[1]->fontStyle().width(), + SkFontStyle::kNormal_Width); + + ASSERT_EQ(candidateTypefaces[2]->fontStyle().weight(), + SkFontStyle::kThin_Weight); + ASSERT_EQ(candidateTypefaces[2]->fontStyle().width(), + SkFontStyle::kExpanded_Width); +} + #if 0 TEST(FontCollection, HasDefaultRegistrations) { From 2e45aed051bc244e403322547e0dca1b927243a8 Mon Sep 17 00:00:00 2001 From: Xiao Yu Date: Thu, 20 Aug 2020 23:23:50 -0700 Subject: [PATCH 5/7] add test --- third_party/txt/src/txt/font_collection.cc | 2 +- third_party/txt/src/txt/font_collection.h | 2 +- .../txt/tests/font_collection_unittests.cc | 154 +++++++++--------- 3 files changed, 76 insertions(+), 82 deletions(-) diff --git a/third_party/txt/src/txt/font_collection.cc b/third_party/txt/src/txt/font_collection.cc index a0bacb05a0d8a..e82b8d09ecaef 100644 --- a/third_party/txt/src/txt/font_collection.cc +++ b/third_party/txt/src/txt/font_collection.cc @@ -203,7 +203,7 @@ std::shared_ptr FontCollection::FindFontFamilyInManagers( } void FontCollection::SortSkTypefaces( - std::vector> sk_typefaces) { + std::vector>& sk_typefaces) { std::sort(sk_typefaces.begin(), sk_typefaces.end(), [](const sk_sp& a, const sk_sp& b) { SkFontStyle a_style = a->fontStyle(); diff --git a/third_party/txt/src/txt/font_collection.h b/third_party/txt/src/txt/font_collection.h index a3ea612c2967a..8a609dfb87159 100644 --- a/third_party/txt/src/txt/font_collection.h +++ b/third_party/txt/src/txt/font_collection.h @@ -130,7 +130,7 @@ class FontCollection : public std::enable_shared_from_this { // Sorts in-place a group of SkTypeface from an SkTypefaceSet into a // reasonable order for future queries. FRIEND_TEST(FontCollectionTest, CheckSkTypefacesSorting); - static void SortSkTypefaces(std::vector> sk_typefaces); + static void SortSkTypefaces(std::vector>& sk_typefaces); const std::shared_ptr& GetFallbackFontFamily( const sk_sp& manager, diff --git a/third_party/txt/tests/font_collection_unittests.cc b/third_party/txt/tests/font_collection_unittests.cc index 7f8426e87626b..9bd0ed9b8e391 100644 --- a/third_party/txt/tests/font_collection_unittests.cc +++ b/third_party/txt/tests/font_collection_unittests.cc @@ -14,12 +14,10 @@ * limitations under the License. */ -#include "flutter/fml/command_line.h" #include "flutter/fml/logging.h" #include "gmock/gmock.h" #include "gtest/gtest.h" -#include "third_party/skia/src/core/SkAdvancedTypefaceMetrics.h" -#include "third_party/skia/src/core/SkScalerContext.h" +#include "third_party/skia/include/utils/SkCustomTypeface.h" #include "txt/font_collection.h" #include "txt_test_utils.h" @@ -29,90 +27,86 @@ namespace txt { // the FRIEND_TEST macro. class FontCollectionTest : public ::testing::Test {}; -class MockSkTypeface : public SkTypeface { - public: - MockSkTypeface(); - MOCK_CONST_METHOD1(onMakeClone, sk_sp(const SkFontArguments&)); - MOCK_CONST_METHOD2(onCreateScalerContext, - SkScalerContext*(const SkScalerContextEffects&, - const SkDescriptor*)); - MOCK_CONST_METHOD1(onFilterRec, void(SkScalerContextRec*)); - MOCK_CONST_METHOD0(onGetAdvancedMetrics, - std::unique_ptr()); - MOCK_CONST_METHOD1(getPostScriptGlyphNames, void(SkString*)); - MOCK_CONST_METHOD1(getGlyphToUnicodeMap, void(SkUnichar* dstArray)); - MOCK_CONST_METHOD1(onOpenStream, - std::unique_ptr(int* ttcIndex)); - MOCK_CONST_METHOD2( - onGetVariationDesignPosition, - int(SkFontArguments::VariationPosition::Coordinate coordinates[], - int coordinateCount)); - MOCK_CONST_METHOD2(onGetVariationDesignParameters, - int(SkFontParameters::Variation::Axis parameters[], - int parameterCount)); - MOCK_CONST_METHOD2(onGetFontDescriptor, - void(SkFontDescriptor*, bool* isLocal)); - MOCK_CONST_METHOD3(onCharsToGlyphs, - void(const SkUnichar* chars, - int count, - SkGlyphID glyphs[])); - MOCK_CONST_METHOD0(onCountGlyphs, int()); - MOCK_CONST_METHOD0(onGetUPEM, int()); - MOCK_CONST_METHOD3(onGetKerningPairAdjustments, - bool(const SkGlyphID glyphs[], - int count, - int32_t adjustments[])); - MOCK_CONST_METHOD1(onGetFamilyName, void(SkString* familyName)); - MOCK_CONST_METHOD0(onCreateFamilyNameIterator, SkTypeface::LocalizedStrings*()); - MOCK_CONST_METHOD1(onGetTableTags, int(SkFontTableTag tags[])); - MOCK_CONST_METHOD4( - onGetTableData, - size_t(SkFontTableTag, size_t offset, size_t length, void* data)); - MOCK_CONST_METHOD1(onCopyTableData, sk_sp(SkFontTableTag)); - MOCK_CONST_METHOD1(onComputeBounds, bool(SkRect*)); - MOCK_CONST_METHOD0(onGetCTFontRef, void*()); -}; - -TEST(FontCollectionTest, CheckSkTypefacesSorting) { - MockSkTypeface mockTypeface1; - - sk_sp typeface1{SkTypeface::MakeFromName( - "Arial", - SkFontStyle(SkFontStyle::kThin_Weight, SkFontStyle::kExpanded_Width, - SkFontStyle::kItalic_Slant))}; - sk_sp typeface2{SkTypeface::MakeFromName( - "Arial", - SkFontStyle(SkFontStyle::kLight_Weight, SkFontStyle::kNormal_Width, - SkFontStyle::kUpright_Slant))}; - sk_sp typeface3{SkTypeface::MakeFromName( - "Arial", - SkFontStyle(SkFontStyle::kNormal_Weight, SkFontStyle::kNormal_Width, - SkFontStyle::kUpright_Slant))}; - std::vector> candidateTypefaces{typeface1, typeface2, - typeface3}; +namespace { +// This function does some boilerplate to fill a builder with enough real +// font-like data. Otherwise, detach won't actually build an SkTypeface. +void PopulateUserTypeface(SkCustomTypefaceBuilder* builder) { + constexpr float upem = 200; + + { + SkFontMetrics metrics; + metrics.fFlags = 0; + metrics.fTop = -200; + metrics.fAscent = -150; + metrics.fDescent = 50; + metrics.fBottom = -75; + metrics.fLeading = 10; + metrics.fAvgCharWidth = 150; + metrics.fMaxCharWidth = 300; + metrics.fXMin = -20; + metrics.fXMax = 290; + metrics.fXHeight = -100; + metrics.fCapHeight = 0; + metrics.fUnderlineThickness = 5; + metrics.fUnderlinePosition = 2; + metrics.fStrikeoutThickness = 5; + metrics.fStrikeoutPosition = -50; + builder->setMetrics(metrics, 1.0f/upem); + } - ASSERT_EQ(candidateTypefaces[0]->fontStyle().weight(), - SkFontStyle::kThin_Weight); - ASSERT_EQ(candidateTypefaces[0]->fontStyle().width(), - SkFontStyle::kExpanded_Width); + const SkMatrix scale = SkMatrix::Scale(1.0f/upem, 1.0f/upem); + for (SkGlyphID index = 0; index <= 67; ++index) { + SkScalar width; + width = 100; + SkPath path; + path.addCircle(50, -50, 75); - ASSERT_EQ(candidateTypefaces[1]->fontStyle().weight(), - SkFontStyle::kLight_Weight); - ASSERT_EQ(candidateTypefaces[1]->fontStyle().width(), - SkFontStyle::kNormal_Width); + builder->setGlyph(index, width/upem, path.makeTransform(scale)); + } +} +} - ASSERT_EQ(candidateTypefaces[2]->fontStyle().weight(), - SkFontStyle::kNormal_Weight); - ASSERT_EQ(candidateTypefaces[2]->fontStyle().width(), - SkFontStyle::kNormal_Width); +TEST(FontCollectionTest, CheckSkTypefacesSorting) { + // We have to make a real SkTypeface here. Not all the structs from the + // SkTypeface headers are fully declared to be able to gmock. + // SkCustomTypefaceBuilder is the simplest way to get a simple SkTypeface. + SkCustomTypefaceBuilder typefaceBuilder1; + typefaceBuilder1.setFontStyle(SkFontStyle(SkFontStyle::kThin_Weight, SkFontStyle::kExpanded_Width, + SkFontStyle::kItalic_Slant)); + // For the purpose of this test, we need to fill this to make the SkTypeface + // build but it doesn't matter. We only care about the SkFontStyle. + PopulateUserTypeface(&typefaceBuilder1); + sk_sp typeface1{typefaceBuilder1.detach()}; + + SkCustomTypefaceBuilder typefaceBuilder2; + typefaceBuilder2.setFontStyle(SkFontStyle(SkFontStyle::kLight_Weight, SkFontStyle::kNormal_Width, + SkFontStyle::kUpright_Slant)); + PopulateUserTypeface(&typefaceBuilder2); + sk_sp typeface2{typefaceBuilder2.detach()}; + + SkCustomTypefaceBuilder typefaceBuilder3; + typefaceBuilder3.setFontStyle(SkFontStyle(SkFontStyle::kNormal_Weight, SkFontStyle::kNormal_Width, + SkFontStyle::kUpright_Slant)); + PopulateUserTypeface(&typefaceBuilder3); + sk_sp typeface3{typefaceBuilder3.detach()}; + + std::vector> candidateTypefaces = {typeface1, typeface2, + typeface3}; // This sorts the vector in-place. txt::FontCollection::SortSkTypefaces(candidateTypefaces); - ASSERT_EQ(candidateTypefaces[0].get(), typeface1.get()); - ASSERT_EQ(candidateTypefaces[1].get(), typeface2.get()); - // ASSERT_EQ(candidateTypefaces[0]->fontStyle().weight(), - // SkFontStyle::kLight_Weight); + // The second one is first because it's both the most normal width font + // with the lightest weight. + ASSERT_EQ(candidateTypefaces[0].get(), typeface2.get()); + // Then the most normal width font with normal weight. + ASSERT_EQ(candidateTypefaces[1].get(), typeface3.get()); + // Then a less normal (expanded) width font. + ASSERT_EQ(candidateTypefaces[2].get(), typeface1.get()); + + // Double check. + ASSERT_EQ(candidateTypefaces[0]->fontStyle().weight(), + SkFontStyle::kLight_Weight); ASSERT_EQ(candidateTypefaces[0]->fontStyle().width(), SkFontStyle::kNormal_Width); From 61df1f7ec032f6a25a968c434931a474347c4da4 Mon Sep 17 00:00:00 2001 From: Xiao Yu Date: Thu, 20 Aug 2020 23:25:28 -0700 Subject: [PATCH 6/7] autoformat --- .../txt/tests/font_collection_unittests.cc | 74 ++++++++++--------- 1 file changed, 38 insertions(+), 36 deletions(-) diff --git a/third_party/txt/tests/font_collection_unittests.cc b/third_party/txt/tests/font_collection_unittests.cc index 9bd0ed9b8e391..7ffbdbabeda79 100644 --- a/third_party/txt/tests/font_collection_unittests.cc +++ b/third_party/txt/tests/font_collection_unittests.cc @@ -15,7 +15,6 @@ */ #include "flutter/fml/logging.h" -#include "gmock/gmock.h" #include "gtest/gtest.h" #include "third_party/skia/include/utils/SkCustomTypeface.h" #include "txt/font_collection.h" @@ -34,64 +33,67 @@ void PopulateUserTypeface(SkCustomTypefaceBuilder* builder) { constexpr float upem = 200; { - SkFontMetrics metrics; - metrics.fFlags = 0; - metrics.fTop = -200; - metrics.fAscent = -150; - metrics.fDescent = 50; - metrics.fBottom = -75; - metrics.fLeading = 10; - metrics.fAvgCharWidth = 150; - metrics.fMaxCharWidth = 300; - metrics.fXMin = -20; - metrics.fXMax = 290; - metrics.fXHeight = -100; - metrics.fCapHeight = 0; - metrics.fUnderlineThickness = 5; - metrics.fUnderlinePosition = 2; - metrics.fStrikeoutThickness = 5; - metrics.fStrikeoutPosition = -50; - builder->setMetrics(metrics, 1.0f/upem); - } + SkFontMetrics metrics; + metrics.fFlags = 0; + metrics.fTop = -200; + metrics.fAscent = -150; + metrics.fDescent = 50; + metrics.fBottom = -75; + metrics.fLeading = 10; + metrics.fAvgCharWidth = 150; + metrics.fMaxCharWidth = 300; + metrics.fXMin = -20; + metrics.fXMax = 290; + metrics.fXHeight = -100; + metrics.fCapHeight = 0; + metrics.fUnderlineThickness = 5; + metrics.fUnderlinePosition = 2; + metrics.fStrikeoutThickness = 5; + metrics.fStrikeoutPosition = -50; + builder->setMetrics(metrics, 1.0f / upem); + } - const SkMatrix scale = SkMatrix::Scale(1.0f/upem, 1.0f/upem); - for (SkGlyphID index = 0; index <= 67; ++index) { - SkScalar width; - width = 100; - SkPath path; - path.addCircle(50, -50, 75); + const SkMatrix scale = SkMatrix::Scale(1.0f / upem, 1.0f / upem); + for (SkGlyphID index = 0; index <= 67; ++index) { + SkScalar width; + width = 100; + SkPath path; + path.addCircle(50, -50, 75); - builder->setGlyph(index, width/upem, path.makeTransform(scale)); - } -} + builder->setGlyph(index, width / upem, path.makeTransform(scale)); + } } +} // namespace TEST(FontCollectionTest, CheckSkTypefacesSorting) { // We have to make a real SkTypeface here. Not all the structs from the // SkTypeface headers are fully declared to be able to gmock. // SkCustomTypefaceBuilder is the simplest way to get a simple SkTypeface. SkCustomTypefaceBuilder typefaceBuilder1; - typefaceBuilder1.setFontStyle(SkFontStyle(SkFontStyle::kThin_Weight, SkFontStyle::kExpanded_Width, - SkFontStyle::kItalic_Slant)); + typefaceBuilder1.setFontStyle(SkFontStyle(SkFontStyle::kThin_Weight, + SkFontStyle::kExpanded_Width, + SkFontStyle::kItalic_Slant)); // For the purpose of this test, we need to fill this to make the SkTypeface // build but it doesn't matter. We only care about the SkFontStyle. PopulateUserTypeface(&typefaceBuilder1); sk_sp typeface1{typefaceBuilder1.detach()}; SkCustomTypefaceBuilder typefaceBuilder2; - typefaceBuilder2.setFontStyle(SkFontStyle(SkFontStyle::kLight_Weight, SkFontStyle::kNormal_Width, - SkFontStyle::kUpright_Slant)); + typefaceBuilder2.setFontStyle(SkFontStyle(SkFontStyle::kLight_Weight, + SkFontStyle::kNormal_Width, + SkFontStyle::kUpright_Slant)); PopulateUserTypeface(&typefaceBuilder2); sk_sp typeface2{typefaceBuilder2.detach()}; SkCustomTypefaceBuilder typefaceBuilder3; - typefaceBuilder3.setFontStyle(SkFontStyle(SkFontStyle::kNormal_Weight, SkFontStyle::kNormal_Width, - SkFontStyle::kUpright_Slant)); + typefaceBuilder3.setFontStyle(SkFontStyle(SkFontStyle::kNormal_Weight, + SkFontStyle::kNormal_Width, + SkFontStyle::kUpright_Slant)); PopulateUserTypeface(&typefaceBuilder3); sk_sp typeface3{typefaceBuilder3.detach()}; std::vector> candidateTypefaces = {typeface1, typeface2, - typeface3}; + typeface3}; // This sorts the vector in-place. txt::FontCollection::SortSkTypefaces(candidateTypefaces); From 6959e74784408086e1d5f2de4c11055dc32cecbf Mon Sep 17 00:00:00 2001 From: Xiao Yu Date: Mon, 24 Aug 2020 09:09:50 -0700 Subject: [PATCH 7/7] have predictable cascading sort order --- third_party/txt/src/txt/font_collection.cc | 58 +++++++++++-------- .../txt/tests/font_collection_unittests.cc | 29 +++++++--- 2 files changed, 55 insertions(+), 32 deletions(-) diff --git a/third_party/txt/src/txt/font_collection.cc b/third_party/txt/src/txt/font_collection.cc index e82b8d09ecaef..31c3ba6d25ac1 100644 --- a/third_party/txt/src/txt/font_collection.cc +++ b/third_party/txt/src/txt/font_collection.cc @@ -204,31 +204,39 @@ std::shared_ptr FontCollection::FindFontFamilyInManagers( void FontCollection::SortSkTypefaces( std::vector>& sk_typefaces) { - std::sort(sk_typefaces.begin(), sk_typefaces.end(), - [](const sk_sp& a, const sk_sp& b) { - SkFontStyle a_style = a->fontStyle(); - SkFontStyle b_style = b->fontStyle(); - - if (a_style.width() != b_style.width()) { - // If a family name query is so generic it ends up bringing in - // fonts of multiple widths (e.g. condensed, expanded), opt to - // be conservative and select the most standard width. - // - // If a specific width is desired, it should be be narrowed down - // via the family name. - // - // The font weights are also sorted lightest to heaviest but - // Flutter APIs have the weight specified to narrow it down - // later. The width ordering here is more consequential since - // TextStyle doesn't have letter width APIs. - return std::abs(a_style.width() - SkFontStyle::kNormal_Width) < - std::abs(b_style.width() - SkFontStyle::kNormal_Width); - } else { - return (a_style.weight() != b_style.weight()) - ? a_style.weight() < b_style.weight() - : a_style.slant() < b_style.slant(); - } - }); + std::sort( + sk_typefaces.begin(), sk_typefaces.end(), + [](const sk_sp& a, const sk_sp& b) { + SkFontStyle a_style = a->fontStyle(); + SkFontStyle b_style = b->fontStyle(); + + int a_delta = std::abs(a_style.width() - SkFontStyle::kNormal_Width); + int b_delta = std::abs(b_style.width() - SkFontStyle::kNormal_Width); + + if (a_delta != b_delta) { + // If a family name query is so generic it ends up bringing in fonts + // of multiple widths (e.g. condensed, expanded), opt to be + // conservative and select the most standard width. + // + // If a specific width is desired, it should be be narrowed down via + // the family name. + // + // The font weights are also sorted lightest to heaviest but Flutter + // APIs have the weight specified to narrow it down later. The width + // ordering here is more consequential since TextStyle doesn't have + // letter width APIs. + return a_delta < b_delta; + } else if (a_style.width() != b_style.width()) { + // However, if the 2 fonts are equidistant from the "normal" width, + // just arbitrarily but consistently return the more condensed font. + return a_style.width() < b_style.width(); + } else if (a_style.weight() != b_style.weight()) { + return a_style.weight() < b_style.weight(); + } else { + return a_style.slant() < b_style.slant(); + } + // Use a cascade of conditions so results are consistent each time. + }); } std::shared_ptr FontCollection::CreateMinikinFontFamily( diff --git a/third_party/txt/tests/font_collection_unittests.cc b/third_party/txt/tests/font_collection_unittests.cc index 7ffbdbabeda79..05c055809f607 100644 --- a/third_party/txt/tests/font_collection_unittests.cc +++ b/third_party/txt/tests/font_collection_unittests.cc @@ -29,7 +29,7 @@ class FontCollectionTest : public ::testing::Test {}; namespace { // This function does some boilerplate to fill a builder with enough real // font-like data. Otherwise, detach won't actually build an SkTypeface. -void PopulateUserTypeface(SkCustomTypefaceBuilder* builder) { +void PopulateUserTypefaceBoilerplate(SkCustomTypefaceBuilder* builder) { constexpr float upem = 200; { @@ -75,25 +75,32 @@ TEST(FontCollectionTest, CheckSkTypefacesSorting) { SkFontStyle::kItalic_Slant)); // For the purpose of this test, we need to fill this to make the SkTypeface // build but it doesn't matter. We only care about the SkFontStyle. - PopulateUserTypeface(&typefaceBuilder1); + PopulateUserTypefaceBoilerplate(&typefaceBuilder1); sk_sp typeface1{typefaceBuilder1.detach()}; SkCustomTypefaceBuilder typefaceBuilder2; typefaceBuilder2.setFontStyle(SkFontStyle(SkFontStyle::kLight_Weight, SkFontStyle::kNormal_Width, SkFontStyle::kUpright_Slant)); - PopulateUserTypeface(&typefaceBuilder2); + PopulateUserTypefaceBoilerplate(&typefaceBuilder2); sk_sp typeface2{typefaceBuilder2.detach()}; SkCustomTypefaceBuilder typefaceBuilder3; typefaceBuilder3.setFontStyle(SkFontStyle(SkFontStyle::kNormal_Weight, SkFontStyle::kNormal_Width, SkFontStyle::kUpright_Slant)); - PopulateUserTypeface(&typefaceBuilder3); + PopulateUserTypefaceBoilerplate(&typefaceBuilder3); sk_sp typeface3{typefaceBuilder3.detach()}; + SkCustomTypefaceBuilder typefaceBuilder4; + typefaceBuilder4.setFontStyle(SkFontStyle(SkFontStyle::kThin_Weight, + SkFontStyle::kCondensed_Width, + SkFontStyle::kUpright_Slant)); + PopulateUserTypefaceBoilerplate(&typefaceBuilder4); + sk_sp typeface4{typefaceBuilder4.detach()}; + std::vector> candidateTypefaces = {typeface1, typeface2, - typeface3}; + typeface3, typeface4}; // This sorts the vector in-place. txt::FontCollection::SortSkTypefaces(candidateTypefaces); @@ -103,8 +110,11 @@ TEST(FontCollectionTest, CheckSkTypefacesSorting) { ASSERT_EQ(candidateTypefaces[0].get(), typeface2.get()); // Then the most normal width font with normal weight. ASSERT_EQ(candidateTypefaces[1].get(), typeface3.get()); - // Then a less normal (expanded) width font. - ASSERT_EQ(candidateTypefaces[2].get(), typeface1.get()); + // Then a less normal (condensed) width font. + ASSERT_EQ(candidateTypefaces[2].get(), typeface4.get()); + // All things equal, 4 came before 1 because we arbitrarily chose to make the + // narrower font come first. + ASSERT_EQ(candidateTypefaces[3].get(), typeface1.get()); // Double check. ASSERT_EQ(candidateTypefaces[0]->fontStyle().weight(), @@ -120,6 +130,11 @@ TEST(FontCollectionTest, CheckSkTypefacesSorting) { ASSERT_EQ(candidateTypefaces[2]->fontStyle().weight(), SkFontStyle::kThin_Weight); ASSERT_EQ(candidateTypefaces[2]->fontStyle().width(), + SkFontStyle::kCondensed_Width); + + ASSERT_EQ(candidateTypefaces[3]->fontStyle().weight(), + SkFontStyle::kThin_Weight); + ASSERT_EQ(candidateTypefaces[3]->fontStyle().width(), SkFontStyle::kExpanded_Width); }