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.

Sign up for GitHub

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

Show kerning groups in the sidebar. #241

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Show kerning groups in the sidebar. #241

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 31 additions & 0 deletions runebender-lib/src/data.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,6 +87,8 @@ pub struct GlyphDetail {
// the full outline, including things like components
pub outline: Arc<BezPath>,
metrics: FontMetrics,
pub kern1_group: String,
pub kern2_group: String,
is_placeholder: bool,
}

Expand Down Expand Up @@ -599,6 +601,7 @@ mod lenses {
use std::sync::Arc;

use druid::{Data, Lens};
use norad::glyph::Glyph;
use norad::GlyphName as GlyphName_;

use super::{
Expand Down Expand Up @@ -722,10 +725,13 @@ mod lenses {
let outline = state.font.get_bezier(&glyph.name);
let is_placeholder = outline.is_none();
let metrics = state.font.info.metrics.clone();
let (kern1_group, kern2_group) = find_kern_groups(&state.font, &glyph);
GlyphDetail {
glyph,
outline: outline.unwrap_or_else(|| state.font.font.placeholder.clone()),
is_placeholder,
kern1_group,
kern2_group,
metrics,
}
}
Expand Down Expand Up @@ -762,10 +768,13 @@ mod lenses {
let outline = data.get_bezier(&glyph.name);
let is_placeholder = outline.is_none();
let metrics = data.info.metrics.clone();
let (kern1_group, kern2_group) = find_kern_groups(data, glyph);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lens get processed a lot, so I'm cautious of doing work in them that might get expensive. I guess this isn't too bad, being O(nm) over the number of glyphs and number of groups, which have practical upper bounds?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Due to the way that it searches, I think it is always worst case on n, and the typical case for m is hard to say.

The better way to do this would be to have the core glyph datastructure hold the kern1 and kern2 group until it's time to save, since UFO demands there only be one of them per glyph anyway. The thing that makes that messy is that the glyphs as they come from Norad are not coupled to the font overall; this is nice for concurrent loading, but because UFO is kinda a mess, it makes it uglier to meet the constraints.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so maybe a compromise would be to have something like a HashMap<GlyphName, [GroupName; 2]> in the runebender FontObject, or something?

Copy link
Member Author

@xorgy xorgy Apr 14, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, that could work; it would definitely be faster. This seems like a problem that should be solved at the Norad level though, since it deals with maintaining a spec constraint.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd prefer norad to be a fairly transparent mapping of the UFO object on disk; so while I agree it's a bit of a funny fit in runebender that's still my current preference? I'm not sure exactly what the rules/constraints are with kern groups though, so maybe @madig has an opinion?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe between Norad and Runebender, we need uhh... a federal bureaucracy.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

RE the constraints on kern groups: the UFO spec says that a given glyph must not appear in more than one kerning group per side.

https://unifiedfontobject.org/versions/ufo3/groups.plist/#4-glyphs-must-not-appear-in-more-than-one-kerning-group-per-side

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@madig how would you feel if we had, in norad, a representation of kerning groups that was distinct from what we load from and write to disk? This would just be some sort of map, where each glyph could have left and right kerning groups assigned. This seems better than exposing a literal representation of the UFO kern-groups to users?

Copy link

@madig madig Apr 16, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm fine with whatever fits Runebender best, but keep in mind that splitting data out of structures requires that you be careful when merging them back in. If you split out kerning groups into their own structure and the user somehow writes kerning groups into the original structure from out under you (scripting), you'll overwrite things or end up with inconsistent state.

Glyphs also has a UI where you set left and right group at glyph level and it serializes into a HashMap<String, HashMap<String, String>>.

GlyphDetail {
glyph: Arc::clone(glyph),
outline: outline.unwrap_or_else(|| data.font.placeholder.clone()),
metrics,
kern1_group,
kern2_group,
is_placeholder,
}
});
Expand All @@ -786,10 +795,13 @@ mod lenses {
let outline = data.get_bezier(&glyph.name);
let is_placeholder = outline.is_none();
let metrics = data.info.metrics.clone();
let (kern1_group, kern2_group) = find_kern_groups(data, glyph);
GlyphDetail {
glyph: Arc::clone(glyph),
outline: outline.unwrap_or_else(|| data.font.placeholder.clone()),
metrics,
kern1_group,
kern2_group,
is_placeholder,
}
});
Expand Down Expand Up @@ -911,6 +923,25 @@ mod lenses {
f(&mut s)
}
}

fn find_kern_groups(data: &Workspace, glyph: &Glyph) -> (String, String) {
let mut kern1_group = "".to_string();
let mut kern2_group = "".to_string();
Comment on lines +928 to +929
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess a few different things here:

  • should these be Option instead of String?
  • you might prefer String::new() for creating a new, empty String.

At a higher level: I wonder if runebender shouldn't keep track of kern groups via some other mechanism, and only extract / write back to the UFO format as needed? trying to manipulate this directly just feels so awkward.

if let Some(gs) = &data.font.ufo.groups {
for (gk, gv) in gs.iter() {
if let Some(gn) = gk.strip_prefix("public.kern1.") {
if (*gv).iter().any(|n| *n == glyph.name) {
kern1_group = gn.to_string();
}
} else if let Some(gn) = gk.strip_prefix("public.kern2.") {
if (*gv).iter().any(|n| *n == glyph.name) {
kern2_group = gn.to_string();
}
}
}
}
(kern1_group, kern2_group)
}
}

//FIXME: put this in some `GlyphExt` trait or something
Expand Down
4 changes: 2 additions & 2 deletions runebender-lib/src/widgets/sidebar.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,13 +63,13 @@ fn selected_glyph_widget() -> impl Widget<GlyphDetail> {
.with_child(
Flex::row()
.with_child(
Label::new("kern group")
Label::dynamic(|d: &GlyphDetail, _| (*d.kern2_group).to_string())
.with_text_color(theme::SECONDARY_TEXT_COLOR)
.with_font(theme::UI_DETAIL_FONT),
)
.with_flex_spacer(1.0)
.with_child(
Label::new("kern group")
Label::dynamic(|d: &GlyphDetail, _| (*d.kern1_group).to_string())
.with_text_color(theme::SECONDARY_TEXT_COLOR)
.with_font(theme::UI_DETAIL_FONT),
)
Expand Down