[klippa] subset CBLC/CBDT tables #1351
Open
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
cmyr
approved these changes
Feb 11, 2025
| @@ -343,6 +346,13 @@ impl Plan { | |||
| this.populate_gids_to_retain(font); | |||
| this.create_old_gid_to_new_gid_map(); | |||
|
|
|||
| //update the unicode to new gid list | |||
| let num = this.unicode_to_new_gid_list.len(); | |||
There was a problem hiding this comment.
is this related to the cblc/cbdt changes?
| @@ -807,13 +818,14 @@ pub trait Subset { | |||
| // A helper trait providing a 'subset' method for various subtables that have no associated tag | |||
| pub(crate) trait SubsetTable<'a> { | |||
| type ArgsForSubset: 'a; | |||
| type SubsetOutput: 'a; | |||
| /// Subset this table and write a subset version of this table into serializer | |||
| fn subset( | |||
| &self, | |||
| plan: &Plan, | |||
There was a problem hiding this comment.
I'd just call this Output, we know it's part of the SubsetTable trait. (Looking back I would have also suggested Args instead of ArgsForSubset but maybe that name was about avoiding clashing with FontRead?
Comment on lines
+2
to
+6
| use crate::serialize::{OffsetWhence, SerializeErrorFlags, Serializer}; | ||
| use crate::{Plan, Subset, SubsetError, SubsetTable}; | ||
| use skrifa::raw::collections::IntSet; | ||
| use write_fonts::types::FixedSize; | ||
| use write_fonts::{ |
There was a problem hiding this comment.
total nit but this can be cleaned up a bit..
Comment on lines
+92
to
+95
| let cblc = args.0; | ||
| let cbdt = args.1; | ||
| let src_bytes = args.2; | ||
| let cbdt_out = args.3; |
There was a problem hiding this comment.
or,
let (cblc, cbdt, src_bytes, cbdt_out) = args;| impl<'a> SubsetTable<'a> for IndexSubtableList<'a> { | ||
| type ArgsForSubset = (&'a Cbdt<'a>, &'a mut Vec<u8>); | ||
| // min_gid(new), max_gid(new), indexSubtableListSize, numberOfIndexSubtables | ||
| type SubsetOutput = (GlyphId, GlyphId, usize, usize); |
There was a problem hiding this comment.
you could consider defining a simple struct for passing in these args, so that you have named fields? in this case in particular it seems easy to get the argument order wrong.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Also updated SubsetTable trait.
Harfbuzz implementation is suboptimal, rust code is using a different approach, following fonttools' method:
https://github.com/fonttools/fonttools/blob/7854669acd63be43e1ad41d0486297d8d6da325d/Lib/fontTools/subset/__init__.py#L1799