From a2236d1ceb6c3f6d4fbe8d0247b955321449300f Mon Sep 17 00:00:00 2001 From: Mitchell Hashimoto Date: Wed, 13 Sep 2023 14:23:07 -0700 Subject: [PATCH 1/4] font: fallback search must verify presentation --- src/Surface.zig | 18 +++++++-------- src/font/DeferredFace.zig | 6 +++++ src/font/Group.zig | 43 ++++++++++++++++++++++-------------- src/font/GroupCache.zig | 4 ++-- src/font/shaper/harfbuzz.zig | 8 +++---- 5 files changed, 47 insertions(+), 32 deletions(-) diff --git a/src/Surface.zig b/src/Surface.zig index 6392e855f..6b53b07a0 100644 --- a/src/Surface.zig +++ b/src/Surface.zig @@ -239,7 +239,7 @@ pub fn init( defer disco_it.deinit(); if (try disco_it.next()) |face| { log.info("font regular: {s}", .{try face.name(&name_buf)}); - try group.addFace(.regular, .{ .deferred = face }); + _ = try group.addFace(.regular, .{ .deferred = face }); } else log.warn("font-family not found: {s}", .{family}); } if (config.@"font-family-bold") |family| { @@ -252,7 +252,7 @@ pub fn init( defer disco_it.deinit(); if (try disco_it.next()) |face| { log.info("font bold: {s}", .{try face.name(&name_buf)}); - try group.addFace(.bold, .{ .deferred = face }); + _ = try group.addFace(.bold, .{ .deferred = face }); } else log.warn("font-family-bold not found: {s}", .{family}); } if (config.@"font-family-italic") |family| { @@ -265,7 +265,7 @@ pub fn init( defer disco_it.deinit(); if (try disco_it.next()) |face| { log.info("font italic: {s}", .{try face.name(&name_buf)}); - try group.addFace(.italic, .{ .deferred = face }); + _ = try group.addFace(.italic, .{ .deferred = face }); } else log.warn("font-family-italic not found: {s}", .{family}); } if (config.@"font-family-bold-italic") |family| { @@ -279,17 +279,17 @@ pub fn init( defer disco_it.deinit(); if (try disco_it.next()) |face| { log.info("font bold+italic: {s}", .{try face.name(&name_buf)}); - try group.addFace(.bold_italic, .{ .deferred = face }); + _ = try group.addFace(.bold_italic, .{ .deferred = face }); } else log.warn("font-family-bold-italic not found: {s}", .{family}); } } // Our built-in font will be used as a backup - try group.addFace( + _ = try group.addFace( .regular, .{ .loaded = try font.Face.init(font_lib, face_ttf, font_size) }, ); - try group.addFace( + _ = try group.addFace( .bold, .{ .loaded = try font.Face.init(font_lib, face_bold_ttf, font_size) }, ); @@ -300,11 +300,11 @@ pub fn init( // Emoji fallback. We don't include this on Mac since Mac is expected // to always have the Apple Emoji available. if (builtin.os.tag != .macos or font.Discover == void) { - try group.addFace( + _ = try group.addFace( .regular, .{ .loaded = try font.Face.init(font_lib, face_emoji_ttf, font_size) }, ); - try group.addFace( + _ = try group.addFace( .regular, .{ .loaded = try font.Face.init(font_lib, face_emoji_text_ttf, font_size) }, ); @@ -321,7 +321,7 @@ pub fn init( if (try disco_it.next()) |face| { var name_buf: [256]u8 = undefined; log.info("font emoji: {s}", .{try face.name(&name_buf)}); - try group.addFace(.regular, .{ .deferred = face }); + _ = try group.addFace(.regular, .{ .deferred = face }); } } } diff --git a/src/font/DeferredFace.zig b/src/font/DeferredFace.zig index acf5ba93f..63d303d2d 100644 --- a/src/font/DeferredFace.zig +++ b/src/font/DeferredFace.zig @@ -254,6 +254,12 @@ pub fn hasCodepoint(self: DeferredFace, cp: u32, p: ?Presentation) bool { .coretext, .coretext_freetype => { // If we are using coretext, we check the loaded CT font. if (self.ct) |ct| { + if (p) |desired_p| { + const traits = ct.font.getSymbolicTraits(); + const actual_p: Presentation = if (traits.color_glyphs) .emoji else .text; + if (actual_p != desired_p) return false; + } + // Turn UTF-32 into UTF-16 for CT API var unichars: [2]u16 = undefined; const pair = macos.foundation.stringGetSurrogatePairForLongCharacter(cp, &unichars); diff --git a/src/font/Group.zig b/src/font/Group.zig index c338adaa4..bdf1904fe 100644 --- a/src/font/Group.zig +++ b/src/font/Group.zig @@ -99,13 +99,15 @@ pub fn deinit(self: *Group) void { /// /// The group takes ownership of the face. The face will be deallocated when /// the group is deallocated. -pub fn addFace(self: *Group, style: Style, face: GroupFace) !void { +pub fn addFace(self: *Group, style: Style, face: GroupFace) !FontIndex { const list = self.faces.getPtr(style); // We have some special indexes so we must never pass those. if (list.items.len >= FontIndex.Special.start - 1) return error.GroupFull; + const idx = list.items.len; try list.append(self.alloc, face); + return .{ .style = style, .idx = @intCast(idx) }; } /// Returns true if we have a face for the given style, though the face may @@ -259,13 +261,17 @@ pub fn indexForCodepoint( defer disco_it.deinit(); if (disco_it.next() catch break :discover) |face| { - var buf: [256]u8 = undefined; - log.info("found codepoint 0x{x} in fallback face={s}", .{ - cp, - face.name(&buf) catch "", - }); - self.addFace(style, .{ .deferred = face }) catch break :discover; - if (self.indexForCodepointExact(cp, style, p)) |value| return value; + // Discovery is supposed to only return faces that have our + // codepoint but we can't search presentation in discovery so + // we have to check it here. + if (face.hasCodepoint(cp, p)) { + var buf: [256]u8 = undefined; + log.info("found codepoint 0x{x} in fallback face={s}", .{ + cp, + face.name(&buf) catch "", + }); + return self.addFace(style, .{ .deferred = face }) catch break :discover; + } } } } @@ -273,7 +279,7 @@ pub fn indexForCodepoint( // If this is already regular, we're done falling back. if (style == .regular and p == null) return null; - // For non-regular fonts, we fall back to regular. + // For non-regular fonts, we fall back to regular with no presentation return self.indexForCodepointExact(cp, .regular, null); } @@ -305,10 +311,13 @@ pub fn hasCodepoint(self: *Group, index: FontIndex, cp: u32, p: ?Presentation) b const list = self.faces.getPtr(index.style); if (index.idx >= list.items.len) return false; const item = list.items[index.idx]; + std.log.warn("hasCodepoint item={}", .{item}); return switch (item) { .deferred => |v| v.hasCodepoint(cp, p), .loaded => |face| loaded: { - if (p) |desired| if (face.presentation != desired) break :loaded false; + if (p) |desired| { + if (face.presentation != desired) break :loaded false; + } break :loaded face.glyphIndex(cp) != null; }, }; @@ -501,13 +510,13 @@ test { var group = try init(alloc, lib, .{ .points = 12 }); defer group.deinit(); - try group.addFace(.regular, .{ .loaded = try Face.init(lib, testFont, .{ .points = 12 }) }); + _ = try group.addFace(.regular, .{ .loaded = try Face.init(lib, testFont, .{ .points = 12 }) }); if (font.options.backend != .coretext) { // Coretext doesn't support Noto's format - try group.addFace(.regular, .{ .loaded = try Face.init(lib, testEmoji, .{ .points = 12 }) }); + _ = try group.addFace(.regular, .{ .loaded = try Face.init(lib, testEmoji, .{ .points = 12 }) }); } - try group.addFace(.regular, .{ .loaded = try Face.init(lib, testEmojiText, .{ .points = 12 }) }); + _ = try group.addFace(.regular, .{ .loaded = try Face.init(lib, testEmojiText, .{ .points = 12 }) }); // Should find all visible ASCII var i: u32 = 32; @@ -569,7 +578,7 @@ test "face count limit" { defer group.deinit(); for (0..FontIndex.Special.start - 1) |_| { - try group.addFace(.regular, .{ .loaded = try Face.init(lib, testFont, .{ .points = 12 }) }); + _ = try group.addFace(.regular, .{ .loaded = try Face.init(lib, testFont, .{ .points = 12 }) }); } try testing.expectError(error.GroupFull, group.addFace( @@ -624,7 +633,7 @@ test "resize" { var group = try init(alloc, lib, .{ .points = 12, .xdpi = 96, .ydpi = 96 }); defer group.deinit(); - try group.addFace(.regular, .{ .loaded = try Face.init(lib, testFont, .{ .points = 12, .xdpi = 96, .ydpi = 96 }) }); + _ = try group.addFace(.regular, .{ .loaded = try Face.init(lib, testFont, .{ .points = 12, .xdpi = 96, .ydpi = 96 }) }); // Load a letter { @@ -677,7 +686,7 @@ test "discover monospace with fontconfig and freetype" { defer lib.deinit(); var group = try init(alloc, lib, .{ .points = 12 }); defer group.deinit(); - try group.addFace(.regular, .{ .deferred = (try it.next()).? }); + _ = try group.addFace(.regular, .{ .deferred = (try it.next()).? }); // Should find all visible ASCII var atlas_greyscale = try font.Atlas.init(alloc, 512, .greyscale); @@ -715,7 +724,7 @@ test "faceFromIndex returns pointer" { var group = try init(alloc, lib, .{ .points = 12, .xdpi = 96, .ydpi = 96 }); defer group.deinit(); - try group.addFace(.regular, .{ .loaded = try Face.init(lib, testFont, .{ .points = 12, .xdpi = 96, .ydpi = 96 }) }); + _ = try group.addFace(.regular, .{ .loaded = try Face.init(lib, testFont, .{ .points = 12, .xdpi = 96, .ydpi = 96 }) }); { const idx = group.indexForCodepoint('A', .regular, null).?; diff --git a/src/font/GroupCache.zig b/src/font/GroupCache.zig index b869e2b5f..504a88ba9 100644 --- a/src/font/GroupCache.zig +++ b/src/font/GroupCache.zig @@ -182,7 +182,7 @@ test { defer cache.deinit(alloc); // Setup group - try cache.group.addFace( + _ = try cache.group.addFace( .regular, .{ .loaded = try Face.init(lib, testFont, .{ .points = 12 }) }, ); @@ -338,7 +338,7 @@ test "resize" { defer cache.deinit(alloc); // Setup group - try cache.group.addFace( + _ = try cache.group.addFace( .regular, .{ .loaded = try Face.init(lib, testFont, .{ .points = 12, .xdpi = 96, .ydpi = 96 }) }, ); diff --git a/src/font/shaper/harfbuzz.zig b/src/font/shaper/harfbuzz.zig index 4069a526b..547829a3b 100644 --- a/src/font/shaper/harfbuzz.zig +++ b/src/font/shaper/harfbuzz.zig @@ -896,11 +896,11 @@ fn testShaper(alloc: Allocator) !TestShaper { errdefer cache_ptr.*.deinit(alloc); // Setup group - try cache_ptr.group.addFace(.regular, .{ .loaded = try Face.init(lib, testFont, .{ .points = 12 }) }); + _ = try cache_ptr.group.addFace(.regular, .{ .loaded = try Face.init(lib, testFont, .{ .points = 12 }) }); if (font.options.backend != .coretext) { // Coretext doesn't support Noto's format - try cache_ptr.group.addFace(.regular, .{ .loaded = try Face.init(lib, testEmoji, .{ .points = 12 }) }); + _ = try cache_ptr.group.addFace(.regular, .{ .loaded = try Face.init(lib, testEmoji, .{ .points = 12 }) }); } else { // On CoreText we want to load Apple Emoji, we should have it. var disco = font.Discover.init(); @@ -912,9 +912,9 @@ fn testShaper(alloc: Allocator) !TestShaper { defer disco_it.deinit(); var face = (try disco_it.next()).?; errdefer face.deinit(); - try cache_ptr.group.addFace(.regular, .{ .deferred = face }); + _ = try cache_ptr.group.addFace(.regular, .{ .deferred = face }); } - try cache_ptr.group.addFace(.regular, .{ .loaded = try Face.init(lib, testEmojiText, .{ .points = 12 }) }); + _ = try cache_ptr.group.addFace(.regular, .{ .loaded = try Face.init(lib, testEmojiText, .{ .points = 12 }) }); var cell_buf = try alloc.alloc(font.shape.Cell, 80); errdefer alloc.free(cell_buf); From e459f0f94b3a06f073993c8bfa2fc47742f96eb1 Mon Sep 17 00:00:00 2001 From: Mitchell Hashimoto Date: Wed, 13 Sep 2023 14:41:01 -0700 Subject: [PATCH 2/4] core: do not do emoji font fallback on Linux Our font discovery mechanism is capable of finding this. --- src/Surface.zig | 13 ------------- 1 file changed, 13 deletions(-) diff --git a/src/Surface.zig b/src/Surface.zig index 6b53b07a0..01cbafb89 100644 --- a/src/Surface.zig +++ b/src/Surface.zig @@ -297,19 +297,6 @@ pub fn init( // Auto-italicize if we have to. try group.italicize(); - // Emoji fallback. We don't include this on Mac since Mac is expected - // to always have the Apple Emoji available. - if (builtin.os.tag != .macos or font.Discover == void) { - _ = try group.addFace( - .regular, - .{ .loaded = try font.Face.init(font_lib, face_emoji_ttf, font_size) }, - ); - _ = try group.addFace( - .regular, - .{ .loaded = try font.Face.init(font_lib, face_emoji_text_ttf, font_size) }, - ); - } - // If we're on Mac, then we try to use the Apple Emoji font for Emoji. if (builtin.os.tag == .macos and font.Discover != void) { if (try app.fontDiscover()) |disco| { From 3c7ba634d2476ec0ac857411153743f981a4166c Mon Sep 17 00:00:00 2001 From: Mitchell Hashimoto Date: Wed, 13 Sep 2023 14:41:43 -0700 Subject: [PATCH 3/4] font: remove excess debugging code --- src/font/Group.zig | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/src/font/Group.zig b/src/font/Group.zig index bdf1904fe..404c3c322 100644 --- a/src/font/Group.zig +++ b/src/font/Group.zig @@ -311,13 +311,10 @@ pub fn hasCodepoint(self: *Group, index: FontIndex, cp: u32, p: ?Presentation) b const list = self.faces.getPtr(index.style); if (index.idx >= list.items.len) return false; const item = list.items[index.idx]; - std.log.warn("hasCodepoint item={}", .{item}); return switch (item) { .deferred => |v| v.hasCodepoint(cp, p), .loaded => |face| loaded: { - if (p) |desired| { - if (face.presentation != desired) break :loaded false; - } + if (p) |desired| if (face.presentation != desired) break :loaded false; break :loaded face.glyphIndex(cp) != null; }, }; From 5e75752d7d82b8493fbfd809e78e79dbd8b76a3d Mon Sep 17 00:00:00 2001 From: Mitchell Hashimoto Date: Wed, 13 Sep 2023 14:42:56 -0700 Subject: [PATCH 4/4] core: do not preload Apple Emoji, our fallback search can find it --- src/Surface.zig | 16 ---------------- 1 file changed, 16 deletions(-) diff --git a/src/Surface.zig b/src/Surface.zig index 01cbafb89..83205cb7d 100644 --- a/src/Surface.zig +++ b/src/Surface.zig @@ -297,22 +297,6 @@ pub fn init( // Auto-italicize if we have to. try group.italicize(); - // If we're on Mac, then we try to use the Apple Emoji font for Emoji. - if (builtin.os.tag == .macos and font.Discover != void) { - if (try app.fontDiscover()) |disco| { - var disco_it = try disco.discover(.{ - .family = "Apple Color Emoji", - .size = font_size.points, - }); - defer disco_it.deinit(); - if (try disco_it.next()) |face| { - var name_buf: [256]u8 = undefined; - log.info("font emoji: {s}", .{try face.name(&name_buf)}); - _ = try group.addFace(.regular, .{ .deferred = face }); - } - } - } - break :group group; }); errdefer font_group.deinit(alloc);