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

Update JetBrains Mono to version v2.251 #776

Merged

Conversation

moritzdietz
Copy link
Collaborator

@moritzdietz moritzdietz commented Feb 2, 2022

Description

This PR updates JetBrains Mono to version v2.251. This is a follow up PR for #648, #572, #540 and #518.

Requirements / Checklist

What does this Pull Request (PR) do?

How should this be manually tested?

Any background context you can provide?

What are the relevant tickets (if any)?

Screenshots (if appropriate or helpful)

Signed-off-by: moritzdietz <moritzdietz@users.noreply.github.com>
@moritzdietz
Copy link
Collaborator Author

Actually something's not right. Making this a draft.

@Finii
Copy link
Collaborator

Finii commented Feb 2, 2022

something's not right

Mind to tell what? ;-)

@moritzdietz
Copy link
Collaborator Author

I just looked at the diff and thought that it's a bit odd that there aren't as many additions as my other PRs have.
I just checked to see if the patching didn't work, but it did. The Glyphs are all in there.

Additionally I saw your other PRs fixing various font-patcher things. Maybe you'd know if there have been changes lately which result in smaller diffs.

(also there are the usual issues around glyphs widths but this wasn't why I marked this as draft to check)

@moritzdietz moritzdietz marked this pull request as ready for review February 2, 2022 20:26
@Finii
Copy link
Collaborator

Finii commented Feb 3, 2022

I just looked at the diff and thought that it's a bit odd that there aren't as many additions as my other PRs have.

I do not really get what you mean. From what I see the diffs look ok?
Do you mean the number of additions/deletions in the text files?
Or the patch-size?

-rw-rw-r-- 1 fini fini   2730005 Feb  3 09:46 0001-Add-new-unpatched-version-of-JetBrainsMono-in-versio.patch
-rw-rw-r-- 1 fini fini   2925873 Feb  3 09:45 0001-Add-updated-JetBrainsMono-fonts-in-version-v2.242.patch
-rw-rw-r-- 1 fini fini   2078752 Feb  3 09:38 0001-JetBrains-Mono-v2.251-Add-unpatched-fonts.patch

It's a bit smaller but I guess the changes are more localized (just adding some glyphs) so that binary diff could produce smaller files?

Additionally I saw your other PRs fixing various font-patcher things. Maybe you'd know if there have been changes lately which result in smaller diffs.

Not really. Maybe a newer git version that has a better binary diff?

With this version, v2.251 ... official is still v2.242 (https://github.com/JetBrains/JetBrainsMono/releases)
You are confident that there are no pending changes or something ;)

Edit: Separate answer from quote

@moritzdietz
Copy link
Collaborator Author

moritzdietz commented Feb 3, 2022

It was the diff-size that threw me off. Nothing more. Don't read too much into it. Really :)

You are confident that there are no pending changes or something ;)

There have been changes which were quiet important / issues were annoying that I waited for ever since the new version appeared to make a new PR. This was done especially for the reason you mention, that in a case there is another change we don't drift.
I feel that I have now waited enough so that I will say that in the case where they choose to do another release, I just make a new PR. There are more benefits of having the newer version than not.

@Finii
Copy link
Collaborator

Finii commented Feb 3, 2022

/spent 40m for detailed review

I reckon Ryan would want to merge himself, I found no issues with the fonts themselves.

Edit: Somehow github is missing gitlab's approve button 😬

@Finii
Copy link
Collaborator

Finii commented Feb 3, 2022

When I redo the patching I get slightly different patched font files (size differs by tens of bytes).

fontforge does not find any real difference, just some very zigzappy icons seem to be encoded slightly differently, which can be because of a different fontforge version.

image
Example diff for NoLigature Regular

@ryanoasis
Copy link
Owner

regarding the conflicts:

#775 (comment)

@moritzdietz
Copy link
Collaborator Author

Ok, should I just drop my second commit then? Then this PR only adds the unpatched font and the CI builds the patched one.

@Finii
Copy link
Collaborator

Finii commented Feb 9, 2022

I'd say: Yes. Luckily you structured the PR so well, with 2 commits 👍
like

git reset --hard HEAD^
git push --force

Or something ;)

@moritzdietz
Copy link
Collaborator Author

moritzdietz commented Feb 9, 2022

Oh, I know and how to do it ;-) I did that on purpose.

@moritzdietz moritzdietz force-pushed the moritzdietz/JetBrainsMono-2.251 branch from 91e82bf to dd52740 Compare February 9, 2022 20:55
@moritzdietz
Copy link
Collaborator Author

moritzdietz commented Feb 23, 2022

Is this good to go now? Asking before I hit merge.

@Finii
Copy link
Collaborator

Finii commented Feb 24, 2022

image

This button is missing in Github, so I copied it over from Gitlab

@moritzdietz moritzdietz merged commit 58898d3 into ryanoasis:master Feb 24, 2022
@moritzdietz moritzdietz deleted the moritzdietz/JetBrainsMono-2.251 branch February 24, 2022 08:00
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants