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

[Managed DWrite] Migrate part of DWriteTypeConverter to managed #9902

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

ThomasGoulet73
Copy link
Contributor

@ThomasGoulet73 ThomasGoulet73 commented Oct 5, 2024

Contributes to #5305

Description

Migrate part of DWriteTypeConverter to managed. I also removed some dead code from my previous Managed DWrite PR. I changed the name of DWriteTypeConverter in PresentationCore to DWriteTypeConverterEx because it conflicted with the one in DirectWriteForwarder.

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 testing.

Risk

Low.

Microsoft Reviewers: Open in CodeFlow

JeremyKuhne
JeremyKuhne previously approved these changes Oct 29, 2024
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.

Looks good to me.

Hopefully in a few weeks we'll have CsWin32 and xunit support in place which will make changes like this possible without pre-emptive test passes.

// We do not support Natural Metrics mode in WPF
// However, the build system complained about not having an explicit case
// for DWRITE_TEXT_MEASURING_METHOD_USE_DISPLAY_NATURAL_METRICS
case DWRITE_MEASURING_MODE.DWRITE_MEASURING_MODE_GDI_NATURAL:
Copy link
Member

Choose a reason for hiding this comment

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

nit: slightly better to fall through here and only have one throw.

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 wanted to keep the case for the comment but I didn't think about just merging case DWRITE_MEASURING_MODE.DWRITE_MEASURING_MODE_GDI_NATURAL with default. I pushed a commit the just merges the 2 and removes the duplicate throw.

@ThomasGoulet73
Copy link
Contributor Author

@JeremyKuhne Do you mind reviewing #6260 as well ? It's in the same vein.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Community Contribution A label for all community Contributions Included in test pass PR metadata: Label to tag PRs, to facilitate with triage Status:Completed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants