SwiftTerm icon indicating copy to clipboard operation
SwiftTerm copied to clipboard

Messed up character width for some fonts

Open lukepistrol opened this issue 1 year ago • 8 comments

For some fonts the character width is a bit messed up (i.e. too wide).

SwiftTerm:

Screenshot 2023-03-24 at 00 30 59

Apple's Terminal (iTerm looks the same):

Screenshot 2023-03-24 at 00 32 05

The font used in this example is FiraCode but this happens with a couple of other fonts (Source Code Pro for Powerline, Fira Mono for Powerline) too which look totally normal in other Terminal emulators.

lukepistrol avatar Mar 23 '23 23:03 lukepistrol

This is odd, how did you get this setup?

migueldeicaza avatar Apr 17 '23 00:04 migueldeicaza

This is odd, how did you get this setup?

This is the issue I raised at this address(https://github.com/CodeEditApp/CodeEdit/issues/1084). @lukepistrol helped me to re-raise it here. You can check the configuration information of the original issue. If you need other help information, you can contact me

Hukeqing avatar Apr 21 '23 02:04 Hukeqing

Folks, I could appreciate a complete bug report, something I can reproduce on my end.

Otherwise, I spend hours tracking down third party issues. I am using those fonts and do not have that problem on iOS.

migueldeicaza avatar Jul 06 '23 10:07 migueldeicaza

Adding onto this, nerd fonts do not seem to render correctly.

What I'm getting: image

What is expected: image

By the looks of if, the font isn't even the right one.

scrapp08 avatar Aug 29 '23 20:08 scrapp08

I’m seeing this issue with a few fonts as well. FontBug

Screenshot taken in my own project, but it’s reproducible in the MacTerminal sample project by:

  1. Installing the attached version of the free Cascadia Code font
  2. In viewDidLoad() in ViewController.swift, inserting terminal.font = NSFont(name: "Cascadia Code", size: 13)!
  3. Running the app

I’m running macOS 14.3.1 (23D60) on an Apple Silicon machine. Let me know if there are any details I’ve missed, and if it would be helpful I can try to reproduce this in a clean VM and an Intel mac to help rule out possibilities.

CascadiaCode.zip

jwells89 avatar Feb 10 '24 22:02 jwells89

I am still seeing this even when I am not using a nerd font. Here I am using SF Mono.

image

Fonts that are spaced out

  • SF Mono (if installed from Apple's developer website - system monospace font design works)
  • IBM Plex
  • Cascadia Code
  • Fira Code
  • Range Mono

austincondiff avatar Jun 13 '24 22:06 austincondiff

Mhm, interesting that this is happening on Mac, as I am using this on iOS just fine.

Thanks for the additional detail.

migueldeicaza avatar Jun 14 '24 20:06 migueldeicaza

For what it's worth this has been a problem with the integrated terminal in VS Code in the past as you can see in this issue.

Xterm (used by VS Code) has had this issue as well.

austincondiff avatar Jun 14 '24 20:06 austincondiff

Any update for this?

nervenes avatar Jul 21 '24 09:07 nervenes

@migueldeicaza any thoughts as to why this is happening? This continues to be a recurring issue that keeps getting brought up by our contributors/users.

austincondiff avatar Jul 29 '24 01:07 austincondiff

I have not had a chance to look into this, I am sadly juggling a lot of things this summer.

migueldeicaza avatar Jul 29 '24 14:07 migueldeicaza

I'm taking a look into this, it appears to be rooted in how SwiftTerm calculates the cell size for a font on macOS here.

To reproduce, the SF Mono font needs to be installed manually. I'm not sure yet why this causes the problem. SF Mono works fine when not installed manually but returns a much wider width otherwise.

thecoolwinter avatar Aug 01 '24 18:08 thecoolwinter

Oh very good catch - it seems like this would return the largest possible width of any of the characters on the set, which explains the problem, as these fonts likely contain glyphs from languages that use two cells (like Chinese characters, or Emoji).

So probably we need something like the "W" hack that is used for iOS, an option is:

https://developer.apple.com/documentation/appkit/nsfont/2887171-getadvancements?changes=_3

And we would pass all the ascii characters to it, and then get the maximum value out of those - for other characters, there is already special handling in the code for it.

migueldeicaza avatar Aug 01 '24 21:08 migueldeicaza

Could folks try this patch?

diff --git a/Sources/SwiftTerm/Apple/AppleTerminalView.swift b/Sources/SwiftTerm/Apple/AppleTerminalView.swift
index 35ce9ce..db1c404 100644
--- a/Sources/SwiftTerm/Apple/AppleTerminalView.swift
+++ b/Sources/SwiftTerm/Apple/AppleTerminalView.swift
@@ -146,7 +146,8 @@ extension TerminalView {
         let lineLeading = CTFontGetLeading (fontSet.normal)
         let cellHeight = ceil(lineAscent + lineDescent + lineLeading)
         #if os(macOS)
-        let cellWidth = fontSet.normal.maximumAdvancement.width
+        let glyph = font.glyph(withName: "W")
+        cellWidth = fontSet.normal.advancement(forGlyph: glyph)
         #else
         let fontAttributes = [NSAttributedString.Key.font: fontSet.normal]
         let cellWidth = "W".size(withAttributes: fontAttributes).width

migueldeicaza avatar Aug 01 '24 21:08 migueldeicaza

That patch fixes the issue. I did also implement the method you mentioned before with grabbing all ASCII characters and getting the max width. Looks like this:

diff --git a/Sources/SwiftTerm/Apple/AppleTerminalView.swift b/Sources/SwiftTerm/Apple/AppleTerminalView.swift
index 35ce9ce..0eb0900 100644
--- a/Sources/SwiftTerm/Apple/AppleTerminalView.swift
+++ b/Sources/SwiftTerm/Apple/AppleTerminalView.swift
@@ -146,7 +146,16 @@ extension TerminalView {
         let lineLeading = CTFontGetLeading (fontSet.normal)
         let cellHeight = ceil(lineAscent + lineDescent + lineLeading)
         #if os(macOS)
-        let cellWidth = fontSet.normal.maximumAdvancement.width
+        // Grab all non-control ascii characters and get the max width.
+        var sizes = UnsafeMutablePointer<NSSize>.allocate(capacity: 95)
+        let ctFont = (font as CTFont)
+        var glyphs = (32..<127).map { CTFontGetGlyphWithName(ctFont, String(Unicode.Scalar($0)) as CFString) }
+        withUnsafePointer(to: glyphs[0]) { glyphsPtr in
+            fontSet.normal.getAdvancements(NSSizeArray(sizes), forCGGlyphs: glyphsPtr, count: 95)
+        }
+        let cellWidth = (0..<95).reduce(into: 0) { partialResult, idx in
+            partialResult = max(partialResult, sizes[idx].width)
+        }
         #else
         let fontAttributes = [NSAttributedString.Key.font: fontSet.normal]
         let cellWidth = "W".size(withAttributes: fontAttributes).width

I found it didn't make a difference for the calculated with on a few different fonts I tried. It is much slower than grabbing a single glyph (about 0.21 in a rough benchmark vs too small to benchmark).

Bechmark code

I wanted to make sure this wouldn't affect performance.

% swiftc main.swift -o a -Onone && ./a
0.21186870669999877 ms
import Foundation
import AppKit

@_optimize(none)
public func blackHole(_: some Any) {}

var times: [TimeInterval] = []

for _ in 0..<10_000 {
    let font = NSFont.monospacedSystemFont(ofSize: 12, weight: .regular)

    var info = mach_timebase_info()
    guard mach_timebase_info(&info) == KERN_SUCCESS else { exit(1) }
    let start = mach_absolute_time()

    let sizes = UnsafeMutablePointer<NSSize>.allocate(capacity: 95)
    let ctFont = (font as CTFont)
    let glyphs = (32..<127).map { CTFontGetGlyphWithName(ctFont, String(Unicode.Scalar($0)) as CFString) }
    withUnsafePointer(to: glyphs[0]) { glyphsPtr in
        font.getAdvancements(NSSizeArray(sizes), forCGGlyphs: glyphsPtr, count: 95)
    }
    let cellWidth = (0..<95).reduce(into: 0) { partialResult, idx in
        partialResult = max(partialResult, sizes[idx].width)
    }
    blackHole(cellWidth)

    let end = mach_absolute_time()
    let elapsed = end - start
    let nanos = elapsed * UInt64(info.numer) / UInt64(info.denom)
    times.append(TimeInterval(nanos) / TimeInterval(NSEC_PER_MSEC))
}

print(times.reduce(0, +) / Double(times.count), "ms")

I'd be happy to make either of these into a PR. Do you have a preference for either?

thecoolwinter avatar Aug 03 '24 01:08 thecoolwinter

Let us go with my version then, to avoid the startup penalty.

It might be nice to keep your code in a comment, in case we need it one day.

Thank you!

migueldeicaza avatar Aug 03 '24 21:08 migueldeicaza

Opened that up, was out of town for a few days sorry for the delay!

thecoolwinter avatar Aug 07 '24 23:08 thecoolwinter

Closing

migueldeicaza avatar Aug 08 '24 20:08 migueldeicaza