Skip to content
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

Stop fontconfig from loading from host’s /etc/fonts/conf.d #4257

Merged
merged 1 commit into from
Sep 21, 2023

Conversation

gunnarhj
Copy link
Contributor

@gunnarhj gunnarhj commented Jul 4, 2023

Fixes: https://launchpad.net/bugs/2025651

At the moment the snaps in at least lunar and mantic load font configuration both from gnome-42-2204 and from the host system’s /etc/fonts/conf.d. This has proved to confuse fontconfig and result in unexpected issues. Stopping the snaps from loading from /etc/fonts/conf.d seems to fix the issue.

This PR reverts part of 407dc755, which is sad IMO. But it’s a result of the reluctance to use font configuration from the host system as expressed in this comment, and for now the proposed change is the easiest way to stop fontconfig from being confused.

  • Have you followed the guidelines for contributing?
  • Have you signed the CLA?
  • Have you successfully run make lint?
  • Have you successfully run pytest tests/unit?

Copy link
Contributor

@seb128 seb128 left a comment

Choose a reason for hiding this comment

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

Thanks Gunnar, I was looking at doing a similar change and asked James for his opinion today and he seemed to agree it was the right thing to do.

Note that the wrapper in snapcraft isn't used anymore in core22, that's confusing and #4250 should help clarifying. Could you also submit the fix against https://github.com/snapcore/snapcraft-desktop-integration/ ?

I've tested the change in firefox and can confirm that
$ LC_CTYPE=ja_JP.UTF-8 fc-match -s sans-serif
correctly returns NotoSansCJK-Regular.ttc as preferred choice now

@gunnarhj
Copy link
Contributor Author

gunnarhj commented Jul 4, 2023

@seb128: Oh. I submitted canonical/snapcraft-desktop-integration#13 too. Should this one be closed then?

@seb128
Copy link
Contributor

seb128 commented Jul 4, 2023

@seb128: Oh. I submitted snapcore/snapcraft-desktop-integration#13 too. Should this one be closed then?

No because as said core20 snaps still use the snapcraft scripts and we still have core20 based snaps (firefox/stable for example)

@gunnarhj
Copy link
Contributor Author

gunnarhj commented Jul 4, 2023

Should this one be closed then?

No because as said core20 snaps still use the snapcraft scripts and we still have core20 based snaps (firefox/stable for example)

But we haven't seen any issue with core20 snaps, have we? After all, people may have put things in /etc/fonts/conf.d which successfully has been honored by the snaps too. Is that kind of regression risk worth it?

@seb128
Copy link
Contributor

seb128 commented Jul 5, 2023

Testing firefox/stable (based on core20) it seems it returns the same wrong font for japanese? But yes you are right that we didn't get real complains until now so for stability reason we might to do the change only for core22...

@gunnarhj
Copy link
Contributor Author

gunnarhj commented Jul 5, 2023

It's not black and white. Consistency is good, which speaks for merging this PR. I have no strong opinion.

As regards why this became a problem in lunar, it seems to me that the move from /etc to /usr in the fontconfig-config and language-selector-common packages is an important factor together with the fact that the snaps do not mount /usr/share/fontconfig. The latter was recently proposed in canonical/snapd#12872 but rejected.

@gunnarhj
Copy link
Contributor Author

gunnarhj commented Jul 6, 2023

I tested the proposed change on FF jammy / core20, and it didn't fall out well, so suddenly I have a strong opinion. 😉

Before change

$ LC_CTYPE=zh_CN.UTF-8 fc-match
NotoSansCJK-Regular.ttc: "Noto Sans CJK SC" "Regular"
$ LC_CTYPE=ja_JP.UTF-8 fc-match
NotoSansCJK-Regular.ttc: "Noto Sans CJK JP" "Regular"
$ LC_CTYPE=ar_EG.UTF-8 fc-match
NotoSansArabicUI-Regular.ttf: "Noto Sans Arabic UI" "Regular"

(I have fonts-noto-ui-core installed which explains that expected Arabic result.)

After change

$ LC_CTYPE=zh_CN.UTF-8 fc-match
NotoSansCJK-Regular.ttc: "Noto Sans CJK SC" "Regular"
$ LC_CTYPE=ja_JP.UTF-8 fc-match
ARIALUNI.TTF: "Arial Unicode MS" "Regular"
$ LC_CTYPE=ar_EG.UTF-8 fc-match
DejaVuSans.ttf: "DejaVu Sans" "Book"

Chinese was 'saved' through the presence of 70-fonts-noto-cjk.conf, but it picks undesired fonts for Japanese and Arabic.

I think the reason is that language-selector-common is missing here:
https://github.com/ubuntu/gnome-sdk/blob/gnome-3-38-2004/snapcraft.yaml#L171

So if we would drop the system’s /etc/fonts/conf.d, 56-language-selector-ar.conf and 64-language-selector-prefer.conf (for example) would be absent, and we would introduce issues in latest/stable/ubuntu-22.04 which are not present at the moment.

If it is desirable to make FF jammy / core20 consistent with lunar and mantic in this respect, language-selector-common needs to be added to this package list. Only after that this PR can be merged without bad effects.

@seb128
Copy link
Contributor

seb128 commented Jul 6, 2023

Thanks Gunnar. At least I had added language-selector-common to the list of packages when I submitted the PR to include the fonts config in gnome-42-2204 but that's a good sign than we should have some process to ensure the fonts configuration in the sdk is updated when we do change in Ubuntu

@gunnarhj
Copy link
Contributor Author

gunnarhj commented Jul 6, 2023

@seb128: I just created ubuntu/gnome-sdk#157 . Please confirm that it's a reasonable step to take.

@codecov-commenter
Copy link

Codecov Report

Merging #4257 (723c715) into main (fc985d4) will increase coverage by 0.01%.
The diff coverage is n/a.

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

@@            Coverage Diff             @@
##             main    #4257      +/-   ##
==========================================
+ Coverage   88.96%   88.98%   +0.01%     
==========================================
  Files         295      296       +1     
  Lines       20151    20192      +41     
==========================================
+ Hits        17928    17968      +40     
- Misses       2223     2224       +1     

see 3 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@gunnarhj
Copy link
Contributor Author

@kenvandine: Do you have a minute to merge this?

@sergiusens sergiusens merged commit 7877ca5 into canonical:main Sep 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants