[glyphs2fontir] Support stylistic set labels

[khaledhosny]

Convert them to featureNames.

Fixes #950

[khaledhosny]

This change seems to work (I see featureNames blocks in the generated feature code), but the featureParams name IDs seems to get re-used by fvar table, so the final font has the wrong names.

[khaledhosny]

This change seems to work (I see featureNames blocks in the generated feature code), but the featureParams name IDs seems to get re-used by fvar table, so the final font has the wrong names.

That is probably the culprit as it does not use NameBuilder like other tables:

fontc/fontir/src/ir.rs

Lines 346 to 360 in 09025bf

	// Claim names for axes and named instances
	let mut name_id_gen = 255;
	let mut key_to_name = names;
	let mut visited = HashSet::new();
	variable_axes
	.iter()
	.map(|axis| &axis.name)
	.chain(named_instances.iter().map(|ni| &ni.name))
	.for_each(|name| {
	if !visited.insert(name) {
	return;
	}
	name_id_gen += 1;
	key_to_name.insert(NameKey::new(name_id_gen.into(), name), name.clone());
	});

[khaledhosny]

Or rather, the fea-rs compiler have no knowledge of any user name IDs allocated by other tables it doesn’t build itself.

[rsheeter]

Apologies, this came in while I was travelling and got missed. We'll get this reviewed.

[cmyr]

Yea sorry for letting this hang... a couple little notes inline, the only one that I think is important is the one regarding the panic.

There are at least two reasons that this might not result in the names ending up in the final name table; the issue you've identified with fvar, but also the simple fact that I don't believe we currently even use the name table that is produced by fea-rs (currently we only use GDEF/GPOS/GSUB, iirc). There's an old issue up for this, at #406; maybe time to figure this one out.

[cmyr]

total style nit, in a fn like this I generally prefer to use an explicit return for the guard case, so the rest of the logic isn't indented unnecessarily, so like

if self.labels.is_empty() {
    return String::new()
}
// actual logic here

[cmyr]

I don't think we should panic here; instead I would skip this label and emit a warning. You can use filter_map instead of map to skip missing items.

[cmyr]

not critical since this is not a hot code path, but in general I'd prefer doing a binary search to a linear scan here; the one gotcha is we would want to ensure that the array is sorted (which we could do with a test case that created an explicitly sorted copy and compared this with the original)

[cmyr]

great, thanks.

Per your earlier comment this should still not result in the names actually ending up in the final font, because of one or more of the fvar issue & ignoring the fea-rs name table; but let's get this merged and we can worry about that as followup.

[khaledhosny]

great, thanks.

Per your earlier comment this should still not result in the names actually ending up in the final font, because of one or more of the fvar issue & ignoring the fea-rs name table; but let's get this merged and we can worry about that as followup.

Yes, it already already affects feature labels written in the feature file directly (e.g. in UFO sources).