Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
21 changes: 11 additions & 10 deletions parley/src/resolve/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -11,8 +11,8 @@ pub(crate) use range::RangedStyleBuilder;
use alloc::{vec, vec::Vec};

use super::style::{
Brush, FontFamily, FontFamilyName, FontFeature, FontSettings, FontStyle, FontVariation,
FontWeight, FontWidth, StyleProperty,
Brush, FontFamily, FontFamilyName, FontFeature, FontFeatures, FontStyle, FontVariation,
FontVariations, FontWeight, FontWidth, StyleProperty,
};
use crate::font::FontContext;
use crate::style::TextStyle;
Expand Down Expand Up @@ -262,15 +262,15 @@ impl ResolveContext {
/// Resolves font variation settings.
pub(crate) fn resolve_variations(
&mut self,
variations: &FontSettings<'_, FontVariation>,
variations: &FontVariations<'_>,
) -> Resolved<FontVariation> {
match variations {
FontSettings::Source(source) => {
FontVariations::Source(source) => {
self.tmp_variations.clear();
self.tmp_variations
.extend(FontVariation::parse_list(source));
.extend(FontVariation::parse_css_list(source).map_while(Result::ok));
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should this be filter_map instead? Seems useful to skip invalid entries when possible.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll just give the Codex response here which I think I agree with:

filter_map(Result::ok) only helps if the iterator can continue producing items after an error. Our parse_css_list iterators don’t recover: on the first Err they set done = true and terminate. In that world:

  • map_while(Result::ok) = “take items until first error”
  • filter_map(Result::ok) = “drop the error, but there are no later items anyway”

So switching to filter_map wouldn’t actually “skip invalid entries when possible” today; it would just slightly change the signal of intent (and arguably mislead future readers into thinking we do recovery).

If the intent is “stop at first parse error, keep earlier valid ones” (matching the old take_while(ok) behavior), map_while(Result::ok) is the clearer expression.
If the intent is “try to salvage later entries”, we’d need to change the parser to recover and keep iterating; then filter_map would make sense.

}
FontSettings::List(settings) => {
FontVariations::List(settings) => {
self.tmp_variations.clear();
self.tmp_variations.extend_from_slice(settings);
}
Expand All @@ -287,14 +287,15 @@ impl ResolveContext {
/// Resolves font feature settings.
pub(crate) fn resolve_features(
&mut self,
features: &FontSettings<'_, FontFeature>,
features: &FontFeatures<'_>,
) -> Resolved<FontFeature> {
match features {
FontSettings::Source(source) => {
FontFeatures::Source(source) => {
self.tmp_features.clear();
self.tmp_features.extend(FontFeature::parse_list(source));
self.tmp_features
.extend(FontFeature::parse_css_list(source).map_while(Result::ok));
}
FontSettings::List(settings) => {
FontFeatures::List(settings) => {
self.tmp_features.clear();
self.tmp_features.extend_from_slice(settings);
}
Expand Down
2 changes: 1 addition & 1 deletion parley/src/setting.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,4 +3,4 @@

//! OpenType settings (features and variations).

pub use text_primitives::{Setting, Tag};
pub use text_primitives::{FontFeature, FontVariation, Tag};
79 changes: 52 additions & 27 deletions parley/src/style/font.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,50 +2,75 @@
// SPDX-License-Identifier: Apache-2.0 OR MIT

use alloc::borrow::Cow;
use alloc::borrow::ToOwned;
use core::fmt;

use crate::setting::Setting;

pub use crate::setting::{FontFeature, FontVariation};
pub use fontique::{FontStyle, FontWeight, FontWidth, GenericFamily};
pub use text_primitives::{FontFamily, FontFamilyName};

/// Setting for a font variation.
pub type FontVariation = Setting<f32>;
/// Font variation settings that can be supplied as a raw source string or a parsed slice.
#[derive(Clone, PartialEq, Debug)]
pub enum FontVariations<'a> {
/// Setting source in CSS format.
Source(Cow<'a, str>),
/// List of settings.
List(Cow<'a, [FontVariation]>),
}

impl<'a> FontVariations<'a> {
/// Creates an empty list of font variations.
pub const fn empty() -> Self {
Self::List(Cow::Borrowed(&[]))
}
}

impl<'a> From<&'a str> for FontVariations<'a> {
fn from(value: &'a str) -> Self {
Self::Source(Cow::Borrowed(value))
}
}

impl<'a> From<&'a [FontVariation]> for FontVariations<'a> {
fn from(value: &'a [FontVariation]) -> Self {
Self::List(Cow::Borrowed(value))
}
}

/// Setting for a font feature.
pub type FontFeature = Setting<u16>;
impl<'a, const N: usize> From<&'a [FontVariation; N]> for FontVariations<'a> {
fn from(value: &'a [FontVariation; N]) -> Self {
Self::List(Cow::Borrowed(&value[..]))
}
}

/// Font settings that can be supplied as a raw source string or
/// a parsed slice.
/// Font feature settings that can be supplied as a raw source string or a parsed slice.
#[derive(Clone, PartialEq, Debug)]
pub enum FontSettings<'a, T>
where
[T]: ToOwned,
<[T] as ToOwned>::Owned: fmt::Debug + PartialEq + Clone,
{
pub enum FontFeatures<'a> {
/// Setting source in CSS format.
Source(Cow<'a, str>),
/// List of settings.
List(Cow<'a, [T]>),
List(Cow<'a, [FontFeature]>),
}

impl<'a, T> From<&'a str> for FontSettings<'a, T>
where
[T]: ToOwned,
<[T] as ToOwned>::Owned: fmt::Debug + PartialEq + Clone,
{
impl<'a> FontFeatures<'a> {
/// Creates an empty list of font features.
pub const fn empty() -> Self {
Self::List(Cow::Borrowed(&[]))
}
}

impl<'a> From<&'a str> for FontFeatures<'a> {
fn from(value: &'a str) -> Self {
Self::Source(Cow::Borrowed(value))
}
}

impl<'a, T> From<&'a [T]> for FontSettings<'a, T>
where
[T]: ToOwned,
<[T] as ToOwned>::Owned: fmt::Debug + PartialEq + Clone,
{
fn from(value: &'a [T]) -> Self {
impl<'a> From<&'a [FontFeature]> for FontFeatures<'a> {
fn from(value: &'a [FontFeature]) -> Self {
Self::List(Cow::Borrowed(value))
}
}

impl<'a, const N: usize> From<&'a [FontFeature; N]> for FontFeatures<'a> {
fn from(value: &'a [FontFeature; N]) -> Self {
Self::List(Cow::Borrowed(&value[..]))
}
}
28 changes: 20 additions & 8 deletions parley/src/style/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -11,8 +11,8 @@ use alloc::borrow::Cow;

pub use brush::*;
pub use font::{
FontFamily, FontFamilyName, FontFeature, FontSettings, FontStyle, FontVariation, FontWeight,
FontWidth, GenericFamily,
FontFamily, FontFamilyName, FontFeature, FontFeatures, FontStyle, FontVariation,
FontVariations, FontWeight, FontWidth, GenericFamily,
};
pub use styleset::StyleSet;
pub use text_primitives::{OverflowWrap, TextWrapMode, WordBreak};
Expand Down Expand Up @@ -81,9 +81,9 @@ pub enum StyleProperty<'a, B: Brush> {
/// Font weight.
FontWeight(FontWeight),
/// Font variation settings.
FontVariations(FontSettings<'a, FontVariation>),
FontVariations(FontVariations<'a>),
/// Font feature settings.
FontFeatures(FontSettings<'a, FontFeature>),
FontFeatures(FontFeatures<'a>),
/// Locale.
Locale(Option<&'a str>),
/// Brush for rendering text.
Expand Down Expand Up @@ -132,9 +132,9 @@ pub struct TextStyle<'a, B: Brush> {
/// Font weight.
pub font_weight: FontWeight,
/// Font variation settings.
pub font_variations: FontSettings<'a, FontVariation>,
pub font_variations: FontVariations<'a>,
/// Font feature settings.
pub font_features: FontSettings<'a, FontFeature>,
pub font_features: FontFeatures<'a>,
/// Locale.
pub locale: Option<&'a str>,
/// Brush for rendering text.
Expand Down Expand Up @@ -177,8 +177,8 @@ impl<B: Brush> Default for TextStyle<'_, B> {
font_width: FontWidth::default(),
font_style: FontStyle::default(),
font_weight: FontWeight::default(),
font_variations: FontSettings::List(Cow::Borrowed(&[])),
font_features: FontSettings::List(Cow::Borrowed(&[])),
font_variations: FontVariations::empty(),
font_features: FontFeatures::empty(),
locale: None,
brush: B::default(),
has_underline: false,
Expand Down Expand Up @@ -217,6 +217,18 @@ impl<'a, B: Brush> From<FontFamilyName<'a>> for StyleProperty<'a, B> {
}
}

impl<'a, B: Brush> From<FontVariations<'a>> for StyleProperty<'a, B> {
fn from(value: FontVariations<'a>) -> Self {
StyleProperty::FontVariations(value)
}
}

impl<'a, B: Brush> From<FontFeatures<'a>> for StyleProperty<'a, B> {
fn from(value: FontFeatures<'a>) -> Self {
StyleProperty::FontFeatures(value)
}
}

impl<B: Brush> From<GenericFamily> for StyleProperty<'_, B> {
fn from(f: GenericFamily) -> Self {
StyleProperty::FontFamily(f.into())
Expand Down
26 changes: 11 additions & 15 deletions parley/src/tests/test_basic.rs
Original file line number Diff line number Diff line change
@@ -1,8 +1,6 @@
// Copyright 2024 the Parley Authors
// SPDX-License-Identifier: Apache-2.0 OR MIT

use std::borrow::Cow;

use peniko::{
color::{AlphaColor, Srgb, palette},
kurbo::Size,
Expand All @@ -11,10 +9,10 @@ use peniko::{
use super::utils::{
ColorBrush, FONT_FAMILY_LIST, TestEnv, asserts::assert_eq_layout_data_alignments,
};
use crate::setting::Setting;
use crate::setting::{FontFeature, FontVariation};
use crate::{
Alignment, AlignmentOptions, ContentWidths, FontFamily, FontSettings, InlineBox, Layout,
LineHeight, StyleProperty, TextStyle, WhiteSpaceCollapse, test_name,
Alignment, AlignmentOptions, ContentWidths, FontFamily, FontFeatures, FontVariations,
InlineBox, Layout, LineHeight, StyleProperty, TextStyle, WhiteSpaceCollapse, test_name,
};

#[test]
Expand Down Expand Up @@ -615,17 +613,17 @@ fn font_features() {
let text = "fi ".repeat(4);
let mut builder = env.ranged_builder(&text);
builder.push(
StyleProperty::FontFeatures(FontSettings::List(Cow::Borrowed(&[Setting {
FontFeatures::from(&[FontFeature {
tag: crate::setting::Tag::new(b"liga"),
value: 1,
}]))),
}]),
0..5,
);
builder.push(
StyleProperty::FontFeatures(FontSettings::List(Cow::Borrowed(&[Setting {
FontFeatures::from(&[FontFeature {
tag: crate::setting::Tag::new(b"liga"),
value: 0,
}]))),
}]),
5..10,
);
let mut layout = builder.build(&text);
Expand All @@ -643,12 +641,10 @@ fn variable_fonts() {
for wght in [100., 500., 1000.] {
let mut builder = env.ranged_builder(text);
builder.push_default(FontFamily::named("Arimo"));
builder.push_default(StyleProperty::FontVariations(FontSettings::List(
Cow::Borrowed(&[Setting {
tag: crate::setting::Tag::new(b"wght"),
value: wght,
}]),
)));
builder.push_default(FontVariations::from(&[FontVariation::new(
crate::setting::Tag::new(b"wght"),
wght,
)]));
let mut layout = builder.build(text);
layout.break_all_lines(Some(100.0));
layout.align(None, Alignment::Start, AlignmentOptions::default());
Expand Down
17 changes: 6 additions & 11 deletions parley/src/tests/test_builders.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,14 +5,13 @@

use fontique::{FontStyle, FontWeight, FontWidth};
use peniko::color::palette;
use std::borrow::Cow;

use super::utils::{
ColorBrush, FONT_FAMILY_LIST, asserts::assert_eq_layout_data, create_font_context,
};
use crate::{
FontContext, FontFamily, FontSettings, Layout, LayoutContext, LineHeight, OverflowWrap,
RangedBuilder, StyleProperty, TextStyle, TextWrapMode, TreeBuilder, WordBreak,
FontContext, FontFamily, FontFeatures, FontVariations, Layout, LayoutContext, LineHeight,
OverflowWrap, RangedBuilder, StyleProperty, TextStyle, TextWrapMode, TreeBuilder, WordBreak,
};

/// Set of options for [`build_layout_with_ranged`].
Expand Down Expand Up @@ -168,8 +167,8 @@ fn create_root_style() -> TextStyle<'static, ColorBrush> {
font_width: FontWidth::CONDENSED,
font_style: FontStyle::Italic,
font_weight: FontWeight::BOLD,
font_variations: FontSettings::List(Cow::Borrowed(&[])), // TODO: Set a non-default value
font_features: FontSettings::List(Cow::Borrowed(&[])), // TODO: Set a non-default value
font_variations: FontVariations::empty(), // TODO: Set a non-default value
font_features: FontFeatures::empty(), // TODO: Set a non-default value
locale: Some("en-US"),
brush: ColorBrush::new(palette::css::GREEN),
has_underline: true,
Expand Down Expand Up @@ -198,12 +197,8 @@ fn set_root_style(rb: &mut RangedBuilder<'_, ColorBrush>) {
rb.push_default(StyleProperty::FontWidth(FontWidth::CONDENSED));
rb.push_default(StyleProperty::FontStyle(FontStyle::Italic));
rb.push_default(StyleProperty::FontWeight(FontWeight::BOLD));
rb.push_default(StyleProperty::FontVariations(FontSettings::List(
Cow::Borrowed(&[]),
)));
rb.push_default(StyleProperty::FontFeatures(FontSettings::List(
Cow::Borrowed(&[]),
)));
rb.push_default(FontVariations::empty());
rb.push_default(FontFeatures::empty());
rb.push_default(StyleProperty::Locale(Some("en-US")));
rb.push_default(StyleProperty::Brush(ColorBrush::new(palette::css::GREEN)));
rb.push_default(StyleProperty::Underline(true));
Expand Down
2 changes: 1 addition & 1 deletion text_primitives/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -53,5 +53,5 @@ pub use font::{FontStyle, FontWeight, FontWidth};
pub use font_family::{FontFamily, FontFamilyName, ParseFontFamilyError, ParseFontFamilyErrorKind};
pub use generic_family::GenericFamily;
pub use language::{Language, ParseLanguageError};
pub use tag::{Setting, Tag};
pub use tag::{FontFeature, FontVariation, ParseSettingsError, ParseSettingsErrorKind, Tag};
pub use text::{BaseDirection, OverflowWrap, TextWrapMode, WordBreak};
Loading