Skip to content
This repository has been archived by the owner on Jan 23, 2023. It is now read-only.
/ corefx Public archive

Make Font serializable #33445

Merged
merged 1 commit into from
Nov 13, 2018
Merged

Make Font serializable #33445

merged 1 commit into from
Nov 13, 2018

Conversation

ViktorHofer
Copy link
Member

@ViktorHofer ViktorHofer commented Nov 12, 2018 •
edited
Loading

Fixes https://github.com/dotnet/corefx/issues/33437

⚡️ I HEREBY DEMAND THE NEWLY ADDED SERIALIZATION TESTS JUST PASS ⚡️ please

@ViktorHofer ViktorHofer added this to the 3.0 milestone Nov 12, 2018
@ViktorHofer ViktorHofer self-assigned this Nov 12, 2018
@ViktorHofer
Copy link
Member Author

@dotnet-bot test NETFX x86 Release Build (infra issue)

safern
safern approved these changes Nov 12, 2018
View reviewed changes
Copy link
Member

@safern safern left a comment

Choose a reason for hiding this comment

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

LGTM!

danmoseley
danmoseley approved these changes Nov 12, 2018
View reviewed changes
Copy link
Member

@danmoseley danmoseley left a comment

Choose a reason for hiding this comment

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

Surprised I missed this one.

@ViktorHofer
Copy link
Member Author

Hit some method name differences between Windows and Unix. We should reconcile the unnecessary code path differences at some point.

ViktorHofer
ViktorHofer commented Nov 13, 2018
View reviewed changes
src/System.Drawing.Common/src/System/Drawing/Font.Unix.cs Outdated
@@ -216,14 +216,14 @@ internal Font(IntPtr nativeFont, string familyName, FontStyle style, float size)
fontFamily = FontFamily.GenericSansSerif;
}

setProperties(fontFamily, size, style, GraphicsUnit.Pixel, 0, false);
Initialize(fontFamily, size, style, GraphicsUnit.Pixel, 0, false);
Copy link
Member Author

Choose a reason for hiding this comment

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

I don't know much about this codebase but shouldn't the last parameter be IsVerticalName(fontFamily.Name) instead of false? cc @safern

@danmoseley
Copy link
Member

System/Drawing/Font.Unix.cs(56,66): error CS0103: The name 'family' does not exist in the current context [/Users/dotnet-bot/j/workspace/dotnet_corefx/master/osx-TGroup_netcoreapp+CGroup_Debug+AGroup_x64+TestOuter_false_prtest/src/System.Drawing.Common/src/System.Drawing.Common.csproj]

@danmoseley
Copy link
Member

danmoseley commented Nov 13, 2018 •
edited
Loading

You gotta build Linux locally 😛

@ViktorHofer ViktorHofer merged commit 6e55e0f into dotnet:master Nov 13, 2018
@ViktorHofer ViktorHofer deleted the FontSerialization branch November 13, 2018 14:39
@clairernovotny
Copy link
Member

Is there/should there be an issue for making RuntimeType serializable? You can't currently serialize typeof(SomeType) or an Assembly instance either (typeof(Foo).Assembly)

@stephentoub
Copy link
Member

Is there/should there be an issue for making RuntimeType serializable? You can't currently serialize typeof(SomeType) or an Assembly instance either (typeof(Foo).Assembly)

Many types that were serializable on netfx are not in core, by design. We're only doing so now on a case-by-case basis where there's a very strong reason for doing so and where the cons don't outweigh the benefits. If you believe there's a strong reason for additional specific types, please open separate issues for them along with the justification. Thanks.

@clairernovotny
Copy link
Member

@stephentoub okay, thanks. I don't have specific very strong reasons, but noticed failing tests when I was porting SoapFormatter to .NET Core. https://github.com/onovotny/SoapFormatter

@stephentoub
Copy link
Member

Ok. Thanks.

jlennox pushed a commit to jlennox/corefx that referenced this pull request Dec 16, 2018
@ViktorHofer @jlennox
Make Font serializable (dotnet#33445)
0a6249a
picenka21 pushed a commit to picenka21/runtime that referenced this pull request Feb 18, 2022
@ViktorHofer
Make Font serializable (dotnet/corefx#33445)
e9327f5 
Commit migrated from dotnet/corefx@6e55e0f
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@safern safern safern approved these changes

@danmoseley danmoseley danmoseley approved these changes

@stephentoub stephentoub stephentoub approved these changes

Assignees

@ViktorHofer ViktorHofer

Labels
area-System.Drawing
Projects
None yet
Milestone
3.0
Development

Successfully merging this pull request may close these issues.
5 participants
@ViktorHofer @danmoseley @clairernovotny @stephentoub @safern