Consider font style when generating familyname and fontname

[jrolfs]

These small modifications to the font patching script take the separate font styles into account when generating the font family and font name of the generated fonts. This ideally consolidates all of the fonts under a single font family each as a unique font style when the patched fonts are installed.

I'm not fully aware of the standards used with regards to the dashes in the different strings that are assumed in this pull request so additional normalization may need to be done in order to prevent these changes from causing problems on fonts that have non-standard or incorrect metadata. I'm happy to put research into this if this is a desired modification to the script.

[jrolfs]

[ryanoasis]

@jrolfs Hey thanks for this. Looks like it is working for you.

Just one thing some of these change might conflict with some refactor I was doing on https://gist.github.com/ryanoasis/af51f008838aaa46fb61 but looks like we both are trying to solve the same problem. I think one reason was the regex was bad... among other things.

Anyway.. whatever happens I will try to get your contribution in even if some of my changes conflict 👍

[jrolfs]

Yeah, I was definitely taking the easy route on this one (ie: avoiding the regular expression). If you're working on a larger refactor that addresses #56 and #58 then by all means just go ahead and take care of it there, it won't hurt my feelings if you just close this one in favor of that 😉

That being said, if you'd like me to look into the regular expression I'd be happy to. I'd really like to avoid this line.

[ryanoasis]

Yeah the regex took me a bit to figure out too, I think it is originally from the powerline font patcher or similar.

Okay yeah I will use what I have but also try to add your changes. 👌

Good to know it won't hurt your feelings either way. I just try to be very welcoming so almost every PR on the projects I've started so far has been included (I think only one actually was not included because I was in doubt on the legal circumstance) and being friendly I believe is important.

I have seen too much negativity and egotism in other projects. 😃 😉

[jrolfs]

I very much appreciate that sentiment. I won't point any fingers but I know exactly what you're talking about. Here's to being nice 🍻

[Qix-]

This doesn't seem to fix it. When using the patch-all script, I'm still seeing duplicates.

This is a deal breaker for me :/

[jrolfs]

@Qix- yep, it's a deal breaker for me too which is why I started looking into it. I'll admit I only used the script on Fira Code because that's the font I use. I'll take a look at Hack because it may not adhere to the "standard" I made an assumption on which expects a - in the un-patched font's original familyname property.

[jrolfs]

@ryanoasis let me know if this is something you've already addressed.

[jrolfs]

@Qix- are you sure you were on the right branch? I just quickly ran the font-patcher script by itself on Hack-Regular and it installed with the correct family and style name.

I'll give the gotta-patch-em-all script a run to see if there's an issue in how it invokes the patcher script.

[ryanoasis]

@Qix-
Seems like the fix did work for @jrolfs but yeah maybe something in the patch all script is causing an issue.

However I feel good about the chances that this will be fixed before the next release 😄

I am away from my laptop at the moment. A lot of eyes suddenly on this project 😊 but I can only help out in a limited fashion right now 😅

[Qix-]

@jrolfs I just used the gotta-patch-em-all script - I'll try the individual patcher.

[jrolfs]

This is what I ended up with after installing the complete variant after running gotta-patch-em-all-font-patcher! on this branch:

Let me know if you're using one of the subset patch variants or are still having this issue and I'd be happy to troubleshoot.

In the mean time, I zipped up my patched Hack output if you want to give it a shot: http://d.pr/f/4kQS

[Qix-]

Well now it's hanging on AurlentSansMono. :|

[jrolfs]

Ah, I've only ran it with the prefix option to target specific fonts:

./gotta-patch-em-all-font-patcher! Hack

Clearly there's more work to be done here :F

[Qix-]

For the record I'm not sure if it's hanging in master, or if it's just this PR. Let me double check.

[jrolfs]

There's a good chance it's just this PR as master doesn't utilize style at all from the regular expression output of the name so if there is no dash in the name it probably blows up. I need to add error handling and/or normalization.

[Qix-]

The thing is that it's not failing, it's just hanging. Not sure if this is normal but the compilation for Hack alone has taken about 10 minutes and counting at the moment, taking up a ton of CPU. Might be worth noting I'm on OS X.

---

Just ran ps aux | grep -i font - getting ton of processes. Looks like it's forking everything :o why?

$ ps aux | grep -i font | wc -l
64

One is fontd and another one is FontBook, which is open right now. The rest are related to compilation. This doesn't seem right.

---

And now, even though I ran ./gotta-patch-em-all-font-patcher\!.sh Hack, it's still patching all of them. Also, they still run even after I ^C, which is definitely not right.

[jrolfs]

Hrm, I'm on OS X as well. Hack generates way faster for me than Fira Code did. Although, when I was patching Fira Code I'm pretty sure the script got into an infinite loop somehow... eventually I ended up killing the process with ctrl-c but the patched output font files seemed complete and working.

Are you experiencing something like that or does the standard output stop?

For the record I'm on:

Dependency	Version
OS X	10.11.3
Python	2.7.8
FontForge	20150913

[Qix-]

Sounds pretty close to what's going on with me. When you Ctrl-C, are the processes still running?

OSX 10.11.2
Python 2.7.10
FontForge 20150913

[jrolfs]

Yeah, whatever fontforge processes that were spawned most recently finish running and then finish generating the particular variant of fonts and then exit cleanly after killing the bash script. I think this is a bug and probably merits a separate issue.

It's also worth noting that the patch all script uses the fontforge -script version of executing the individual patch script so it uses whatever version of Python that ends up bundled with FontForge (in my case I must have been running the patch all script on OS X 10.11.3's builtin Python 2.7.10). I tested 2.7.8 and 2.7.11 from homebrew using both the individual patcher script and a modified patch all script and Hack still works fine for me 😕.

I'll open an issue when I have time and try and see if I can reproduce the infinite loop with FiraCode again and hopefully debug a bit.

[Qix-]

@jrolfs any chance of uploading your fixed Hack fonts? I just can't get this to work D:

[jrolfs]

@Qix- it should be in this comment at the bottom: #61 (comment). Let me know if it works for you.

[Qix-]

Oh perfect, thank you.

[ryanoasis]

I plan to pull this (#61) into the 0.7.0 branch next. just out of steam at the moment 😅

[ryanoasis]

I did merge this into the 0.7.0 branch but basically only the contribution/commits will be there because my refactor was in conflict with it.

However I cannot test on osx so if someone wants to poke around in 0.7.0 branch to test that'd be cool and if it doesn't resolve the issue then perhaps I need to use at least part of @jrolfs changes to get it working 😄

I am trying to get everything into 0.7.0 but I have yet to rebuild all the fonts.. I would ideally like to make sure the #61 and #56 are actually fixed before rebuilding everything 😆

Taking a break now. Just FYI I will try to check the gitter chat more often these days

[poliquin]

I don't think 0.7.0 (081fe33) resolves the problem. I tried running font-patcher on the Input Mono font and while the styles are now grouped correctly in Font Book, OSX still thinks there are duplicates:

When resolving the duplicates automatically, everything but one variant gets turned off:

So Font Book thinks the variants all represent the same font.

[ryanoasis]

@poliquin Thanks for testing the latest in Font Book!

There is part of @jrolfs PR I overwrote due to merge conflict that might be needed to solve this completely

If you want to try to add this (approximately line 122 of the patcher):

fontname += " - " + subFamily

[jrolfs]

@ryanoasis I did update the regular expression in my branch to make that part unnecessary.

[ryanoasis]

@jrolfs Ah yep I see that you did update the regex, but you still have this line too:

fontname = fontname + additionalFontNameSuffix.replace(" ", "") + "-" + style

Which I was trying to put back in after I resolved a conflict after merging yours.

Note: I also update the regex in my refactoring and I think it should work but if not I will revisit and look more closely at yours as well 😄

... part unnecesary

I think you were thinking of this part: style = style.replace("-", "") instead?

[jrolfs]

Ah, yeah, I was confused about which line you were referencing (lazy response without checking the source). I simply moved the - out of the second group in the expression in my branch. I think you may have done more work on it in your branch?

Either way, that line should be necessary on the 7.0 branch for things to work properly @poliquin

[poliquin]

@ryanoasis Making that change at line 122 fixes the main issue, although Font Book shows a "'name' table usability" warning when importing the font:

This doesn't appear to cause major issues, and the font can be installed successfully. But when I install the patched font and try to use it I get oddly sized glyphs in some cases:

The above might not have anything to do with Font Book and this pull request, however.

[ryanoasis]

Ugh I thought I replied not sure what happened... sorry.

warning when importing the font

Was this occurring before the 'line 122' change as well or just afterwards?

oddly sized glyphs in some cases

This is something I am going to give most of my attention (related to this project) for the next release. There are multiple issues with glyph sizes and positions. I have since added labels on the related tickets but we are probably targeting fixing some or most of them in the v0.8.0 release.

[ryanoasis]

@jrolfs No problem. Yeah I also worked on the regex and in general font name tweaks and refactoring in my original refactor. I am going to add the 'line 122' change to 0.7.0 branch

[poliquin]

Was this occurring before the 'line 122' change as well or just afterwards?

The warning only appears after the line 122 change.

[ryanoasis]

@poliquin Okay I don't think those warnings are anything serious and I will continue on with this release with this as a known issue. However this could possibly be fixed later if it seems to be causing actual problems.

As you pointed out the glyph sizing would be a higher priority issue to fix anyway.

As for #61 I am calling 'good enough'. I cannot really test on os x and those who have the fonts seem to be working better (not perfect) in Font Book now.

[jrolfs]

@ryanoasis yeah I think this is indeed "good enough". If you've incorporated line 122 into your 7.0 branch then this can probably be closed unless you want to merge it for the time being.

[ryanoasis]

@jrolfs Yep I will get the line 122 change in 👍 to the 0.7.0 branch.

The only items left before merging that to master for release are: #52 and just rebuilding all the fonts

[ryanoasis]

Forgot I actually did make the change in the 0.7.0 branch: 52e9a95

I am just working on #52 and rebuilding the fonts 😄 ... maybe this weekend 🍀

[Qix-]

Now if only iTerm2 would hurry up and support ligatures I could have a kick-ass font with all these icons in them. 💃

[jrolfs]

@Qix- fo the record I did send him some Scotch so let's all just hope real hard on the iTerm thing :F

[Qix-]

@jrolfs oh man that was you? I saw someone talking about that on there, that was awesome.