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

fontdir: Consider scalable fonts in index fonts.dir #96697

Merged
merged 8 commits into from Oct 5, 2020

Conversation

@lyodaom
Copy link
Contributor

@lyodaom lyodaom commented Aug 30, 2020

mkfontdir requires the file fonts.scale to consider scalable fonts, thus, mkfontscale should be run before.

This change affects to everyone using the option enableFontDir

Motivation for this change
  • The need to use scalable fonts in an old application (using the X11 core font API instead of font-config)
Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.
@rnhmjoj
Copy link
Contributor

@rnhmjoj rnhmjoj commented Aug 31, 2020

mkfontdir requires the file fonts.scale to consider scalable fonts, thus, mkfontscale should be run before.

Ah, I've always thought fonts.dir only included bitmap fonts and fonts.scale vector fonts, but mkfontdir(1) is pretty clear about this: thanks. I wonder if we whould change the order everywhere, there are tons of packages calling the two commands in the wrong order.

In any case, I'm not familiar with this modules: IIUC it should glue all fonts.{scale,dir,alias} together.
I don't understand some of the steps it's doing:

  1. collects all fonts.dir, ttf and otf fonts to /share/X11-fonts (why not fonts.scale as well?)
  2. removes all `fonts.{dir,scale,alias}
  3. runs mkfont{dir,scale} in /share/X11-fonts (won't this only detect scalable fonts? the find command is not linking pfa, pfb, pcf, bdf, etc..)
  4. concatenates all fonts.alias files and saves to /share/X11-fonts (makes sense)

@lyodaom
Copy link
Contributor Author

@lyodaom lyodaom commented Aug 31, 2020

Hi @rnhmjoj and thanks for your comment,

  1. and 2. this part could be improved, we don't need to link fonts.dir if we are going to delete it in the next line
    The reason why we don't use the fonts.dir and fonts.scale provided by font packages is because they might not provide it, and thus it is safer to recreate them after we collect them all in the same directory
  2. You are also right about this, I'm not sure if avoiding bit-mapped fonts is intentional, I think that it would make more sense to collect all the fonts specified by the user (even if they are bit-mapped)

I could add one more commit to fix those other issues, I'll start testing it

@rnhmjoj
Copy link
Contributor

@rnhmjoj rnhmjoj commented Aug 31, 2020

I could add one more commit to fix those other issues, I'll start testing it

That would be great, thanks. I also wanted to ask: what are you using to test the change? I'd like to try it myself.

@rnhmjoj
Copy link
Contributor

@rnhmjoj rnhmjoj commented Aug 31, 2020

Sorry, I meant which program you were using to display the fonts. I only know of xfontsel, which uses the X11 font API.

@lyodaom
Copy link
Contributor Author

@lyodaom lyodaom commented Aug 31, 2020

I'm using the same one for displaying, I also use xlsfonts to confirm the font was taken by X server(in my case Xwayland)

@lyodaom
Copy link
Contributor Author

@lyodaom lyodaom commented Aug 31, 2020

I added the most common bit-mapped font formats (which very often are provided compressed), however I was tempted to just search for all regular files.

@lyodaom
Copy link
Contributor Author

@lyodaom lyodaom commented Aug 31, 2020

By the way, you probably need to add the X11-fonts package folder as the only font-path by using xset fp= /path/to/x11-fonts-output/share/X11-fonts, for some reason it doesn't allow to do so with /run/current-system/sw/share/X11-fonts, I suspect it doesn't like the fact that fonts.dir is a link

@lyodaom lyodaom force-pushed the fix-font-dir branch 2 times, most recently from c3a04a2 to 97ff135 Aug 31, 2020
@lyodaom lyodaom force-pushed the fix-font-dir branch 4 times, most recently from 92d750f to 1f1ee0a Aug 31, 2020
@rnhmjoj
Copy link
Contributor

@rnhmjoj rnhmjoj commented Sep 1, 2020

Ok, I figured how to add the font path without manually doing xset fp= . I'll push the changes here.

@lyodaom
Copy link
Contributor Author

@lyodaom lyodaom commented Sep 1, 2020

Nice, I'll test that in Xwayland, I think the behavior is a bit different. I suspect it doesn't handle compressed font files.

@lyodaom
Copy link
Contributor Author

@lyodaom lyodaom commented Sep 1, 2020

Yes, as I said, Xwayland doesn't handle compressed font files (at least when FontPath is specified via xset), the font is listed in a XListFonts call, but if you try to open the font, you'd get a null pointer.
The second issue I had is: I coundn't find a way to configure a FontPath in Xwayland, it seems like it doesn't consider xorg.conf FontPaths.
Probably they are not issues to fix in this PR, but I thought it was interesting to know.

@rnhmjoj
Copy link
Contributor

@rnhmjoj rnhmjoj commented Sep 1, 2020

Unfortunately I don't use Xwayland so I can't really help.

@lyodaom
Copy link
Contributor Author

@lyodaom lyodaom commented Sep 1, 2020

What do you think about uncompressing fonts in the X11-fonts script? it would solve an issue for Xwayland users

@rnhmjoj
Copy link
Contributor

@rnhmjoj rnhmjoj commented Sep 1, 2020

Uhm, it's not very clean but if it's the only way I would add another option, off by default because it can be quite expensive if there are a lot of fonts installed.

If we are adding more options to this module we may also have to rename

fonts.enableFontdir → fonts.fontDir.enable
fonts.fontDir.decompressFonts

@lyodaom
Copy link
Contributor Author

@lyodaom lyodaom commented Sep 3, 2020

I rebased on top of master to fix one of the checks

@rnhmjoj
Copy link
Contributor

@rnhmjoj rnhmjoj commented Sep 4, 2020

Thank you! I made some additional fixups:

  • Added a mkRenamedOptionModule to make the option rename backward compatible
  • Mentioned the changes in the release notes for 20.09
  • Linked /share/X11 when Xwayland is enabled.

rnhmjoj
rnhmjoj approved these changes Sep 4, 2020
Copy link
Contributor

@rnhmjoj rnhmjoj left a comment

I checked a final time with XWayland (in sway) and Xorg with xlsfonts: everything seems to work as expected. This could use another pair of eyes but it looks ready to go for me.

@lyodaom
Copy link
Contributor Author

@lyodaom lyodaom commented Sep 4, 2020

Thank you for reviewing and for all the fixes.

@nixos-discourse
Copy link

@nixos-discourse nixos-discourse commented Sep 7, 2020

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/prs-ready-for-review/3032/297

nixos/modules/programs/xwayland.nix Outdated Show resolved Hide resolved
nixos/modules/services/x11/xserver.nix Outdated Show resolved Hide resolved
@lyodaom
Copy link
Contributor Author

@lyodaom lyodaom commented Sep 7, 2020

Hi @erikarvstedt and thanks for reviewing, I rebased one more time to have those changes in its related commit

@rnhmjoj
Copy link
Contributor

@rnhmjoj rnhmjoj commented Sep 27, 2020

I think we are far too late for releasing this into 20.09. Can you move the release note into 21.03?

lyodaom and others added 8 commits Oct 4, 2020
- Fix wrong order in which font indexes are created
mkfontdir requires the file fonts.scale to consider scalable fonts,
thus, mkfontscale should be run before

- Search more font formats, in particular, bit-mapped formats
Renaming enableFontDir to fontDir.enable
This will let Xwayland use the global font folder as font path
- Add option `programs.xwayland.defaultFontPath`
- Modify sway to enable Xwayland
@rnhmjoj
Copy link
Contributor

@rnhmjoj rnhmjoj commented Oct 5, 2020

Thank you for updating the note. The PR looked already good to me but I also run a bunch of X.org related tests to be use: everything seems ok. I'm going to merge.

@rnhmjoj rnhmjoj merged commit 04670f8 into NixOS:master Oct 5, 2020
17 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants