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

Atlas Engine: Way to get back spaces between lines? #14068

Closed
leimath opened this issue Sep 24, 2022 · 3 comments · Fixed by #14255
Closed

Atlas Engine: Way to get back spaces between lines? #14068

leimath opened this issue Sep 24, 2022 · 3 comments · Fixed by #14255
Assignees
Labels
Area-AtlasEngine In-PR This issue has a related PR Issue-Bug It either shouldn't be doing this or needs an investigation. Needs-Tag-Fix Doesn't match tag requirements Priority-2 A description (P2) Product-Terminal The new Windows Terminal.

Comments

@leimath
Copy link

leimath commented Sep 24, 2022

Windows Terminal version

1.16.220921001-preview

Windows build number

10.0.19044.0

Other Software

helix-editor

Steps to reproduce

Font: Overpass Mono

Expected Behavior

without atlas terminal

2022-09-24-000125

This is without Atlas Engine in the current terminal preview. In the helix editor there are some space around the 1.

Actual Behavior

with atlas engine

2022-09-24-000126

This is with Atlas Engine, latest terminal preview.

A small query -
Is there a way to get back the space above the 1? I understand that it is intentional that line-gaps are intentionally ignored as per the release notes. Or is there any advantage over not having the space around the one?

@leimath leimath added Issue-Bug It either shouldn't be doing this or needs an investigation. Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting labels Sep 24, 2022
@leimath
Copy link
Author

leimath commented Sep 24, 2022

Please close the issue if I misunderstood this feature.

@lhecker
Copy link
Member

lhecker commented Sep 24, 2022

It's somewhat ridiculous how annoying it can be to deal with fonts... 😅
So some fonts incorrectly specify a line gap, even though they don't want one, and some fonts do specify one without which there's no proper line spacing. Additionally I assumed that terminals technically don't have any gaps between character cells anyways (#14014). So what should we do now?

I'll try to download your font and try to find a solution that works for both, "Terminus TTF" and "Overpass Mono" simultaneously, but I suspect either of the two fonts will need to be broken, depending on whether we want to support line gaps or not.

@lhecker lhecker self-assigned this Sep 25, 2022
@ninlil
Copy link

ninlil commented Sep 26, 2022

The same occurs with "Comic Code" ($0 "demo-purchase" available from https://www.myfonts.com/products/demo-regular-comic-code-474333 - actually to the point of cutting of the top 1 or 2 rows of pixels on high characters (like "f" and "8")

Yes, I understand that "technically" characters fill an entire "cell", but IMO:

  • the release-version works just fine (i.e as the font says)
  • there is no need to ignore it due to "visual considerations" like empty space between blocks (the user chooses the font)
  • if you really want it the add an option to enable/disable the extra line-height
  • or just add a custom "line-height" setting to allow users to have extra spacing between lines (some fonts are really tight, and some people have hard time reading -> "Accessability-feature")

So, please don't cut this extra line-height

@carlos-zamora carlos-zamora added Product-Terminal The new Windows Terminal. Priority-2 A description (P2) Area-AtlasEngine and removed Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting labels Oct 5, 2022
@ghost ghost added the Needs-Tag-Fix Doesn't match tag requirements label Oct 5, 2022
@carlos-zamora carlos-zamora added this to the Terminal v1.17 milestone Oct 5, 2022
@ghost ghost removed the Needs-Tag-Fix Doesn't match tag requirements label Oct 5, 2022
@ghost ghost added the In-PR This issue has a related PR label Nov 5, 2022
@ghost ghost added the Needs-Tag-Fix Doesn't match tag requirements label Jan 24, 2023
lhecker added a commit that referenced this issue Feb 14, 2023
Does what it says in the title. After this commit you can customize the height
and width of the terminal's cells. This commit supports parts of CSS'
`<length-percentage>` data type: Font-size relative sizes as multiples (`1.2`),
percentage (`120%`), or advance-width relative (`1.2ch`), as well as absolute
sizes in CSS pixels (`px`) or points (`pt`).

This PR is neither bug free in DxEngine, nor in AtlasEngine.
The former fails to implement glyph advance corrections (for instance #9381),
as well as disallowing glyphs to overlap rows. The latter has the same
overlap issue, but more severely as it tries to shrink glyphs to fit in.

Closes #3498
Closes #14068

## Validation Steps Performed
* Setting `height` to `1` creates 12pt tall rows ✅
* Setting `height` to `1ch` creates square cells ✅
* Setting `width` to `1` creates square cells ✅
* Setting `width` or `height` to `Npx` or `Npt` works ✅
* Trailing zeroes are trimmed properly during serialization ✅
* Patching the PR to allow >100 line heights and entering "100.123456"
  displays 6 fractional digits ✅
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-AtlasEngine In-PR This issue has a related PR Issue-Bug It either shouldn't be doing this or needs an investigation. Needs-Tag-Fix Doesn't match tag requirements Priority-2 A description (P2) Product-Terminal The new Windows Terminal.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants