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

[Managed DWrite] Migrate FontCollectionLoader to managed #6260

Merged
merged 1 commit into from
Feb 27, 2025
Merged

[Managed DWrite] Migrate FontCollectionLoader to managed #6260

merged 1 commit into from
Feb 27, 2025

Conversation

ThomasGoulet73
Copy link
Contributor

@ThomasGoulet73 ThomasGoulet73 commented Mar 16, 2022 •
edited
Loading

Contributes to #5305

Description

Migrate DWrite FontCollectionLoader to managed.

Customer Impact

It might be faster and should allow better support of trimming and the support of NativeAOT once everything is migrated to C#.

Regression

No.

Testing

Local build + CI + I tested a few apps that uses custom fonts to test this code path.

Risk

Low. For the most part, it is a copy of the C++ code manually rewritten to C# with as little changes as possible.

@ThomasGoulet73 ThomasGoulet73 requested a review from a team as a code owner March 16, 2022 02:41
@ghost ghost added the PR metadata: Label to tag PRs, to facilitate with triage label Mar 16, 2022
@ghost ghost requested review from dipeshmsft, singhashish-wpf and SamBent March 16, 2022 02:41
@ghost ghost added the Community Contribution A label for all community Contributions label Mar 16, 2022
@ThomasGoulet73 ThomasGoulet73 force-pushed the managed-dwrite-fontcollectionloader branch from 8213b38 to 8c595e4 Compare March 16, 2022 03:02
@ThomasGoulet73 ThomasGoulet73 force-pushed the managed-dwrite-fontcollectionloader branch from 8c595e4 to 83e9b7b Compare April 9, 2022 02:38
@ghost ghost assigned ThomasGoulet73 Apr 9, 2022
@dipeshmsft dipeshmsft self-assigned this May 5, 2022
@ThomasGoulet73 ThomasGoulet73 force-pushed the managed-dwrite-fontcollectionloader branch from 83e9b7b to 9096f6b Compare May 11, 2022 00:36
kant2002 added a commit to kant2002/wpf that referenced this pull request Jul 5, 2022
@kant2002
Continuation of DWriteForwarder replacement
451afd8 
Based on foundation layed in the dotnet#6260
@ThomasGoulet73 ThomasGoulet73 mentioned this pull request Nov 17, 2022
Closed
@ThomasGoulet73 ThomasGoulet73 force-pushed the managed-dwrite-fontcollectionloader branch from 86f055b to 7a8760a Compare October 27, 2023 16:20
@ThomasGoulet73
Copy link
Contributor Author

I rebased on main now that #6171 is merged.

@ThomasGoulet73 ThomasGoulet73 force-pushed the managed-dwrite-fontcollectionloader branch from 7a8760a to 4a1b753 Compare February 1, 2024 00:48
@ThomasGoulet73
Copy link
Contributor Author

I just pushed a rebase and reran my manual tests, everything seems to be working good.

@dotnet/wpf-developers: Could this be included in the next test pass ?

@ThomasGoulet73 ThomasGoulet73 force-pushed the managed-dwrite-fontcollectionloader branch from 4a1b753 to 2a4c7ba Compare August 27, 2024 00:49
@ThomasGoulet73 ThomasGoulet73 requested a review from a team as a code owner August 27, 2024 00:49
@ThomasGoulet73
Copy link
Contributor Author

I rebased to fix the conflicts.

@dipeshmsft
Copy link
Member

@ThomasGoulet73, sorry but it will take us time to get to this PR. We have started with the CTP's and will try to get to this PR as soon as possible.

@ThomasGoulet73 ThomasGoulet73 mentioned this pull request Oct 30, 2024
Open
JeremyKuhne
JeremyKuhne requested changes Oct 30, 2024
View reviewed changes
Copy link
Member

@JeremyKuhne JeremyKuhne left a comment

Choose a reason for hiding this comment

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

A few comments. The default constructor comment is the one I'm most concerned about.

@dotnet-policy-service dotnet-policy-service bot added the 📭 waiting-author-feedback To request more information from author. label Oct 30, 2024
@ThomasGoulet73
Copy link
Contributor Author

Thanks for the review @JeremyKuhne. My migration process is to stay as close as possible to the C++/CLI code (If the code is not wrong) and, if it's alright with you, I'd prefer to do that to avoid potential mistakes. Your suggestions are most likely correct and cleaner but I'm trying to avoid potential regression and to keep a clean Git history (Have the first commit of migrated files be as close as possible to the original and further cleanup later).

So if you agree, I don't think your comments are blocking and it can be done later on (I already planned on doing some cleanup in the future once when most or all the code is migrated). I'm open to doing your suggestions in this PR if that's what you prefer.

@dotnet-policy-service dotnet-policy-service bot removed the 📭 waiting-author-feedback To request more information from author. label Nov 8, 2024
@lindexi
Copy link
Member

lindexi commented Nov 8, 2024

I agree with @ThomasGoulet73 . This PR has been around for a long time and it's even more important to get it merged as soon as possible.

JeremyKuhne
JeremyKuhne approved these changes Nov 8, 2024
View reviewed changes
Copy link
Member

@JeremyKuhne JeremyKuhne left a comment

Choose a reason for hiding this comment

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

Sorry, missed the fact that the assert was in the header, not the .cpp file. I have no concerns transliterating, just want to point out the weird so it is understood.

@ThomasGoulet73 ThomasGoulet73 force-pushed the managed-dwrite-fontcollectionloader branch from 2a4c7ba to f377cd2 Compare January 24, 2025 00:04
@ThomasGoulet73
Copy link
Contributor Author

I rebased on main to fix the conflicts.

@Kuldeep-MS Kuldeep-MS merged commit 344dd14 into dotnet:main Feb 27, 2025
8 checks passed
@lindexi
Copy link
Member

lindexi commented Feb 27, 2025

Awesome!

@h3xds1nz
Copy link
Member

Great, happy to see this in! Now just the rest of the DW forwarder 😀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@JeremyKuhne JeremyKuhne JeremyKuhne approved these changes

@Kuldeep-MS Kuldeep-MS Kuldeep-MS approved these changes

@dipeshmsft dipeshmsft Awaiting requested review from dipeshmsft

@singhashish-wpf singhashish-wpf Awaiting requested review from singhashish-wpf

@SamBent SamBent Awaiting requested review from SamBent

Assignees

@ThomasGoulet73 ThomasGoulet73

@dipeshmsft dipeshmsft

Labels
Community Contribution A label for all community Contributions PR metadata: Label to tag PRs, to facilitate with triage Queue for test pass Status:Proposed
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@ThomasGoulet73 @dipeshmsft @lindexi @h3xds1nz @JeremyKuhne @Kuldeep-MS @himgoyalmicro