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

Switch to HatScripts/circle-flags #17

Open
jimjam-slam opened this issue Mar 4, 2022 · 17 comments
Open

Switch to HatScripts/circle-flags #17

jimjam-slam opened this issue Mar 4, 2022 · 17 comments

Comments

@jimjam-slam
Copy link
Owner

jimjam-slam commented Mar 4, 2022

The EmojiOne flag dataset has been frozen for a while now, and we've had some issues with inaccurate flags (eg. PR #16 fixes #14 and #7). https://github.com/HatScripts/circle-flags looks like it's had more recent updates and is MIT licensed.

@jimjam-slam jimjam-slam added this to the version 1.0 milestone Mar 4, 2022
@jimjam-slam
Copy link
Owner Author

jimjam-slam commented Mar 4, 2022

Note I should do a comparison to catch any changes first! In particular:

  • Qatar
  • Latvia
  • Saint Martin

@jimjam-slam
Copy link
Owner Author

jimjam-slam commented Apr 10, 2022

Related: the {ggsvg} package, by @coolbutuseless, might be an alternate way to get the flags in that avoids some of the issues we've had with {grImport2}. Might be worth investigating rebuilding around these for a 1.0 release!

@rempsyc
Copy link

rempsyc commented Sep 16, 2023

What would be the steps involved for switching to the HatScripts/circle-flags images?

@jimjam-slam
Copy link
Owner Author

jimjam-slam commented Sep 18, 2023

The basic flow is:

  1. Create a directory in inst/, called svg
  2. Download the SVGs for the flags into inst/svg/, naming them according to their 2-letter ISO code in lowercase (eg. inst/svg/au.svg for the Australian flag)
  3. Run store_flags.r from inst/. This updates data/lflags.rda.
  4. (Optional) Running modify_flags.r opens data/lflags.rda, patches a couple of flags, and saves it back.
  5. Build the package with the new data/lflags.rda.

If there are problems with step 3 (especially if we switch data sources to HatScripts/circle-flags), it may be that the structure of some new SVGs doesn't fit well with grConvert or grImport2.

It may also be that I need to run it if grConvert can't be installed on Windows! This is about the point at which I'd be starting to look at a different SVG pipeline more broadly.

@rempsyc
Copy link

rempsyc commented Sep 18, 2023

Is there a reason we don’t keep the flags on the repo (while adding them to .rbuildignore to avoid installing them with the package) for reproducibility? Instead of having to download and rename them manually? Is it that the files are too big?

@jimjam-slam
Copy link
Owner Author

It honestly just hadn't occured to me 😅 Ggflags was the first package I took maintenance on, and it definitely hasn't been put together as I would put a package together now with reproducibility (or even CRAN submission) in mind. I'd be happy for the flag SVGs to be bundled in separately!

I'm unfortunately having trouble running {grConvert} on my Mac too—something's going wrong with poppler. I'll have another try tonight, but the alternative solution might be to run it on a Linux cloud machine like Cloudspaces or GitHub Actions.

I've added the package to my R-Universe, but if we can't run {grConvert} on Windows or Mac, we can't add the flag bundling process itself to the package build process (which I think would be ideal). But if we could at least still have the flag SVGs in the inst folder alongside the bundling script for reproducibility, and automatically renaming them from their codepoints to the ISO codes, I think that's a great compromise.

(I'm actually not sure if the flags are too big! Definitely worth checking)

@jimjam-slam
Copy link
Owner Author

jimjam-slam commented Sep 18, 2023

The specific problem I'm having installing grConvert is with the poppler dependency, which is used for PDFs rather than SVGs. librsvg is used for SVGs in grConvert::convertPicture. I wonder if we can just use librsvg directly...

@jimjam-slam
Copy link
Owner Author

In {grConvert}, the R code connects to a C function, svg_to_svg. We could strip down that code in {grConvert} and incorporate it in, although we'd need Simon to add a licence to the package that allowed that.

There's also the {rsvg} package, which also uses librsvg. It has a rsvg_svg function, although I don't 100% know if that would be a suitable replacement (I think we'd just have to try it and see). If it could be a drop-in replacement, that'd be a lot easier, as rsvg is still actively maintained!

@jimjam-slam
Copy link
Owner Author

Keen to have a crack at using {rsvg} as the replacement on Sunday (weekend time is a bit compressed at the moment unfortunately)!

@rempsyc
Copy link

rempsyc commented Sep 28, 2023

If possible, I would rather have my PR merged on main first, so that my own R CMD check stops failing 😬

@rempsyc
Copy link

rempsyc commented Sep 28, 2023

Right now all your changes from the last two years have been on the dev branch, and the main branch has not been updated for two years. But when people install ggflags, it installs from the main branch, not dev, so people have not been getting all the latest changes, it seems.

@jimjam-slam
Copy link
Owner Author

Okay, I've had a crack at this issue in #28, but it's not working because I'm having problems with {rsvg} (described in that PR).

But in the mean time, I've merged dev into main, so v0.0.3 is essentially the R CMD CHECK fixes and your authorship :)

@jimjam-slam
Copy link
Owner Author

Okay, the new HatScripts/circle-flags set is working—it needed some pre-processing because those SVGs use <mask>s for the circular frames rather than <clipPaths> (which librsvg didn't like), but otherwise they seem to render fine!

One thing I have held off on is introducing the non-country flags included with that set. I would like to, but there's a bit of a larger question about how we communicate licensing to people, as although circle-flags is MIT-licensed, the flags themselves have their own copyright. Obviously this is a larger problem even with the existing flags, but I'd like to do some thinking before I add them all in.

@rempsyc
Copy link

rempsyc commented Sep 30, 2023

Oh, cool, great job! I understand for the non-country flags. Should we mention the licensing thing in the readme as well as in the data set documentation? It's possible to have different licenses for different components.

@jimjam-slam
Copy link
Owner Author

Yeah, I think it makes sense to say that circle-flags and the package code are both MIT-licensed, but the flags themselves may have their own copyright or usage requirements. I'm just sent out a couple of emails on this that I might wait on first, but that's essentially the approach I'm planning!

Unfortunately, although the SVGs converted using {rsvg} (with my workaround substituting the <mask> for a <clipPath>) render fine in browsers, they render without the clipping at all in ggplot2 :(

image

@jimjam-slam
Copy link
Owner Author

ropensci/rsvg#40 is fixed, but I'm still having rendering problems as above—but going by ropensci/rsvg#41 it looks like Paul is also making some updates to grImport2 (it looks like we're all getting hit by some upstream updates from librsvg at once). Will try to test the grImport2 update when I can!

@jimjam-slam
Copy link
Owner Author

Taking another swing at this with an updated grImport2, it looks like the circular clipping paths remain. However, I'm currently using grImport2 both for build-time processing and for rendering. If I bypass it entirely, I can use {ggsvg} to render hatScripts/circle-flags without any processing at all, and they appear to look correct:

svg_ae2 <- paste(readLines("svg/ae.svg"), collapse = "\n")
# Warning message:
# In readLines("svg/ae.svg") : incomplete final line found on 'svg/ae.svg'
grid::grid.draw(svg_to_rasterGrob(svg_ae2))
image

This means I could potentially re-write the package to just use ggsvg directly and skip {grImport2} (which I think appears to be where the problem is, rather than {rsvg}).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants