Skip to content

Add attributes to System.Drawing.Printing.PrintDocument #35162

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
Merged
safern merged 2 commits into from
Jun 23, 2020
Merged

Add attributes to System.Drawing.Printing.PrintDocument #35162

safern merged 2 commits into from
Jun 23, 2020

Conversation

@eriawan
Copy link
Member

@eriawan eriawan commented Apr 18, 2020 •
edited
Loading

Fixes #34696

I added DefaultEventAttribute and DefaultPropertyAttribute on PrintDocument class in System.Drawing.Printing.PrintDocument, to be in sync with .NET Framework's PrintDocument as described in #34696

@safern
I mark this as WIP, because after I opened I System.Drawing.Common.sln and did build using Visual Studio on src\libraries\src\System.Drawing.Common.csproj on my machine, I got these errors:

Error occurred while restoring NuGet packages: Invalid restore input. Duplicate frameworks found: 'netcoreapp3.0, netcoreapp5.0, netcoreapp5.0, netcoreapp3.0'. Input files: D:\dotnetruntime\src\libraries\System.Drawing.Common\src\System.Drawing.Common.csproj.
1>------ Build started: Project: System.Drawing.Common (ref\System.Drawing.Common), Configuration: Debug Any CPU ------
1>System.Drawing.Common -> D:\dotnetruntime\artifacts\bin\System.Drawing.Common\ref\netcoreapp5.0-Debug\System.Drawing.Common.dll
2>------ Build started: Project: System.Drawing.Common (src\System.Drawing.Common), Configuration: Debug Any CPU ------
2>System.Drawing.Common -> D:\dotnetruntime\artifacts\bin\System.Drawing.Common\netcoreapp5.0-Windows_NT-Debug\System.Drawing.Common.dll
2>C:\Users\eriaw\.nuget\packages\microsoft.dotnet.apicompat\5.0.0-beta.20201.2\build\Microsoft.DotNet.ApiCompat.targets(82,5): error : CannotRemoveAttribute : Attribute 'System.ComponentModel.DefaultEventAttribute' exists on 'System.Drawing.Printing.PrintDocument' in the contract but not the implementation.
2>C:\Users\eriaw\.nuget\packages\microsoft.dotnet.apicompat\5.0.0-beta.20201.2\build\Microsoft.DotNet.ApiCompat.targets(82,5): error : CannotRemoveAttribute : Attribute 'System.ComponentModel.DefaultPropertyAttribute' exists on 'System.Drawing.Printing.PrintDocument' in the contract but not the implementation.
2>C:\Users\eriaw\.nuget\packages\microsoft.dotnet.apicompat\5.0.0-beta.20201.2\build\Microsoft.DotNet.ApiCompat.targets(96,5): error : ApiCompat failed for 'D:\GITHUB_REPO\dotnetruntime\artifacts\bin\System.Drawing.Common\netcoreapp5.0-Unix-Debug\System.Drawing.Common.dll'
2>Done building project "System.Drawing.Common.csproj" -- FAILED.
========== Build: 1 succeeded, 1 failed, 0 up-to-date, 0 skipped ==========

Is there I need to update on my side or do I miss something else?

UPDATE 1:
I think I know what I've been missing! The attributes should also be added to PrintDocument class in PrintDocument.Unix.cs as well.

I have updated my commit.

@ghost
Copy link

ghost commented Apr 18, 2020

Tagging subscribers to this area: @safern, @tannergooding
Notify danmosemsft if you want to be subscribed.

@eriawan eriawan mentioned this pull request Apr 18, 2020
Closed
@eriawan eriawan force-pushed the add_attributes_S.Drawing.Printing.PrintDocument branch from e6cb572 to 68e5a3e Compare April 18, 2020 17:43
@eriawan eriawan changed the title [WIP] Add attributes to System.Drawing.Printing.PrintDocument Add attributes to System.Drawing.Printing.PrintDocument Apr 18, 2020
@eriawan
Copy link
Member Author

eriawan commented Apr 18, 2020

@safern
The CI is green! 😄
Please review, thanks in advance.

safern
safern reviewed Apr 20, 2020
View reviewed changes
src/libraries/System.Drawing.Common/ref/System.Drawing.Common.csproj Outdated
Copy link
Member

@safern safern Apr 20, 2020

Choose a reason for hiding this comment

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

Hmm, @ericstj we've been avoiding bringing System.ComponentModel.TypeConverter closure into System.Drawing.Common, what do you suggest here, maybe pushing these two attributes down to primitives or S.ComponentModel?

Copy link
Member

@ericstj ericstj Apr 20, 2020

Choose a reason for hiding this comment

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

I believe the attribute usage is decoupled enough from the normal use case that we should be able to add the dependency and avoid loading TypeConverter, as I mentioned here: #1011 (comment)

Copy link
Member

@safern safern Apr 20, 2020

Choose a reason for hiding this comment

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

@eerhardt what level of trimming would apps need so that TypeConverter is trimmed from the closure if we only use attributes?

Copy link
Member

@eerhardt eerhardt Apr 20, 2020

Choose a reason for hiding this comment

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

They would need to pass --used-attrs-only true (which is not the default) to the linker in order to get attributes that aren't being read to be trimmed.

See https://github.com/mono/linker/blob/7573e99c7665f565fe4a6ac4784d5c641213d5e1/src/linker/Linker/Driver.cs#L960

Copy link
Member

@eerhardt eerhardt Apr 21, 2020

Choose a reason for hiding this comment

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

Note: The --used-attrs-only setting has issues with it, as called out in dotnet/linker#952 (comment). So I wouldn't expect high usage of this setting until those problems are fixed.

Copy link
Member Author

@eriawan eriawan Jun 18, 2020

Choose a reason for hiding this comment

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

@safern
Just to remind, please review my previous report about trying trim on System.Drawing.Common.
thanks before! 🙂

Copy link
Member

@safern safern Jun 18, 2020

Choose a reason for hiding this comment

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

Thanks, @eriawan.

@eerhardt @ericstj so it seems with the current change this will cause people to have System.ComponentModel.TypeConverter in their standalone apps even when publishing trimmed. Any recommendations to avoid that?

Copy link
Member Author

@eriawan eriawan Jun 23, 2020

Choose a reason for hiding this comment

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

@safern @eerhardt @ericstj
pardon, is there any update about the recommendation to avoid incllusion of System.ComponentModel.TypeConverter assembly using trimmed publish?

Copy link
Member

@ericstj ericstj Jun 23, 2020

Choose a reason for hiding this comment

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

Well, we could push the attribute types down to a lower assembly. Either System.ComponentModel.Primitives or System.ObjectModel as @safern suggested. It's still not clear to me how important this is. One thing we could do is take this as is and then if we get feedback on app size issues (eg: if this matters for browser) then we push the types down. The thing that I'm trying to reason about is how much we care about this metric, certainly in Winforms use case we don't since both TypeConverter and Drawing will always be used.

Copy link
Member

@safern safern Jun 23, 2020

Choose a reason for hiding this comment

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

@ericstj fair point; we can do that. Will reset CI then to make sure we get a fresh run and then merge.

@eriawan eriawan force-pushed the add_attributes_S.Drawing.Printing.PrintDocument branch from 68e5a3e to 3fc9fb2 Compare May 8, 2020 19:58
@jaredpar jaredpar mentioned this pull request May 11, 2020
Closed
@eriawan eriawan force-pushed the add_attributes_S.Drawing.Printing.PrintDocument branch from 3fc9fb2 to 122315d Compare May 11, 2020 22:17
@stephentoub stephentoub requested a review from safern June 8, 2020 21:54
eriawan added 2 commits June 16, 2020 01:18
@eriawan
add DefaultPropertyAttribute and DefaultEventAttribute to PrintDocume…
3e1066a 
…nt class and add references to System.ComponentModel.TypeConverter
@eriawan
add the DefaultPropertyAttribute and DefaultEventAttribute to PrintDo…
670ea8e 
…cument in the ref

Add DefaultEventAttribute and DefaultPropertyAttribute to PrintDocument in PrintDoument.Unix.cs
@eriawan eriawan force-pushed the add_attributes_S.Drawing.Printing.PrintDocument branch from 122315d to 670ea8e Compare June 15, 2020 19:09
@safern safern closed this Jun 23, 2020
@safern safern reopened this Jun 23, 2020
@safern
Copy link
Member

safern commented Jun 23, 2020

Build are green. Perf build didn't update since the interpreter build was removed from CI.

@safern safern merged commit 9de1a7e into dotnet:master Jun 23, 2020
@safern
Copy link
Member

safern commented Jun 23, 2020

Thanks @eriawan for the patience and effort!

@eriawan
Copy link
Member Author

eriawan commented Jun 23, 2020

@safern
You're welcome 👍

@eriawan eriawan deleted the add_attributes_S.Drawing.Printing.PrintDocument branch June 23, 2020 20:48
@ghost ghost locked as resolved and limited conversation to collaborators Dec 9, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@danmoseley danmoseley danmoseley left review comments

@eerhardt eerhardt eerhardt left review comments

@ericstj ericstj ericstj left review comments

@vitek-karas vitek-karas vitek-karas left review comments

@safern safern safern approved these changes

Assignees

No one assigned

Labels

area-System.Drawing

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

System.Drawing.Printing.PrintDocument is missing DefaultEvent and DefaultProperty attributes

7 participants

@eriawan @safern @danmoseley @eerhardt @ericstj @vitek-karas @Dotnet-GitSync-Bot