Description: Whilst testing font match matching, I uncovered some issues. 1. The matching algorithm detects that the font is a CJK font if there are any CJK characters in the font name. However, if you searched for a CJK font against a font that is not a CJK font (i.e. the family name has no CJK characters) then it will still match against this font family. 2. The rounded, typewriter, gothic, schoolbook and titling/captilization checks will always succeed, regardless of what it is matched against. 3. When checking if a font family is fixed, we also check if it has a typewriter attribute - this code is not reachable when using a font like typewriter or lineprinter which uses Typewriter and Fixed attributes. 4. Despite using the search parameter in FindFontFamilyByAttributes() it will aways return a font regardless of whether the search font exists or not. Steps to Reproduce: Exposed via unit testing. Actual Results: Unit tests I wrote failed. Expected Results: Unit tests should pass. Reproducible: Always User Profile Reset: No Additional Info: This is across all LibreOffice builds.
Issues uncovered at: https://gerrit.libreoffice.org/c/core/+/122036/
Patch to fix submitted here: https://gerrit.libreoffice.org/c/core/+/122836/
(In reply to Chris Sherlock from comment #0) > 3. When checking if a font family is fixed, we also check if it has a > typewriter attribute - this code is not reachable when using a font > like typewriter or lineprinter which uses Typewriter and Fixed > attributes. I wonder if fixing this would affect monospace font detection on macOS - bug 144757.
Nice to have you found and working on these; just please do that in separate changes, so each of the issue has an own commit. Thank you!
I'll see what I can do.
Mike, have split the patches: - Tests (including failing, disabled tests) are in this patch: https://gerrit.libreoffice.org/c/core/+/123308 - Fix for the CJK matching is in this fix: https://gerrit.libreoffice.org/c/core/+/123309/ - Refactoring to make it easier to make the fixes: https://gerrit.libreoffice.org/c/core/+/122836/ - rounding and other style match fixes: https://gerrit.libreoffice.org/c/core/+/123310/ - fix for GetFixedMatchValue(): https://gerrit.libreoffice.org/c/core/+/123311/ - followup fix for other issues I located: https://gerrit.libreoffice.org/c/core/+/123311/
Oh, and just a note: I have added SAL_INFO debug messages into the code so you can see how the matching works. So, you can see the output of font matching calculation via setting the following environment variable: export SAL_LOG='+INFO.vcl.fonts'
An example of what you can see when setting the export variable (tests that search for CJK fonts - first is meant to match, second is meant to not match): [_RUN_____] VclPhysicalFontCollectionTest::testShouldFindCJKFamily info:vcl.fonts:62376:17953849:vcl/source/font/PhysicalFontCollection.cxx:443: GetCJKMatchValue: 20000000 info:vcl.fonts:62376:17953849:vcl/source/font/PhysicalFontCollection.cxx:467: GetCTLMatchValue: 0 info:vcl.fonts:62376:17953849:vcl/source/font/PhysicalFontCollection.cxx:487: GetNoneLatinMatchValue: 0 info:vcl.fonts:62376:17953849:vcl/source/font/PhysicalFontCollection.cxx:537: GetSymbolMatchValue: 0 info:vcl.fonts:62376:17953849:vcl/source/font/PhysicalFontCollection.cxx:551: GetFamilyNameMatchValue: 0 info:vcl.fonts:62376:17953849:vcl/source/font/PhysicalFontCollection.cxx:586: GetScriptMatchValue: 0 info:vcl.fonts:62376:17953849:vcl/source/font/PhysicalFontCollection.cxx:610: GetFixedMatchValue: 0 info:vcl.fonts:62376:17953849:vcl/source/font/PhysicalFontCollection.cxx:639: GetSpecialMatchValue: 0 info:vcl.fonts:62376:17953849:vcl/source/font/PhysicalFontCollection.cxx:669: GetDecorativeMatchValue: 0 info:vcl.fonts:62376:17953849:vcl/source/font/PhysicalFontCollection.cxx:699: GetTitlingCapitalsMatchValue: 0 info:vcl.fonts:62376:17953849:vcl/source/font/PhysicalFontCollection.cxx:731: GetOutlineShadowMatchValue: 0 info:vcl.fonts:62376:17953849:vcl/source/font/PhysicalFontCollection.cxx:750: GetFontNameMatchValue: 0 info:vcl.fonts:62376:17953849:vcl/source/font/PhysicalFontCollection.cxx:767: GetSerifMatchValue: 0 info:vcl.fonts:62376:17953849:vcl/source/font/PhysicalFontCollection.cxx:784: GetSansSerifMatchValue: 0 info:vcl.fonts:62376:17953849:vcl/source/font/PhysicalFontCollection.cxx:809: GetItalicMatchValue: 0 info:vcl.fonts:62376:17953849:vcl/source/font/PhysicalFontCollection.cxx:840: GetWidthMatchValue: 0 info:vcl.fonts:62376:17953849:vcl/source/font/PhysicalFontCollection.cxx:877: GetWeightMatchValue: 0 info:vcl.fonts:62376:17953849:vcl/source/font/PhysicalFontCollection.cxx:892: GetScalableMatchValue: 40000 info:vcl.fonts:62376:17953849:vcl/source/font/PhysicalFontCollection.cxx:904: GetStandardMatchValue: 0 info:vcl.fonts:62376:17953849:vcl/source/font/PhysicalFontCollection.cxx:916: GetDefaultMatchValue: 0 info:vcl.fonts:62376:17953849:vcl/source/font/PhysicalFontCollection.cxx:928: GetFullMatchValue: 0 info:vcl.fonts:62376:17953849:vcl/source/font/PhysicalFontCollection.cxx:940: GetNormalMatchValue: 0 info:vcl.fonts:62376:17953849:vcl/source/font/PhysicalFontCollection.cxx:952: GetOtherMatchValue: 0 info:vcl.fonts:62376:17953849:vcl/source/font/PhysicalFontCollection.cxx:964: GetRoundedMatchValue: 0 info:vcl.fonts:62376:17953849:vcl/source/font/PhysicalFontCollection.cxx:976: GetTypewriterMatchValue: 0 info:vcl.fonts:62376:17953849:vcl/source/font/PhysicalFontCollection.cxx:994: GetGothicMatchValue: 0 info:vcl.fonts:62376:17953849:vcl/source/font/PhysicalFontCollection.cxx:1012: GetSchoolbookMatchValue: 0 info:vcl.fonts:62376:17953849:vcl/source/font/PhysicalFontCollection.cxx:1091: Total match value: 20040000 VclPhysicalFontCollectionTest::testShouldFindCJKFamily finished in: 11ms [_RUN_____] VclPhysicalFontCollectionTest::testShouldNotFindCJKFamily info:vcl.fonts:62376:17953849:vcl/source/font/PhysicalFontCollection.cxx:443: GetCJKMatchValue: -10000000 info:vcl.fonts:62376:17953849:vcl/source/font/PhysicalFontCollection.cxx:467: GetCTLMatchValue: 0 info:vcl.fonts:62376:17953849:vcl/source/font/PhysicalFontCollection.cxx:487: GetNoneLatinMatchValue: 0 info:vcl.fonts:62376:17953849:vcl/source/font/PhysicalFontCollection.cxx:537: GetSymbolMatchValue: 0 info:vcl.fonts:62376:17953849:vcl/source/font/PhysicalFontCollection.cxx:551: GetFamilyNameMatchValue: 0 info:vcl.fonts:62376:17953849:vcl/source/font/PhysicalFontCollection.cxx:586: GetScriptMatchValue: 0 info:vcl.fonts:62376:17953849:vcl/source/font/PhysicalFontCollection.cxx:610: GetFixedMatchValue: 0 info:vcl.fonts:62376:17953849:vcl/source/font/PhysicalFontCollection.cxx:639: GetSpecialMatchValue: 0 info:vcl.fonts:62376:17953849:vcl/source/font/PhysicalFontCollection.cxx:669: GetDecorativeMatchValue: 0 info:vcl.fonts:62376:17953849:vcl/source/font/PhysicalFontCollection.cxx:699: GetTitlingCapitalsMatchValue: 0 info:vcl.fonts:62376:17953849:vcl/source/font/PhysicalFontCollection.cxx:731: GetOutlineShadowMatchValue: 0 info:vcl.fonts:62376:17953849:vcl/source/font/PhysicalFontCollection.cxx:750: GetFontNameMatchValue: 0 info:vcl.fonts:62376:17953849:vcl/source/font/PhysicalFontCollection.cxx:767: GetSerifMatchValue: 0 info:vcl.fonts:62376:17953849:vcl/source/font/PhysicalFontCollection.cxx:784: GetSansSerifMatchValue: 0 info:vcl.fonts:62376:17953849:vcl/source/font/PhysicalFontCollection.cxx:809: GetItalicMatchValue: 0 info:vcl.fonts:62376:17953849:vcl/source/font/PhysicalFontCollection.cxx:840: GetWidthMatchValue: 0 info:vcl.fonts:62376:17953849:vcl/source/font/PhysicalFontCollection.cxx:877: GetWeightMatchValue: 0 info:vcl.fonts:62376:17953849:vcl/source/font/PhysicalFontCollection.cxx:892: GetScalableMatchValue: 40000 info:vcl.fonts:62376:17953849:vcl/source/font/PhysicalFontCollection.cxx:904: GetStandardMatchValue: 0 info:vcl.fonts:62376:17953849:vcl/source/font/PhysicalFontCollection.cxx:916: GetDefaultMatchValue: 0 info:vcl.fonts:62376:17953849:vcl/source/font/PhysicalFontCollection.cxx:928: GetFullMatchValue: 0 info:vcl.fonts:62376:17953849:vcl/source/font/PhysicalFontCollection.cxx:940: GetNormalMatchValue: 0 info:vcl.fonts:62376:17953849:vcl/source/font/PhysicalFontCollection.cxx:952: GetOtherMatchValue: 0 info:vcl.fonts:62376:17953849:vcl/source/font/PhysicalFontCollection.cxx:964: GetRoundedMatchValue: 0 info:vcl.fonts:62376:17953849:vcl/source/font/PhysicalFontCollection.cxx:976: GetTypewriterMatchValue: 0 info:vcl.fonts:62376:17953849:vcl/source/font/PhysicalFontCollection.cxx:994: GetGothicMatchValue: 0 info:vcl.fonts:62376:17953849:vcl/source/font/PhysicalFontCollection.cxx:1012: GetSchoolbookMatchValue: 0 info:vcl.fonts:62376:17953849:vcl/source/font/PhysicalFontCollection.cxx:1091: Total match value: -9960000
Bug 144757 is an unrelated issue around not setting the pitch corrrectly on MacOS, I have a patch queued up for this one so hopefully that will be resolved also.
(In reply to Chris Sherlock from comment #9) Thanks for looking into it! (Unfortunately, I haven't a macOS, so can't check, and we need to wait until someone checks - or did you tested it yourself? It looks good for me - I'd just push if you know it's OK)
FWIW, there are some fixes for these issues, but I will need to wait for someone else to review them.
I have no commit rights.
Chris Sherlock committed a patch related to this issue. It has been pushed to "master": https://git.libreoffice.org/core/commit/97ef7881cec4f7baa1ef8e7b6b84b6848f8c5f4a tdf#144961 vcl: test PhysicalFontCollection::FindFontFamilyByAttributes() It will be available in 7.3.0. The patch should be included in the daily builds available at https://dev-builds.libreoffice.org/daily/ in the next 24-48 hours. More information about daily builds can be found at: https://wiki.documentfoundation.org/Testing_Daily_Builds Affected users are encouraged to test the fix and report feedback.
Hossein committed a patch related to this issue. It has been pushed to "master": https://git.libreoffice.org/core/commit/ddd675c5a79b26d23da8d4affeeb888ac3664635 tdf#144961 vcl: fix CJK matching in FindFontFamilyByAttributes It will be available in 7.4.0. The patch should be included in the daily builds available at https://dev-builds.libreoffice.org/daily/ in the next 24-48 hours. More information about daily builds can be found at: https://wiki.documentfoundation.org/Testing_Daily_Builds Affected users are encouraged to test the fix and report feedback.
Some background to this bug: the platform specific code populates a PhysicalFontCollection with its fonts via the SalGraphics function GetDevFontList(). AquaSalGraphics::GetDevFontList() is probably a good example of how this works: 1. GetDevFontList() passes in a PhysicalFontCollection pointer, this is what will be populated with the MacOS system fonts. 2. The list of core fonts is assigned to a SalData mpFontList structure (a SystemFontList) 3. These fonts are then “announced” do the system - basically the SystemFontList has a collection of PhysicalFontFaces, which it adds to the PhysicalFontCollection via PhysicalFontCollection::Add() 4. At this point, Add() attempts to find or create a font family via PhysicalFontCollection::FindOrCreateFontFamily() - it takes the PhysicalFontFace family name string, searches to see if this exists already (if not then it creates a new PhysicalFontFamily which it adds to its font family collection) and returns the PhysicalFontFamily. Interestingly, only the search name is populated. It is the next step that actually populates the family name... 5. It then uses this PhysicalFontFamily to add a font face, via PhysicalFontFamily::AddFontFace(). This checks if PhysicalFontFamily’s font face collection has been already been populated, if not then it sets a variety of family attributes, otherwise it checks and populates a number of attributes that are unknown up to this point. AddFontFace populates a bunch of other family attributes (e.g. whether it is symbol font, the weight, italics, etc) and then adds the font face if necessary (note there might already be a font face, might be considered better than the one being added). PhysicalFontCollection allows you to search for fonts by attributes. There is a complex matching algorithm that I have documented with my unit tests I submitted. The function that does the searching is PhysicalFontCollection::FindFontFamilyByAttributes() - it has a number of parameters that influences the search: * The font attributes to search on (e.g. you want to find a Symbol font) * The font weight * The font width * The font italics * The search name of the font An exhaustive search is done for every font family in the collection, each font is given a font score and the best font is determined based on this score. My tests found that a number of searches were not correctly being matched.
FWIW: this patch apparently fails because of font matching issues.. https://gerrit.libreoffice.org/c/core/+/127877 And well this bug is all about font matching.. No clue if this has been discussed internally already.. or if there is any relation.. So only mentioning..
(In reply to Telesto from comment #16) > FWIW: this patch apparently fails because of font matching issues.. > https://gerrit.libreoffice.org/c/core/+/127877 > And well this bug is all about font matching.. No clue if this has been > discussed internally already.. or if there is any relation.. So only > mentioning.. https://gerrit.libreoffice.org/c/core/+/127877 has nothing to do with this
Dear Chris Sherlock, This bug has been in ASSIGNED status for more than 3 months without any activity. Resetting it to NEW. Please assign it back to yourself if you're still working on this.