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

Add Neovim icon (fixes #1383) #1391

Merged
merged 7 commits into from
Oct 23, 2023

Conversation

georgeguimaraes
Copy link
Contributor

@georgeguimaraes georgeguimaraes commented Oct 20, 2023

Description

I decided to take a shot into implementing #1383. Not sure if this is it, or if I should run bin/scripts/gotta-patch-em-all-font-patcher!.sh and commit all the changes. Let me know if this is ok.

Requirements / Checklist

What does this Pull Request (PR) do?

Adds the neovim icon as discussed in #1383

How should this be manually tested?

The generated original-source.otf now has the Neovim icon:

CleanShot 2023-10-20 at 15 04 30

Any background context you can provide?

What are the relevant tickets (if any)?

#1383

Screenshots (if appropriate or helpful)

@georgeguimaraes georgeguimaraes changed the title Add Neovim icon (solves #1383) Add Neovim icon (fixes #1383) Oct 20, 2023
@Finii
Copy link
Collaborator

Finii commented Oct 21, 2023

Thanks for the PR!

To add a icon one just needs to throw the svg into the correct directory and add a line to icons.tsv. The workflow than automagically updates the i_seti.sh and the original-source.otf (workflow PackSVGs). Maybe we should add that to the comments in icons.tsv?

Manually one needs to

  • expand the patching range in font-patcher
  • correct the wiki page

In the original issue you mentioned font-logos. There is a PR pending to add the neovim icon there. You correctly pointed out that the repo seems dead.
But in fact, after #1391 has been raised I investigated and it seems we can revive the repo (see Issue 95 over there).
That would of course be preferred.

So maybe we should wait a bit and see if we can get that going again. The messages seemed rather promising.

@Finii
Copy link
Collaborator

Finii commented Oct 21, 2023

I believe the font-logos neovim icon has a solid lift-side-bar thing, while here it is hollow (to reflect that it is blue instead of green, a guess).
This is just an observation.

Screenshot 2023-10-21 at 12 44 55

Taken from font-logos fork release

@Finii
Copy link
Collaborator

Finii commented Oct 21, 2023

The original black-and-white logo published by neovim themselves is like THIS logo, i.e. with a hollow left leg:

https://github.com/neovim/neovim.github.io/blob/master/logos/neovim-logo-1color.svg

@georgeguimaraes
Copy link
Contributor Author

In the original issue you mentioned font-logos. There is a PR pending to add the neovim icon there. You correctly pointed out that the repo seems dead.
But in fact, after #1391 has been raised I investigated and it seems we can revive the repo (see lukas-w/font-logos#95).
That would of course be preferred.

That's interesting! Let's wait then if font-logos comes back to activity. It seems that the maintainer has not given access to other people.

However, I think there may be an underlying question here: Should Nerd Fonts rely on upstream repos that (as you mentioned in that issue) appear to have no standalone use? As oppose to, I'd say, Material Design. Of course, that's just me thinking out loud, it's a questions for nerd fonts' maintainers to discuss. :)

Just hoping to get Neovim icon soon :)

@georgeguimaraes
Copy link
Contributor Author

The original black-and-white logo published by neovim themselves is like THIS logo, i.e. with a hollow left leg:

https://github.com/neovim/neovim.github.io/blob/master/logos/neovim-logo-1color.svg

Yes, that's the one I used (hollow left leg).

@georgeguimaraes
Copy link
Contributor Author

To add a icon one just needs to throw the svg into the correct directory and add a line to icons.tsv. The workflow than automagically updates the i_seti.sh and the original-source.otf (workflow PackSVGs). Maybe we should add that to the comments in icons.tsv?

I guess this paragraph in the contributing file, in a entry regarding new icons, should be enough.

@georgeguimaraes
Copy link
Contributor Author

btw @Finii, tks for the interactions here! I really appreciate the care you have with nerd fonts and all the work

@georgeguimaraes
Copy link
Contributor Author

Manually one needs to

  • expand the patching range in font-patcher

You mean this line

{'Enabled': True, 'Name': "Seti-UI + Custom", 'Filename': "original-source.otf", 'Exact': False, 'SymStart': 0xE4FA, 'SymEnd': 0xE5FF, 'SrcStart': 0xE5FA, 'ScaleRules': None, 'Attributes': SYM_ATTR_DEFAULT},
?

It seems that U+E5AE is already included in that range. Is that so?

@Finii
Copy link
Collaborator

Finii commented Oct 22, 2023

It seems that U+E5AE is already included in that range. Is that so?

Oh, I anticipated that already in 869d6f9, so there is nothing to do in font-patcher :-)

Screenshot 2023-10-22 at 23 51 08

[why]
To avoid filename clashes between Seti and Custom we have all Custom
files with a _nf suffix. This has not been documented.

Signed-off-by: Fini Jastrow <[email protected]>
[why]
After a new icon has been added to the core set we want to make that
visible via a new version number.

Signed-off-by: Fini Jastrow <[email protected]>
[why]
We want the workflow to do this so that we have control over the
fontforge version that is used.

Signed-off-by: Fini Jastrow <[email protected]>
[why]
If the svg file is renamed the icon list has to be updated :rolleyes:

Signed-off-by: Fini Jastrow <[email protected]>
@Finii Finii merged commit 39dd0cb into ryanoasis:master Oct 23, 2023
0 of 5 checks passed
@Finii
Copy link
Collaborator

Finii commented Oct 23, 2023

Thanks again for the PR.

I hope the changes of the documentation is readable and helps a bit.

Here you can see the workflow in action that crates the new original font:
https://github.com/ryanoasis/nerd-fonts/actions/runs/6612722696/job/17959104734

And here the automatic commits added after your PR:

image

So the changes are now in the zip and the docker patcher.
All the prepatched fonts will only follow on the next release - to have it already now you need to self patch (or tell me which font(s) you need, I can prepare them)

@georgeguimaraes
Copy link
Contributor Author

Nice changes to contributing.md. Much clearer to understand that all the heavy lifting of patching the fonts and committing them to the repo is done by the workflows.

I'll try to patch Victor Mono by myself. If I don't succeed I'll ask for help. Thanks so much!

@Finii
Copy link
Collaborator

Finii commented Oct 23, 2023

@allcontributors please add @georgeguimaraes for code

@allcontributors
Copy link
Contributor

@Finii

I've put up a pull request to add @georgeguimaraes! 🎉

@Finii
Copy link
Collaborator

Finii commented Oct 23, 2023

Nearly forgot...

I always struggle with the type; code seems not fitting, but all other are also not really good. I believe we used code before for this kind of contribution.

@berlinx2104
Copy link

Screenshot from 2023-10-27 20-57-47
Can you guys reconsider that the left part should be overpainted so it's actually easier to see at the terminal? That edge is really small to see at the terminal. Anyway, thank you so much for taking the time to make it and I respect all your decisions.

@Finii
Copy link
Collaborator

Finii commented Oct 27, 2023

@berlinx2104 Thanks for the feedback!

I will try to improve the visibility.

The completely filled symbol is already in Font Logos (see links above) which I expect will pull the PR that contains it soon and afterwards we update that here, so both variants will end up in the patched fonts.
(But unfortunately Lukas did not make one of us Maintainer, yet)

@georgeguimaraes
Copy link
Contributor Author

CleanShot 2023-10-27 at 13 51 07

I kinda got used to see it as a modified "V", but yes, the left leg is too subtle. Maybe it was a design decision by the Neovim team?

@Finii
Copy link
Collaborator

Finii commented Oct 27, 2023

Do you think this is better, or not enough?

image

@Finii
Copy link
Collaborator

Finii commented Oct 27, 2023

Corrected the aspect ratio:

image

@Finii
Copy link
Collaborator

Finii commented Oct 27, 2023

Maybe it was a design decision by the Neovim team?

Well, it was designed to be displayed at a much bigger size. That is why icons come in different sizes, because rescaling usually is not good enough. Here I increased the gap and increased the 'stroke width' of the left leg. In that process I changed the angle of the middle-bar. If I do not change that the ascpect ratio changes, see image above.

@georgeguimaraes georgeguimaraes deleted the gg-add-neovim-icon-to-custom branch October 27, 2023 17:14
@georgeguimaraes
Copy link
Contributor Author

That looks way better (the 2nd version)

@Finii
Copy link
Collaborator

Finii commented Oct 27, 2023

Ok, I will push that, so that you (both) can try it out in real...

@Finii
Copy link
Collaborator

Finii commented Oct 27, 2023

Changes pushed via 27f6ea1

image

ZIP and docker ready.

@georgeguimaraes
Copy link
Contributor Author

CleanShot 2023-10-27 at 15 16 10

It is certainly more visible. I can clearly see the hollow leg on my terminal now. Thanks, @Finii

@berlinx2104
Copy link

Thanks, @Finii
Screenshot from 2023-10-29 00-29-42

perrin4869 added a commit to perrin4869/dotfiles that referenced this pull request Nov 21, 2023
LNKLEO pushed a commit to LNKLEO/Nerd that referenced this pull request Nov 24, 2023
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.

3 participants