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

Systemfonts and friends #128

Merged
merged 86 commits into from
May 4, 2021
Merged

Systemfonts and friends #128

merged 86 commits into from
May 4, 2021

Conversation

matthewstern
Copy link
Contributor

@matthewstern matthewstern commented Apr 5, 2021

Changes

  • replaced sysfonts-based font lookup with systemfonts-based font "registration"
  • did away with Calibri on PCs as fallback - just left it as default sans-serif font on all platforms
  • updated vignettes to draw pngs with ragg devices
  • converted cmapplot_globals from an exported list to an environment
  • created three new functions: get_cmapplot_globals, get_cmapplot_global, and set_cmapplot_global to view and override entries within globals if needed. New documentation as needed.
  • moved the package startup message from .onLoad to .onAttach to avoid the note produced by devtools::check and conform to best practices.
  • shifted cmapplot_globals creation into new file cmapplot_globals.R
  • locally-built pkgdown site seems to work again!
  • adjusted pkgdown action build to gh-pages-test branch on any PR, and to gh-pages on push to master.

Note that finalize_plot saved outputs won't work as this PR does not encapsulate the work in progress by @sarahcmap, but it should look right in the plots window if you have updated RStudio and enabled an "agg" backend.

Testing requests:

  • check() is producing an error at "rebuilding vignette outputs" about ragg. I believe I modified the vignettes correctly as per here and the output does seem to be working for me. Will others explore to see if they run into the same issue? (This may not be our problem?)
  • test fonts and vignette construction on Mac
  • test fonts, vignette builds, and global getting and setting via installation from github (vs load_all) (devtools::install_github("CMAP-REPOS/cmapplot", "systemfonts", build_vignettes = TRUE))
  • test fonts on a machine where Whitney has been installed into a local user directory rather than C:/Windows/fonts/. I believe this will be the case if you installed Whitney without an admin password from IT.
    confirm here:
    image

To dos this PR produces:

  • I'm not sure the "new window" mode for finalize plot supports ragg. (As it is, this mode currently only works on a PC). We'll need to investigate whether there is a way to open a new window device with ragg, or remove the "new window" mode entirely from finalize_plot. @sarahcmap if this PR gets merged in would you be willing to explore this along with your other modifications to that function?
  • Check to see if the use of systemfonts prevents breakage in the documentation examples for theme_cmap() and finalize_plot(). If these now do not cause errors, rewrite the example sections of these documentation pages to actually run examples.

matthewstern and others added 13 commits April 2, 2021 22:35
`cmapplot_global` env initializes with "sans" (e.g. Arial) as default font. .onLoad uses systemfonts pkg to register Whitney if available and overwrite fonts as Whitney instead. the `display_cmap_fonts()` test function is updated to call on the new `cmapplot_global` env rather than the `cmapplot_globals` list.
gave up on new cmap_global nomenclature--too many references throughout package to update. Converted remaining items from the list to the environment.
basic, for testing. would need documentation if it works.
not sure why exporting doesn't seem to be working
@matthewstern matthewstern added this to the v1.2 milestone Apr 5, 2021
Copy link
Contributor

@sarahcmap sarahcmap left a comment

Choose a reason for hiding this comment

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

  • On cmap laptop:
    • Installed from GitHub using devtools.
    • Vignettes and locally built pkgdown site looked good from a quick click-through,
    • plot creation within RStudio with ragg backend looks good.
  • On personal mac:
    • Installed with GitHub using devtools.
    • Vignettes look good, did not try local pkgdown site
    • plotting in RStudio with ragg backend looks good (same as PC plot)
  • I'll look into how/if the 'window' mode would work with ragg as part of finalize_plot. I'll pull these changes into my v1.2finalize branch to make sure everything works together.

@matthewstern
Copy link
Contributor Author

  • On cmap laptop:
    • plot creation within RStudio with ragg backend looks good.

Thanks @sarahcmap, Mac confirmation with systemfonts is really exciting! Any chance the Whitney fonts on your cmap laptop are installed NOT in c:\windows\fonts\?

@sarahcmap
Copy link
Contributor

Thanks @sarahcmap, Mac confirmation with systemfonts is really exciting! Any chance the Whitney fonts on your cmap laptop are installed NOT in c:\windows\fonts\?

They are in the usual folder (C:/Windows/Fonts), so can't comment on that item unfortunately!

@nmpeterson
Copy link
Contributor

I've taken a look on my Windows laptop; still have to review on Mac. Moved a few chunks of code around.

It looks like, if you don't explicitly change the default graphics rendering in RStudio to AGG, plots will produce errors like the following, which I... don't love. Does everybody else get these errors, too?

Warning messages:
1: In grid.Call(C_textBounds, as.graphicsAnnot(x$label), x$x, x$y, :
font family not found in Windows font database
2: In grid.Call.graphics(C_text, as.graphicsAnnot(x$label), x$x, x$y, :
font family not found in Windows font database
3: In grid.Call.graphics(C_text, as.graphicsAnnot(x$label), x$x, x$y, :
font family not found in Windows font database
4: In grid.Call(C_textBounds, as.graphicsAnnot(x$label), x$x, x$y, :
font family not found in Windows font database
5: In grid.Call(C_textBounds, as.graphicsAnnot(x$label), x$x, x$y, :
font family not found in Windows font database

If we go forward with this, we'll have to update the readme and the installation vignette to make it very clear that AGG has to be used.

@nmpeterson
Copy link
Contributor

Also, I just ran pkgdown::build_site() and everything is in Arial.

matthewstern and others added 12 commits April 23, 2021 02:49
looks like options() isn't sticky across R sessions, which seems strange to me.
primary pkgdown workflow now deploys to gh-pages-test on any open PR update, and *should* deploy to gh-pages on push to master.
@matthewstern
Copy link
Contributor Author

This is ready for re-review!

This branch now...

  • registers the three needed fonts with all necessary specified variants (plain, bold, italic, bolditalic). The fonts are named overtly in .onLoad, and if any of the fonts are not found, use_whitney will not be set to TRUE. At least on my PC, it searches for systemwide fonts ("C/Windows/Fonts") and user-specific fonts.
  • checks for RStudio version and graphics backend setting, with necessary actions and warnings
  • updates the pkgdown Github action to dynamically deploy to gh-pages on a push to master, or gh-pages-test on any update to an open PR.

FWIW, the fonts I've chosen to stand in as the various needed variants. There is some discretion with what is used for bold, I picked what made the most sense to me:

      # register preferred strong font (Whitney Semibold), with variants
      systemfonts::register_font(
        name = cmapplot_globals$preferred_font$strong,
        plain = find_path("Whitney-Semibold-Adv", whitney_paths),
        bold = find_path("Whitney-Black-Adv", whitney_paths),
        italic = find_path("Whitney-SemiboldItal-Adv", whitney_paths),
        bolditalic = find_path("Whitney-BlackItal-Adv", whitney_paths)
      )

      # register preferred regular font (Whitney Medium), with variants
      systemfonts::register_font(
        name = cmapplot_globals$preferred_font$regular,
        plain = find_path("Whitney-Medium-Adv", whitney_paths),
        bold = find_path("Whitney-Bold-Adv", whitney_paths),
        italic = find_path("Whitney-MediumItal-Adv", whitney_paths),
        bolditalic = find_path("Whitney-BoldItal-Adv", whitney_paths)
      )

      # register preferred light font (Whitney Book), with variants
      systemfonts::register_font(
        name = cmapplot_globals$preferred_font$light,
        plain = find_path("Whitney-Book-Adv", whitney_paths),
        bold = find_path("Whitney-Semibold-Adv", whitney_paths),
        italic = find_path("Whitney-BookItal-Adv", whitney_paths),
        bolditalic = find_path("Whitney-SemiboldItal-Adv", whitney_paths)
      )

@matthewstern matthewstern requested a review from nmpeterson May 2, 2021 21:53
@nmpeterson
Copy link
Contributor

Works as advertised on my CMAP laptop

@dlcomeaux
Copy link
Contributor

dlcomeaux commented May 4, 2021

EDIT: I solved this - for some reason, I have the fonts installed twice, and that was causing the code to error. I added in an automatic adjustment to use just the first font in this case. @matthewstern let me know if you think this would cause any other issues in the code, I can't think of any but you're the one who's been deep in this.


I tried installing the latest version of this branch onto my CMAP laptop and was not able to get it to work. To confirm what I was supposed to do:

  • I have updated RStudio to the latest version
  • I changed graphics handling to AGG
  • I installed the systemfonts branch using devtools::install_github()
  • I loaded the cmapplot library.

When I do so, I got an error during installation, specifically relating to semibold:

*** arch - i386
Error : Font 'Whitney-Semibold-Adv' not found.
*** arch - x64
Error : Font 'Whitney-Semibold-Adv' not found.
** testing if installed package can be loaded from final location
*** arch - i386
Error : Font 'Whitney-Semibold-Adv' not found.
*** arch - x64
Error : Font 'Whitney-Semibold-Adv' not found.
** testing if installed package keeps a record of temporary installation path
* DONE (cmapplot)

This is a bit confusing to me, because when I run the systemfonts code in cmapplot.R, the system is able to find the path for semibold just fine. And, I don't think that semibold is installed any differently than the other two faces. Any ideas on what could be causing the issue? Perhaps there's an issue with the font, rather than with the R code (although the font seems to work just fine within Word)? Open to ideas, I'm at a loss.

If there are multiple copies of a font installed (like is the case on my machine for whatever reason...not sure if I did that by mistake).
@matthewstern
Copy link
Contributor Author

matthewstern commented May 4, 2021

Thanks for the thorough testing and tweak @dlcomeaux.

This shouldn't cause any new errors I can think of because the way the filename is calculated: result <- grep(paste0("(\\\\|/)", filename, ".[ot]tf$"), paths, value = TRUE) is quite conservative, as it goes from the last / or \ to the end of the string, so the only possible matches I can think of are multiple installations of the same file or both an otf and ttf version of the same font. In either of these situations, returning an arbitrary filepath seems like an acceptable solution.

BTW I believe the repeat errors you experienced on installation were because you had build_vignettes = TRUE. Most of our vignettes begin with library(cmapplot), which would then reproduce the error. It would have been interesting to confirm, while the error was occurring, that the vignettes had built, but had rendered in Arial.

@matthewstern
Copy link
Contributor Author

Speaking of which, @sarahcmap @nmpeterson @nmpeterson did any of you install .ttfs? I've only tested with .otfs and am curious to confirm that everything renders similarly with .ttfs.

@dlcomeaux
Copy link
Contributor

Thanks for the thorough testing and tweak @dlcomeaux.

This shouldn't cause any new errors I can think of because the way the filename is calculated: result <- grep(paste0("(\\\\|/)", filename, ".[ot]tf$"), paths, value = TRUE) is quite conservative, as it goes from the last / or \ to the end of the string, so the only possible matches I can think of are multiple installations of the same file or both an otf and ttf version of the same font. In either of these situations, returning an arbitrary filepath seems like an acceptable solution.

BTW I believe the repeat errors you experienced on installation were because you had build_vignettes = TRUE. Most of our vignettes begin with library(cmapplot), which would then reproduce the error. It would have been interesting to confirm, while the error was occurring, that the vignettes had built, but had rendered in Arial.

Is build_vignettes=Tthe default? I didn't set it explicitly. And to your other question, I have .OTFs

@matthewstern matthewstern merged commit bbdef8e into version1.2 May 4, 2021
@matthewstern matthewstern deleted the systemfonts branch May 4, 2021 14:42
@matthewstern matthewstern mentioned this pull request May 4, 2021
@nmpeterson
Copy link
Contributor

nmpeterson commented May 4, 2021

@matthewstern I have not tried TTFs. I.T. only gave me OTFs. I created the TTF files using a website that does the format conversion because at one point when I was originally working on the font specification I was considering using a package that only worked with TTFs. However, I have no idea how reliable that conversion process was -- the resulting TTFs could conceivably have issues due to their dubious origin, so probably best to stick with the OTFs. (Please feel free to test, though, either with the Whitney TTFs or another typeface for which you can obtain "official" TTFs.)

@matthewstern
Copy link
Contributor Author

Thanks Noel. @dlcomeaux just tested with the TTFs you shared with us some time ago and indicated they worked. I had forgotten that you had manufactured them, though! Good to know, and I will stick to OTFs going forward. TBH I'm just glad I'm finally figuring out how to speak in regex a bit.

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.

v1.2 font handling and output goals
4 participants