-
Notifications
You must be signed in to change notification settings - Fork 2k
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.
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
windows: Support all OpenType
font features
#10756
Conversation
pub struct FontFeatures { | ||
enabled: u64, | ||
disabled: u64, | ||
#[cfg(target_os = "windows")] | ||
other_enabled: SharedString, |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 aArcCow<'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 astr
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 :)
There was a problem hiding this comment.
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**
.
718c416
to
5b71fe6
Compare
This PR brings support for all `OpenType` font features to `macOS(v10.10+)`. Now, both `Windows`(with #10756 ) and `macOS` support all font features. Due to my limited familiarity with the APIs on macOS, I believe I have made sure to call `CFRelease` on all variables where it should be called. Close #11486 , and I think the official website's [documentation](https://zed.dev/docs/configuring-zed) can be updated after merging this PR. > Zed supports a subset of OpenType features that can be enabled or disabled for a given buffer or terminal font. The following OpenType features can be enabled or disabled too: calt, case, cpsp, frac, liga, onum, ordn, pnum, ss01, ss02, ss03, ss04, ss05, ss06, ss07, ss08, ss09, ss10, ss11, ss12, ss13, ss14, ss15, ss16, ss17, ss18, ss19, ss20, subs, sups, swsh, titl, tnum, zero. https://github.com/zed-industries/zed/assets/14981363/44e503f9-1496-4746-bc7d-20878c6f8a93 Release Notes: - Added support for **all** `OpenType` font features to macOS.
This PR brings support for all `OpenType` font features to `macOS(v10.10+)`. Now, both `Windows`(with zed-industries#10756 ) and `macOS` support all font features. Due to my limited familiarity with the APIs on macOS, I believe I have made sure to call `CFRelease` on all variables where it should be called. Close zed-industries#11486 , and I think the official website's [documentation](https://zed.dev/docs/configuring-zed) can be updated after merging this PR. > Zed supports a subset of OpenType features that can be enabled or disabled for a given buffer or terminal font. The following OpenType features can be enabled or disabled too: calt, case, cpsp, frac, liga, onum, ordn, pnum, ss01, ss02, ss03, ss04, ss05, ss06, ss07, ss08, ss09, ss10, ss11, ss12, ss13, ss14, ss15, ss16, ss17, ss18, ss19, ss20, subs, sups, swsh, titl, tnum, zero. https://github.com/zed-industries/zed/assets/14981363/44e503f9-1496-4746-bc7d-20878c6f8a93 Release Notes: - Added support for **all** `OpenType` font features to macOS.
Release Notes:
OpenType
font features to DirectWrite.Recording.2024-04-19.104731.mp4