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.

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

Update Source Code Pro and include italics #65665

Merged
merged 1 commit into from
Nov 20, 2019

Conversation

tspiteri
Copy link
Contributor

Fixes #65502.

A few notes:

@hellow554
Copy link
Contributor

hellow554 commented Oct 22, 2019

As stated in #65502, this does increase the download size.

Can you say how much it will increase?

@tspiteri
Copy link
Contributor Author

Reproducing the table in the issue below. Updating the upright fonts to the latest version costs an extra 79 KiB, and including the italics costs 82 KiB, total increase is 161 KiB. I believe this shouldn't be such an issue, especially with caching.

Font Current Size Latest Size
Regular 55472 95872
Semibold 55360 95576
Italic   84264
Total 110832 275712

@JohnCSimon JohnCSimon added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Oct 26, 2019
@tspiteri
Copy link
Contributor Author

@JohnCSimon I think you got the wrong label.

@Dylan-DPC-zz Dylan-DPC-zz added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Nov 2, 2019
@JohnCSimon
Copy link
Member

Ping from triage
@QuietMisdreavus can you please review this pr?
cc: @tspiteri
Thanks

@Dylan-DPC-zz
Copy link

r? @GuillaumeGomez

@GuillaumeGomez
Copy link
Member

So we're doubling the size of the font to add italic if I understood correctly?

@tspiteri
Copy link
Contributor Author

Basically adding about 75% current size to add italic, and this PR also adds another 75% to update the upright and bold to the latest version, so it's a 150% increase in total (from 110 kb to 275 kb).

@GuillaumeGomez
Copy link
Member

I'm not sure if it's worth it... Any additional opinion @rust-lang/rustdoc ?

@joelpalmer
Copy link

Ping from Triage: Any updates @rust-lang/rustdoc @GuillaumeGomez @tspiteri?

@ollie27
Copy link
Member

ollie27 commented Nov 19, 2019

I think we should merge this. If we're going to ship custom fonts they should at least be up to date and complete.

@GuillaumeGomez
Copy link
Member

Fine by me then. Thanks @tspiteri !

@bors: r+ rollup

@bors
Copy link
Contributor

bors commented Nov 20, 2019

📌 Commit ea9519b has been approved by GuillaumeGomez

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Nov 20, 2019
Centril added a commit to Centril/rust that referenced this pull request Nov 20, 2019
…dePro, r=GuillaumeGomez

Update Source Code Pro and include italics

Fixes rust-lang#65502.

A few notes:
  * As stated in rust-lang#65502, this does increase the download size.
  * Since this PR changes the font set, I think docs.rs would have to be updated if this PR is merged.
  * The fonts have a double extension (.ttf.woff); this is to keep the names consistent with the upstream font release which does that to distinguish these from the .otf.woff files ([Source Code Pro otf renders poorly on older Windows system apps](adobe-fonts/source-code-pro#25 (comment))).
@tspiteri
Copy link
Contributor Author

@GuillaumeGomez Great! When this is merged I think docs.rs has to be updated, though I couldn't figure out how that works in rust-lang/docs.rs#270.

bors added a commit that referenced this pull request Nov 20, 2019
Rollup of 8 pull requests

Successful merges:

 - #65665 (Update Source Code Pro and include italics)
 - #66478 (rustc_plugin: Remove the compatibility shim)
 - #66497 (Fix #53820)
 - #66526 (Add more context to `async fn` trait error)
 - #66532 (Generate DWARF address ranges for faster lookups)
 - #66546 (Remove duplicate function)
 - #66548 ([RISCV] Disable Atomics on all Non-A RISC-V targets)
 - #66553 (remove HermitCore leftovers from sys/unix)

Failed merges:

r? @ghost
@GuillaumeGomez
Copy link
Member

Don't worry, let them handle their part. ;)

cc @pietroalbini

@bors bors merged commit ea9519b into rust-lang:master Nov 20, 2019
@tspiteri tspiteri deleted the italic-and-update-SourceCodePro branch November 20, 2019 21:14
@ehuss
Copy link
Contributor

ehuss commented Nov 21, 2019

This broke font rendering on firefox on macos with the dark theme. See #60365 (comment) for details. Can we maybe have a different solution?

image

@tspiteri
Copy link
Contributor Author

Aargh!

@GuillaumeGomez Should I open a PR to revert this PR?

@GuillaumeGomez
Copy link
Member

Yes please.

@GuillaumeGomez
Copy link
Member

Considering it's broken, I already open the PR to revert it here.

@tspiteri tspiteri restored the italic-and-update-SourceCodePro branch November 21, 2019 10:45
@tspiteri

This comment has been minimized.

JohnTitor added a commit to JohnTitor/rust that referenced this pull request Mar 23, 2021
…dePro, r=GuillaumeGomez

Update Source Code Pro and include italics

Fixes rust-lang#65502.

rust-lang#65665, a similar PR to this was merged but reverted because of rust-lang#65665 (comment).

The issue in that comment is the upstream issue adobe-fonts/source-code-pro#217 which should now be fixed in the upstream since [2.032R-ro/1.052R-it/1.012R-VAR release](https://github.com/adobe-fonts/source-code-pro/releases/tag/2.032R-ro/1.052R-it/1.012R-VAR), so I think this can now be merged.

A couple of notes from the original PR:
* Since this PR changes the font set, I think docs.rs would have to be updated if this PR is merged.
* The fonts have a double extension (.ttf.woff); this is to keep the names consistent with the upstream font release which does that to distinguish these from the .otf.woff files (Source Code Pro otf renders poorly on older Windows system apps).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Rustdoc uses fake italics for code
10 participants