Merging Awesome Font Fix by bhilburn · Pull Request #472 · Powerlevel9k/powerlevel9k

bhilburn · 2017-04-06T23:45:46Z

This is the PR effort that merges @V1rgul's #385, which we need as soon as possible.

It is separate from #385 because I added a new icon set for AdobeSourceCodePro, for which we were previously telling users to use awesome-fontconfig since the codepoints lined up reasonably well. That was a mistake on my part, and now we are going to break this for those users.

More on this in the review that I'm about to leave. I would like to resolve my review comments before completing this merge.

@V1rgul - Your testing of this branch will be critical. It would also be great to get @protist testing this.

…V1rgul-master

bhilburn

Would like to address these comments before merge.

bhilburn · 2017-04-06T23:47:06Z

functions/icons.zsh

    # fontconfig with awesome-font required! See
    # https://github.com/gabrielelana/awesome-terminal-fonts
+    # if not defined, set recommended linux path
+    typeset -p "POWERLEVEL9K_FONTAWESOME_PATH" > /dev/null 2>&1  || POWERLEVEL9K_FONTAWESOME_PATH=~/.fonts


We have a lot of OSX users, and I'm concerned this will break things for them. However, looking at the "Awesome Font" installation instructions for OSX, it doesn't talk about installing these scripts, anywhere.

We either need some OSX users to weigh-in, here, or even better if we could get @gabrielelana's thoughts on how to handle this for OSX, that would be even better.

Yep, unfortunately AFAIK on OSX doesn't work like that, and it's even more difficult with Sierra. Anyway, the mapping part is good even for OSX, the environment variable above probably doesn't work but it's harmless, I don't think it will "break" anything

@bhilburn what do you think it will break?

@gabrielelana - Thanks for the response! The piece I don't understand for OSX is where is the *.sh script that we need to execute to get the mapping variables? I'm assuming that is a requirement on OSX, as well?

@bhilburn Yes, you need them on OSX as well, I guess we can keep them in the same place as on Linux (aka ~/.fonts directory), I will integrate my documentation to tell OSX users to always copy the mapping files

@gabrielelana - Okay, awesome. Can you let us know once the Awesome docs are updated? If I merge this before that's done, we are both likely to get hammered with support issues :)

@gabrielelana - Just checking in on this :) We can't proceed on our end until OSX has the script in the user directory, otherwise we'll break everything for a large number of users.

Btw. the docs are already updated: gabrielelana/awesome-terminal-fonts@1397337
Thx. @gabrielelana 👍

bhilburn · 2017-04-06T23:51:43Z

functions/icons.zsh

+      SSH_ICON                       '(ssh)'
+    )
+  ;;
+  'AdobeSourceCodePro')


So, creating icon sets is cheap for us. Since only the one that the user actually specified actually gets loaded, we can specify as many as we want without burdening the user. I should have just created this font set ages ago instead of telling SourceCodePro users to use awesome-fontconfig, because now this is going to break.

Which takes me to my question: in awesome-fontconfig, is there a way we can have ZSH fall-through to the next option, AdobeSourceCodePro, if the Awesome Font scripts aren't found? This would be fairly easy if there was something like a continue statement, but I actually can't find any documentation of that for ZSH. Thoughts, @dritter?

We could print an error if the scripts aren't found, with a hint to this page or the troubleshooting page, but I don't think we should silently fallback on AdobeSourceCodePro, that would make it uneasy for us in the future

Hmm. I think the easiest way is to rename the icon sets.. I would leave the old one awesome-fontconfig and rename the new one to something like awesome-fontmap or awesome-mapped-fontconfig.
That way we can leave the old way like it is and recommend every user that has trouble with updated fonts to use the new one with the maps.
Btw. IMHO we should favour the new mapped way over the old one and reflect that in the documentation.

V1rgul · 2017-04-07T08:26:31Z

Ok, every icon is working for me :)

protist · 2017-04-07T10:52:36Z

Thanks for the fix, but unfortunately I'm away from my computer on holidays right now. I'm definitely happy to help test, but it might be ~2 weeks from now.

protist · 2017-04-18T05:02:14Z

I'm back from holidays… 🙂

This seems to work well for me! Thank you! FWIW on Arch Linux, the default package-manager–installed location for the awesome-terminal-fonts *.sh scripts is as follows.

POWERLEVEL9K_FONTAWESOME_PATH='/usr/share/fonts/awesome-terminal-fonts/'

I'm not sure if that would be universal enough (even in Linux) to set as a default, though. Thanks for the PR!

bhilburn · 2017-04-18T21:27:18Z

Thanks for testing, @protist!

Okay, it seems like the code-points themselves are ready to go (makes sense since they are just @V1rgul's commits), but we have an issue with regard to installation path of the the font-mapping script.

If we can't find the script, and thus execute it, we will break things for a lot of users. There is the obvious fix of users simply finding the script themselves and symlinking it into a location where we are looking, but that is:

a) A painful user experience.
b) Will generate a high support load for us.
c) Opens the door to lots of accidental breakages (e.g., a user copies the script instead of symlinking it, @gabrielelana updates the code-points, P9k breaks).

It's clear that using the script that @gabrielelana packages is the correct way to do this, but we need to be sure that we aren't making things more difficult for our users. Unfortunately, I don't know how to do this without just making a giant list of directories to check (it's not like this is going to be in pkgconfig or in the env).

Any other ideas, here?

protist · 2017-04-19T04:03:03Z

No worries @bhilburn! It's probably not useful, but it looks like the awesome-terminal-font package from Arch Linux's AUR just puts the *.sh scripts in the same directory as the *.ttf fonts themselves. However, I'm not sure if this is (a) consistent in other systems, or (b) a path that can be extracted programatically anyway.

gabrielelana · 2017-05-21T15:38:19Z

@bhilburn sorry to be so late, I have updated the README.md of awesome-terminal-fonts, take a look and let me know if this will suffice or if you need something else

dritter

Strange. I thought I reviewed this PR a while ago.. Seems like I just left comments.

Anyway. My recommendation is to add this icon set under a new name, so that we can leave the old awesome-fontconfig as it is and users of that won't have a bad experience.
The docs should be changed to recommend the new icon set with font maps.

What do you think @bhilburn ?

bhilburn · 2017-08-27T16:42:39Z

This work was picked up by @pfrybar and continued in #599

V1rgul and others added 8 commits January 24, 2017 13:39

update fontconfig icons, using awesome-terminal-fonts maps

eee2871

fontconfig better network icon

f16a943

fontconfig lock icon

ed5e8fd

merge: fix conflicts

be7b34b

configurable fontawesome path for awesome-fontconfig

056614a

merge: fix conflicts

4a6479c

Merge branch 'master' of https://github.com/V1rgul/powerlevel9k into …

c86c82f

…V1rgul-master

Adding "AdobeSourceCodePro" option, deleting whitespace

286ae7c

bhilburn added enhancement Fonts labels Apr 6, 2017

bhilburn self-assigned this Apr 6, 2017

bhilburn requested a review from dritter April 6, 2017 23:45

bhilburn commented Apr 6, 2017

View reviewed changes

onaforeignshore mentioned this pull request Apr 7, 2017

Merge engines #474

Closed

20 tasks

onaforeignshore mentioned this pull request Apr 8, 2017

Merge engines #476

Closed

23 tasks

onaforeignshore added a commit to onaforeignshore/powerlevel9k that referenced this pull request Apr 12, 2017

merged Powerlevel9k#466 Powerlevel9k#472 Powerlevel9k#480

dc37e50

bhilburn mentioned this pull request Apr 19, 2017

update fontconfig icons, using awesome-terminal-fonts maps #385

Closed

V1rgul mentioned this pull request May 4, 2017

Custom icons in prompt broken #506

Closed

V1rgul mentioned this pull request Jun 15, 2017

Question about zsh_highlight_styles #550

Closed

dritter reviewed Jul 18, 2017

View reviewed changes

bhilburn mentioned this pull request Jul 27, 2017

Fix reference for Python icon. #577

Merged

pfrybar mentioned this pull request Aug 12, 2017

similar to PR #472, fixing for awesome font mappings #599

Merged

bhilburn closed this Aug 27, 2017

Conversation

bhilburn commented Apr 6, 2017

Uh oh!

bhilburn left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gabrielelana Apr 7, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

V1rgul Apr 7, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

V1rgul commented Apr 7, 2017

Uh oh!

protist commented Apr 7, 2017

Uh oh!

protist commented Apr 18, 2017

Uh oh!

bhilburn commented Apr 18, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

protist commented Apr 19, 2017

Uh oh!

gabrielelana commented May 21, 2017

Uh oh!

dritter left a comment

Choose a reason for hiding this comment

Uh oh!

bhilburn commented Aug 27, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

gabrielelana Apr 7, 2017 •

edited

Loading

V1rgul Apr 7, 2017 •

edited

Loading

bhilburn commented Apr 18, 2017 •

edited

Loading