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

Bugfix Codicon Circles #1261

Merged
merged 3 commits into from
May 30, 2023
Merged

Bugfix Codicon Circles #1261

merged 3 commits into from
May 30, 2023

Conversation

Finii
Copy link
Collaborator

@Finii Finii commented May 26, 2023

Description

Revert scaling of (most of) the Codicons back to individual maximize as it has been with v2.3 and before.

On the way a new Scaling method is introduced and removed again as it does technically solve the problem but probably people do not want relative consistency but big icons i.e. maximizing.

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)?

Fixes: #1240

Screenshots (if appropriate or helpful)

@Finii
Copy link
Collaborator Author

Finii commented May 26, 2023

Here examples of the various tries:

Current situation

image

This looks reasonable for NFM but the left bearing of circle-filled looks strange for NF.
The reason is that the small and the large circle are in a ScaleGroup where shifting and scaling is done with the 'combined bounding box' in mind. So the centers of both circles are on top of each other in NFM and NF.

The NFP is even more strange, the center of the circles are not even the center of the advance widths?! The circle-large-outline is not part of the group and it is handled ok.

image

ScaleGroupsVert

A new scaling group type added, seems to look good:

image
Top to bottom: NFM, NF, NFP

And here a proportional font

image
Top to bottom: NF, NFP

Here it also seems to look good, albeit the quasi-cellsize is a bit wide.

Legacy situation

Means v2.3.3

image
Top to bottom: NFM, NF, NFP

Codicons: Maximise all circles

[why]
The scaling works okay for `Nerd Font Mono` fonts, but not for `Nerd Font`
fonts. The later have 'unexpected' left side bearing, which looks even
more out of place because the width is unrelated to the advance width
and might place the majority of the glyph into the next cell.

[how]
In fact we do not want scaling and shifting to be uniform for all glyphs
but the intend was to get the scaling from the biggest (full size)
circle and all the smaller ones would be proportionally smaller.

So we add a new Scale grouping that just takes the scale and the
vertical shift of the combined boundingbox, but individually calculates
the horizontal shift.

Signed-off-by: Fini Jastrow <ulf.fini.jastrow@desy.de>
Finii added a commit that referenced this pull request May 30, 2023
See PR #1261 for details on the reasons.

Signed-off-by: Fini Jastrow <ulf.fini.jastrow@desy.de>
Finii added a commit that referenced this pull request May 30, 2023
See PR #1261 for details on the reasons.

This mostly reverts commit
  f84a4a4  font-patcher: Add Codicons scale list

Signed-off-by: Fini Jastrow <ulf.fini.jastrow@desy.de>
@Finii Finii marked this pull request as ready for review May 30, 2023 19:58
See PR #1261 for details on the reasons.

This mostly reverts commit
  f84a4a4  font-patcher: Add Codicons scale list

Signed-off-by: Fini Jastrow <ulf.fini.jastrow@desy.de>
This reverts commit fcf5c84.

[why]
There is no current usecase for the ScaleGroupVert.
@Finii Finii merged commit 8ea2e71 into master May 30, 2023
@Finii Finii deleted the bugfix/codicons-circles branch May 30, 2023 20:03
@237dmitry 237dmitry mentioned this pull request Jun 6, 2023
LNKLEO pushed a commit to LNKLEO/Nerd that referenced this pull request Nov 24, 2023
See PR ryanoasis#1261 for details on the reasons.

This mostly reverts commit
  f84a4a4  font-patcher: Add Codicons scale list

Signed-off-by: Fini Jastrow <ulf.fini.jastrow@desy.de>
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.

\ue0b4 cuts half of the previous character
1 participant