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

Nullables!! #192

Open
wants to merge 45 commits into
base: master
Choose a base branch
from
Open

Nullables!! #192

wants to merge 45 commits into from

Conversation

Happypig375
Copy link
Contributor

Closes #191

Nullable reference types are now enabled for:

  • Typography.OpenFont
  • Typography.GlyphLayout
  • Typography.MsdfGen
  • Typography.TextBreak
  • Typography.TextFlow
  • Typography.TextServices
  • Typography.One

Nullable warnings from above projects will be treated as errors to ensure that nullability seen by users is truthful. If they stay as warnings, it is very possible that they will be ignored like the rest of Typography's warnings 😉 They are not enabled for PixelFarm projects - annotating them will need action from the PixelFarm repo as well.

Additional goodies:

  • C# 8.0 enabled for all projects
  • More code sharing - eliminated some duplicate files, e.g. FontManagement.cs was in both Typography.TextFlow and Demo/DevTextPrinter
  • SDK-style multi-targeting build projects! This is necessary because of <Nullable> has no effect in old-style csproj dotnet/project-system#5551
  • Typography.One now does not reference Unpack. This is because of
  1. The code in Unpack is too much to annotate
  2. Users will very likely use System.IO.Compression instead which includes all of Unpack's functionality (Brotli, Deflate and GZip compression) with the added benefit of performance

P.S. This is probably my largest PR ever 😆 🎉

@prepare
Copy link
Member

prepare commented Mar 26, 2020

@Happypig375 ,

Thank you for your PR.

At this time,
The PixelFarm lib is under 'refactoring phase'.

After that => I will review this PR soon.

@zwcloud
Copy link
Contributor

zwcloud commented Mar 26, 2020 •
edited

So many logic changes. Have you gone through all the visual demos to make sure nothing is broken?
It is not enough to be buildable.

@Happypig375
Copy link
Contributor Author

I did not try the Android/iOS demos but Windows demos work just fine.

@zwcloud
Copy link
Contributor

zwcloud commented Mar 26, 2020 •
edited

@prepare You can pick some important APIs and make a list of them. I'd like to help adding unit tests on them. Visual correctness is quite fragile, I think.

@charlesroddie
Copy link

@prepare I would suggest merging this in relatively fast if possible. Enabling NRTs is good hygiene and clarifies any current structure, which will only help any future refactoring. This PR represents a lot of work and conflicts will accumulate fast.

@Happypig375 Happypig375 mentioned this pull request May 11, 2020
Open
@Happypig375
Copy link
Contributor Author

@prepare When will this be merged? I'll fix conflicts if you confirm that this will be merged.

@prepare
Copy link
Member

prepare commented May 29, 2020

@Happypig375

I need to test this at least with the
PixelFarm and HtmlRenderer and agg-sharp (MatterHackers/agg-sharp#1307) too.

@Happypig375
Copy link
Contributor Author

A few days ago the amount of conflicting files could fit inside the GitHub interface. Now it is off the charts.

@Happypig375 Happypig375 mentioned this pull request Jun 20, 2020
Closed
@Happypig375
Copy link
Contributor Author

@prepare What's the progress of testing this PR?

@Happypig375
Copy link
Contributor Author

The silence is deafening.

@Happypig375 Happypig375 mentioned this pull request Jun 28, 2020
Open
@prepare prepare mentioned this pull request Jul 14, 2020
Open
@Happypig375 Happypig375 mentioned this pull request Jul 16, 2020
Open
@charlesroddie
Copy link

Note @Happypig375 :

#181 (comment)
I will remove old projects for .net2.0-3.5 in this December too.

The presence of the obsolete dotnet projects in this repo is causing a lot of the diff in this PR. I assume it won't be hard to merge any change removing these projects into this PR (the main work being the existing conflicts), since it would be mainly be deleting a lot of files. @prepare do you agree that would make this PR reviewable?

@Happypig375 Happypig375 mentioned this pull request Feb 25, 2021
Merged
@virzak
Copy link
Contributor

virzak commented Mar 5, 2021

So because #221 merged, you have to resolve now. Let me know if you need any help with it.

@Happypig375
Copy link
Contributor Author

@virzak Seems not worth the work if it's not going to be merged anyways.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Enable nullable?
5 participants
@Happypig375 @prepare @zwcloud @charlesroddie @virzak