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

Jump to bottom

windows: Support all OpenType font features #10756

Merged
merged 24 commits into from
Apr 26, 2024
Merged

windows: Support all OpenType font features #10756

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
24 commits
Select commit Hold shift + click to select a range
fd7bcc1
init
JunkuiZhang Apr 19, 2024
e4d92ce
update settings
JunkuiZhang Apr 19, 2024
3eede9e
update editor
JunkuiZhang Apr 19, 2024
cb364a9
update editor
JunkuiZhang Apr 19, 2024
a3f0919
update search
JunkuiZhang Apr 19, 2024
e31c069
update outline
JunkuiZhang Apr 19, 2024
b31b4d5
update extension_ui
JunkuiZhang Apr 19, 2024
819b7c8
update terminal view
JunkuiZhang Apr 19, 2024
b7a8b45
update assistant panel
JunkuiZhang Apr 19, 2024
058eb15
update assistant
JunkuiZhang Apr 19, 2024
303005a
update terminal
JunkuiZhang Apr 19, 2024
223aa64
update collab
JunkuiZhang Apr 19, 2024
289a32e
update collab panel
JunkuiZhang Apr 19, 2024
a4d6060
use these features
JunkuiZhang Apr 19, 2024
0e392f5
refactor
JunkuiZhang Apr 19, 2024
a1e917e
update `serialize`
JunkuiZhang Apr 19, 2024
781366d
properly handle error
JunkuiZhang Apr 19, 2024
c506828
add removed ;
JunkuiZhang Apr 19, 2024
9c7daf9
make import windows only
JunkuiZhang Apr 19, 2024
5d82c5b
fix macOS
JunkuiZhang Apr 19, 2024
8fe2bb9
fix useless asref
JunkuiZhang Apr 19, 2024
ef029fd
update ui_text_field
JunkuiZhang Apr 19, 2024
5b71fe6
special case for empty features
JunkuiZhang Apr 23, 2024
0d14db4
Merge branch 'main' into font-features
mikayla-maki Apr 26, 2024
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
2 changes: 1 addition & 1 deletion crates/assistant/src/assistant_panel.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2873,7 +2873,7 @@ impl InlineAssistant {
cx.theme().colors().text
},
font_family: settings.ui_font.family.clone(),
font_features: settings.ui_font.features,
font_features: settings.ui_font.features.clone(),
font_size: rems(0.875).into(),
font_weight: FontWeight::NORMAL,
font_style: FontStyle::Normal,
Expand Down
2 changes: 1 addition & 1 deletion crates/assistant/src/completion_provider/open_ai.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -241,7 +241,7 @@ impl AuthenticationPrompt {
let text_style = TextStyle {
color: cx.theme().colors().text,
font_family: settings.ui_font.family.clone(),
font_features: settings.ui_font.features,
font_features: settings.ui_font.features.clone(),
font_size: rems(0.875).into(),
font_weight: FontWeight::NORMAL,
font_style: FontStyle::Normal,
Expand Down
2 changes: 1 addition & 1 deletion crates/collab_ui/src/chat_panel/message_editor.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -522,7 +522,7 @@ impl Render for MessageEditor {
cx.theme().colors().text
},
font_family: settings.ui_font.family.clone(),
font_features: settings.ui_font.features,
font_features: settings.ui_font.features.clone(),
font_size: TextSize::Small.rems(cx).into(),
font_weight: FontWeight::NORMAL,
font_style: FontStyle::Normal,
Expand Down
2 changes: 1 addition & 1 deletion crates/collab_ui/src/collab_panel.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2171,7 +2171,7 @@ impl CollabPanel {
cx.theme().colors().text
},
font_family: settings.ui_font.family.clone(),
font_features: settings.ui_font.features,
font_features: settings.ui_font.features.clone(),
font_size: rems(0.875).into(),
font_weight: FontWeight::NORMAL,
font_style: FontStyle::Normal,
Expand Down
6 changes: 3 additions & 3 deletions crates/editor/src/editor.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10340,7 +10340,7 @@ impl Render for Editor {
EditorMode::SingleLine | EditorMode::AutoHeight { .. } => TextStyle {
color: cx.theme().colors().editor_foreground,
font_family: settings.ui_font.family.clone(),
font_features: settings.ui_font.features,
font_features: settings.ui_font.features.clone(),
font_size: rems(0.875).into(),
font_weight: FontWeight::NORMAL,
font_style: FontStyle::Normal,
Expand All @@ -10353,7 +10353,7 @@ impl Render for Editor {
EditorMode::Full => TextStyle {
color: cx.theme().colors().editor_foreground,
font_family: settings.buffer_font.family.clone(),
font_features: settings.buffer_font.features,
font_features: settings.buffer_font.features.clone(),
font_size: settings.buffer_font_size(cx).into(),
font_weight: FontWeight::NORMAL,
font_style: FontStyle::Normal,
Expand Down Expand Up @@ -10778,7 +10778,7 @@ pub fn diagnostic_block_renderer(diagnostic: Diagnostic, _is_valid: bool) -> Ren
let theme_settings = ThemeSettings::get_global(cx);
text_style.font_family = theme_settings.buffer_font.family.clone();
text_style.font_style = theme_settings.buffer_font.style;
text_style.font_features = theme_settings.buffer_font.features;
text_style.font_features = theme_settings.buffer_font.features.clone();
text_style.font_weight = theme_settings.buffer_font.weight;

let multi_line_diagnostic = diagnostic.message.contains('\n');
Expand Down
2 changes: 1 addition & 1 deletion crates/extensions_ui/src/extensions_ui.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -739,7 +739,7 @@ impl ExtensionsPage {
cx.theme().colors().text
},
font_family: settings.ui_font.family.clone(),
font_features: settings.ui_font.features,
font_features: settings.ui_font.features.clone(),
font_size: rems(0.875).into(),
font_weight: FontWeight::NORMAL,
font_style: FontStyle::Normal,
Expand Down
4 changes: 2 additions & 2 deletions crates/gpui/src/platform/cosmic_text/text_system.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -90,7 +90,7 @@ impl PlatformTextSystem for CosmicTextSystem {
let candidates = if let Some(font_ids) = state.font_ids_by_family_cache.get(&font.family) {
font_ids.as_slice()
} else {
let font_ids = state.load_family(&font.family, font.features)?;
let font_ids = state.load_family(&font.family, &font.features)?;
state
.font_ids_by_family_cache
.insert(font.family.clone(), font_ids);
Expand Down Expand Up @@ -211,7 +211,7 @@ impl CosmicTextSystemState {
fn load_family(
&mut self,
name: &str,
_features: FontFeatures,
_features: &FontFeatures,
) -> Result<SmallVec<[FontId; 4]>> {
// TODO: Determine the proper system UI font.
let name = if name == ".SystemUIFont" {
Expand Down
2 changes: 1 addition & 1 deletion crates/gpui/src/platform/mac/open_type.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -107,7 +107,7 @@ const kTypographicExtrasType: i32 = 14;
const kVerticalFractionsSelector: i32 = 1;
const kVerticalPositionType: i32 = 10;

pub fn apply_features(font: &mut Font, features: FontFeatures) {
pub fn apply_features(font: &mut Font, features: &FontFeatures) {
// See https://chromium.googlesource.com/chromium/src/+/66.0.3359.158/third_party/harfbuzz-ng/src/hb-coretext.cc
// for a reference implementation.
toggle_open_type_feature(
Expand Down
10 changes: 7 additions & 3 deletions crates/gpui/src/platform/mac/text_system.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -123,12 +123,12 @@ impl PlatformTextSystem for MacTextSystem {
let mut lock = RwLockUpgradableReadGuard::upgrade(lock);
let font_key = FontKey {
font_family: font.family.clone(),
font_features: font.features,
font_features: font.features.clone(),
};
let candidates = if let Some(font_ids) = lock.font_ids_by_font_key.get(&font_key) {
font_ids.as_slice()
} else {
let font_ids = lock.load_family(&font.family, font.features)?;
let font_ids = lock.load_family(&font.family, &font.features)?;
lock.font_ids_by_font_key.insert(font_key.clone(), font_ids);
lock.font_ids_by_font_key[&font_key].as_ref()
};
Expand Down Expand Up @@ -219,7 +219,11 @@ impl MacTextSystemState {
Ok(())
}

fn load_family(&mut self, name: &str, features: FontFeatures) -> Result<SmallVec<[FontId; 4]>> {
fn load_family(
&mut self,
name: &str,
features: &FontFeatures,
) -> Result<SmallVec<[FontId; 4]>> {
let name = if name == ".SystemUIFont" {
".AppleSystemUIFont"
} else {
Expand Down
2 changes: 1 addition & 1 deletion crates/gpui/src/style.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -262,7 +262,7 @@ impl TextStyle {
pub fn font(&self) -> Font {
Font {
family: self.font_family.clone(),
features: self.font_features,
features: self.font_features.clone(),
weight: self.font_weight,
style: self.font_style,
}
Expand Down
86 changes: 85 additions & 1 deletion crates/gpui/src/text_system/font_features.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,7 @@
#[cfg(target_os = "windows")]
use crate::SharedString;
#[cfg(target_os = "windows")]
use itertools::Itertools;
use schemars::{
schema::{InstanceType, Schema, SchemaObject, SingleOrVec},
JsonSchema,
Expand All @@ -7,10 +11,14 @@ macro_rules! create_definitions {
($($(#[$meta:meta])* ($name:ident, $idx:expr)),* $(,)?) => {

/// The OpenType features that can be configured for a given font.
#[derive(Default, Copy, Clone, Eq, PartialEq, Hash)]
#[derive(Default, Clone, Eq, PartialEq, Hash)]
pub struct FontFeatures {
enabled: u64,
disabled: u64,
#[cfg(target_os = "windows")]
other_enabled: SharedString,
Copy link
Contributor

Choose a reason for hiding this comment

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

Why are these additional fields needed?

Copy link
Contributor Author

@JunkuiZhang JunkuiZhang Apr 19, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

The other_enabled and other_disabled serve the same function as Vec<FeatureName>. Currently, Zed only supports 34 OpenType features from calt to zero, stored in enabled: u64 and disabled: u64 as bitfields, allowing for a maximum of 64 features. In theory, there can be up to 36*36*36*36 OpenType features. For instance, Firacode supports features like cv01 to cv30, while some special fonts may even use cv99 for certain reasons. Therefore, to support all OpenType features, the current FontFeatures struct would need to introduce a structure similar to Vec<T>, which would not support Copy. Cloning operations with Vec<T> are relatively costly, so SharedString is used here since its clone operation simply duplicates the pointer.

This change might have some impact on the performance of macOS, but I guess it shouldn't be significant. In macOS, FontFeatures has only two members of type u64, so the performance gap between clone and copy should be minimal. Alternatively, I could separate the implementation of FontFeatures for macOS and Windows without altering the existing macOS code.

Copy link
Contributor

@mikayla-maki mikayla-maki Apr 22, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

If this is a correctness issue, where both implementation are wrong, then we shouldn't be hiding this behind a #[cfg] and should enable it for everything. As for an efficient representation, I think this packed representation makes sense. Maybe let's document the other_enabled / other_disabled format (e.g. 'a list of 4 character font feature values'). I think I'd want to use something like a ArcCow<'static, [char; 4]>, so that it's super explicit what's being stored, but I expect that's a lot more trouble to work with and it will be even less efficient than a str representation, as the initial ascii characters won't be packed into a single u8.

That said, I expect the common case is that there are no additional font features? For that case, I think it would be nice to special case the into call on 165. If the strings are empty, we can call "".into() to get a SharedString wrapping a static reference. Cloning that should be even cheaper than the Arc clone, and should mean there's next to no performance hit.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If this is a correctness issue, where both implementation are wrong, then we shouldn't be hiding this behind a #[cfg] and should enable it for everything. As for an efficient representation, I think this packed representation makes sense. Maybe let's document the other_enabled / other_disabled format (e.g. 'a list of 4 character font feature values'). I think I'd want to use something like a ArcCow<'static, [char; 4]>, so that it's super explicit what's being stored, but I expect that's a lot more trouble to work with and it will be even less efficient than a str representation, as the initial ascii characters won't be packed into a single u8.

I'm sorry, I didn't fully follow you. Here, it's not just a correctness issue; currently, Zed only supports a subset of font features. The font features that Zed doesn't support but are supported by DirectWrite (cv99 for example) are stored here.

Assuming the user has the following settings:

  "buffer_font_features": {
      "ss01": true,    <-- stored in `enabled` as bitfield
      "ss02": true,    ...
      "ss03": true,    ...
      "ss04": true,    ...
      "ss05": true,    ...
      "cv01": true,    <-- stored in `other_enabled`
      "cv02": true,
      "cv03": true,
      "cv04": true,
    }

The FontFeature:

other_enabled: "cv01cv02cv03cv04"
other_disabled: ""

That said, I expect the common case is that there are no additional font features? For that case, I think it would be nice to special case the into call on 165. If the strings are empty, we can call "".into() to get a SharedString wrapping a static reference. Cloning that should be even cheaper than the Arc clone, and should mean there's next to no performance hit.

This is a nice idea.

Copy link
Contributor

Choose a reason for hiding this comment

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

What I mean is:

  • can you have more than 34 open type features on macOS or Linux?
  • If yes, then we should enable this for all platforms
  • If no, then we should leave this enabled just for windows.

Looking at wikipedia, it seems likely that OpenType can have more than 34 features on all platforms. As such, we should enable this change for all platforms.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My idea is that the current PR only provides implementations for additional font features on the Windows platform, without addressing other platforms. Therefore, in this PR, minimize the impact, I only enabling it for Windows for now. If other platforms implement support in the future, we can then remove the corresponding cfg tags.

To be honest, Linux should be able to support other font features, but I'm unsure about macOS. On macOS, Core Text uses a selector mechanism to enable specific font features. However, as I mentioned, theoretically, all 26 English letters and 10 numbers could be considered "valid" OpenType font features. For example, the Iosevka font uses VS** and VL**, which are not listed in wiki. It's challenging to toggle these font features on macOS.

Copy link
Contributor

Choose a reason for hiding this comment

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

I see, so you're saying it takes more work to fully utilize these on other platforms? That makes sense then. I'll merge this then :)

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 see, so you're saying it takes more work to fully utilize these on other platforms? That makes sense then. I'll merge this then :)

Especially on macOS, I couldn't find the selector for toggling these cv** font features like cv01, but almost every font is utilizing cv**.

#[cfg(target_os = "windows")]
other_disabled: SharedString,
}

impl FontFeatures {
Expand Down Expand Up @@ -47,6 +55,14 @@ macro_rules! create_definitions {
}
}
)*
{
for name in self.other_enabled.as_ref().chars().chunks(4).into_iter() {
result.push((name.collect::<String>(), true));
}
for name in self.other_disabled.as_ref().chars().chunks(4).into_iter() {
result.push((name.collect::<String>(), false));
}
}
result
}
}
Expand All @@ -59,6 +75,15 @@ macro_rules! create_definitions {
debug.field(stringify!($name), &value);
};
)*
#[cfg(target_os = "windows")]
{
for name in self.other_enabled.as_ref().chars().chunks(4).into_iter() {
debug.field(name.collect::<String>().as_str(), &true);
}
for name in self.other_disabled.as_ref().chars().chunks(4).into_iter() {
debug.field(name.collect::<String>().as_str(), &false);
}
}
debug.finish()
}
}
Expand All @@ -80,6 +105,7 @@ macro_rules! create_definitions {
formatter.write_str("a map of font features")
}

#[cfg(not(target_os = "windows"))]
fn visit_map<M>(self, mut access: M) -> Result<Self::Value, M::Error>
where
M: MapAccess<'de>,
Expand All @@ -100,6 +126,54 @@ macro_rules! create_definitions {
}
Ok(FontFeatures { enabled, disabled })
}

#[cfg(target_os = "windows")]
fn visit_map<M>(self, mut access: M) -> Result<Self::Value, M::Error>
where
M: MapAccess<'de>,
{
let mut enabled: u64 = 0;
let mut disabled: u64 = 0;
let mut other_enabled = "".to_owned();
let mut other_disabled = "".to_owned();

while let Some((key, value)) = access.next_entry::<String, Option<bool>>()? {
let idx = match key.as_str() {
$(stringify!($name) => Some($idx),)*
other_feature => {
if other_feature.len() != 4 || !other_feature.is_ascii() {
log::error!("Incorrect feature name: {}", other_feature);
continue;
}
None
},
};
if let Some(idx) = idx {
match value {
Some(true) => enabled |= 1 << idx,
Some(false) => disabled |= 1 << idx,
None => {}
};
} else {
match value {
Some(true) => other_enabled.push_str(key.as_str()),
Some(false) => other_disabled.push_str(key.as_str()),
None => {}
};
}
}
let other_enabled = if other_enabled.is_empty() {
"".into()
} else {
other_enabled.into()
};
let other_disabled = if other_disabled.is_empty() {
"".into()
} else {
other_disabled.into()
};
Ok(FontFeatures { enabled, disabled, other_enabled, other_disabled })
}
}

let features = deserializer.deserialize_map(FontFeaturesVisitor)?;
Expand All @@ -125,6 +199,16 @@ macro_rules! create_definitions {
}
)*

#[cfg(target_os = "windows")]
{
for name in self.other_enabled.as_ref().chars().chunks(4).into_iter() {
map.serialize_entry(name.collect::<String>().as_str(), &true)?;
}
for name in self.other_disabled.as_ref().chars().chunks(4).into_iter() {
map.serialize_entry(name.collect::<String>().as_str(), &false)?;
}
}

map.end()
}
}
Expand Down
2 changes: 1 addition & 1 deletion crates/outline/src/outline.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -274,7 +274,7 @@ impl PickerDelegate for OutlineViewDelegate {
let text_style = TextStyle {
color: cx.theme().colors().text,
font_family: settings.buffer_font.family.clone(),
font_features: settings.buffer_font.features,
font_features: settings.buffer_font.features.clone(),
font_size: settings.buffer_font_size(cx).into(),
font_weight: FontWeight::NORMAL,
font_style: FontStyle::Normal,
Expand Down
2 changes: 1 addition & 1 deletion crates/search/src/buffer_search.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -114,7 +114,7 @@ impl BufferSearchBar {
color
},
font_family: settings.buffer_font.family.clone(),
font_features: settings.buffer_font.features,
font_features: settings.buffer_font.features.clone(),
font_size: rems(0.875).into(),
font_weight: FontWeight::NORMAL,
font_style: FontStyle::Normal,
Expand Down
2 changes: 1 addition & 1 deletion crates/search/src/project_search.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1307,7 +1307,7 @@ impl ProjectSearchBar {
cx.theme().colors().text
},
font_family: settings.buffer_font.family.clone(),
font_features: settings.buffer_font.features,
font_features: settings.buffer_font.features.clone(),
font_size: rems(0.875).into(),
font_weight: FontWeight::NORMAL,
font_style: FontStyle::Normal,
Expand Down
3 changes: 2 additions & 1 deletion crates/terminal_view/src/terminal_element.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -578,7 +578,8 @@ impl Element for TerminalElement {

let font_features = terminal_settings
.font_features
.unwrap_or(settings.buffer_font.features);
.clone()
.unwrap_or(settings.buffer_font.features.clone());

let line_height = terminal_settings.line_height.value();
let font_size = terminal_settings.font_size;
Expand Down
8 changes: 4 additions & 4 deletions crates/theme/src/settings.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -325,13 +325,13 @@ impl settings::Settings for ThemeSettings {
ui_font_size: defaults.ui_font_size.unwrap().into(),
ui_font: Font {
family: defaults.ui_font_family.clone().unwrap().into(),
features: defaults.ui_font_features.unwrap(),
features: defaults.ui_font_features.clone().unwrap(),
weight: Default::default(),
style: Default::default(),
},
buffer_font: Font {
family: defaults.buffer_font_family.clone().unwrap().into(),
features: defaults.buffer_font_features.unwrap(),
features: defaults.buffer_font_features.clone().unwrap(),
weight: FontWeight::default(),
style: FontStyle::default(),
},
Expand All @@ -349,14 +349,14 @@ impl settings::Settings for ThemeSettings {
if let Some(value) = value.buffer_font_family.clone() {
this.buffer_font.family = value.into();
}
if let Some(value) = value.buffer_font_features {
if let Some(value) = value.buffer_font_features.clone() {
this.buffer_font.features = value;
}

if let Some(value) = value.ui_font_family.clone() {
this.ui_font.family = value.into();
}
if let Some(value) = value.ui_font_features {
if let Some(value) = value.ui_font_features.clone() {
this.ui_font.features = value;
}

Expand Down
2 changes: 1 addition & 1 deletion crates/ui_text_field/src/ui_text_field.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -123,7 +123,7 @@ impl Render for TextField {

let text_style = TextStyle {
font_family: settings.buffer_font.family.clone(),
font_features: settings.buffer_font.features,
font_features: settings.buffer_font.features.clone(),
font_size: rems(0.875).into(),
font_weight: FontWeight::NORMAL,
font_style: FontStyle::Normal,
Expand Down
Loading