-
Notifications
You must be signed in to change notification settings - Fork 323
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
Use Enso Font #7516
Use Enso Font #7516
Conversation
Moving toward the new design: - New font replaces DejaVu Sans Mono. - Change bolded SpanWidgets to extra-bolded.
Use new font in: - Docs - Error visualizations
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.
on the JS side it's mostly things being deleted, so no added code that needs to be reviewed
build/build/src/ide/web/download.rs
Outdated
#[derive(Debug)] | ||
struct FixedContentUrl<D> { | ||
url: Url, | ||
filename: Box<Path>, |
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.
Out of curiosity, why Box<Path>
is used everywhere instead of PathBuf
?
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.
It's a more specific type. PathBuf
supports all the operations of Box<Path>
, and additional operations. The additional operations offered by PathBuf
are not appropriate for this value--it's the file part of a path, so e.g. it makes no sense to append anything to it. This is not a very important distinction in this case, but it also doesn't cost us anything here to be precise.
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 code is heavily duplicated. I think we could just extend ensogl-text-font-family
crate and use it in the build script and enso-font crate.
build/build/src/ide/web/download.rs
Outdated
/// The `filename` parameter may be any path-safe string; it will only be seen when inspecting the | ||
/// cache. | ||
pub async fn get_file_from_cache_or_download( |
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.
Do you mean that filename
will be used just as Cache key? This is how I've understood this, but I'm not 100% sure.
And what "path-safe" means? I deduce from the code below, that it rather should not be an absolute path.
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.
Clarified the docs.
@@ -0,0 +1,11 @@ | |||
[package] | |||
name = "enso-enso-font" |
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.
This name is a bit amusing :) why not just "enso-font"?
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.
Yeah, it sounds funny, but it is to avoid confusion: Since we prefix everything with enso-
, enso-font
would be what we call the Enso package for fonts (and actually after refactoring, that name is used). This is the Enso package for the Enso font.
build/build/src/ide/web/download.rs
Outdated
@@ -0,0 +1,105 @@ | |||
//! Support for downloading files. |
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 believe this effectively duplicates build/ci_utils/src/cache/download.rs
, including the use-case of using octocrab to download the file?
I think these should be unified and be part of ide_ci::cache::download
.
The Downloader
is neat. I don't like the assumption of contents being constant by URL (as my use-case is a counterexample) — but perhaps the Key
type could be provided by the Downloader
as well?
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.
Oh, I missed that module. Rewrote it to use the existing API.
QA Report: 🔴 Everything seems fine except for one issue. The cursor position does not match the actual place where the new characters are added/removed. This is not present on the most recent develop. Kapture.2023-08-14.at.11.52.26.mp4 |
Keziah Wesley reports a new STANDUP for yesterday (2023-08-14): Progress: Debugging an issue with cursor/selection logic. The PR does not change this logic, but I think the bug is exposed by using a non-monospace font for editable text. It should be finished by 2023-08-16. Next Day: Next day I will be working on the #7516 task. Find the bug. |
Keziah Wesley reports a new STANDUP for yesterday (2023-08-15): Progress: Debugging wrong glyph counts. It should be finished by 2023-08-16. Next Day: Next day I will be working on the #7516 task. Find the bug. |
The PR did not pass QA, so it can't be merged yet. I will check now. |
QA passed 📗 |
Pull Request Description
Use the new Enso Font; also change the anti-aliasing logic to be based on device pixel ratio, rather than platform. This will improve the clarity of font rendering on Windows/Linux machines with high pixel densities.
Design reference:
Tested on various combinations of DPR/platform:
OS X,
devicePixelRatio
= 2 (should look similar to how we were already rendering mplus1 on OS X):Windows,
devicePixelRatio
= 1.25 (should look similar to how we were already rendering mplus1 on this platform/DPR):Linux,
devicePixelRatio
= 1 (should look similar to how we were already rendering mplus1 on this platform/DPR):Important Notes
Style changes:
Implementation improvements:
encountered in the past.
Checklist
Please ensure that the following checklist has been satisfied before submitting the PR:
Scala,
Java,
and
Rust
style guides. In case you are using a language not listed above, follow the Rust style guide.
./run ide build
.