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

MudColor: Add XML comments and make it serializable with STJ / NewtonsoftJson #8579

Merged
merged 6 commits into from
Apr 6, 2024

Conversation

ScarletKuro
Copy link
Member

@ScarletKuro ScarletKuro commented Apr 5, 2024

Description

Fixes: #7688
Fixes: #3095

This makes it compatible only with STJ.
I know other Serializers exists like Newtonsoft.Json, YamlDotNet, and many other binary ones.
But I think think most customers are using the built in - STJ, and the issues were specifically talking about it. I think advanced user can workaround it with custom converters.

How Has This Been Tested?

New unit tests.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

Checklist:

  • The PR is submitted to the correct branch (dev).
  • My code follows the code style of this project.
  • I've added relevant tests.

@github-actions github-actions bot added bug Something does not work as intended/expected PR: needs review labels Apr 5, 2024
Copy link

codecov bot commented Apr 5, 2024

Codecov Report

Attention: Patch coverage is 96.26168% with 4 lines in your changes are missing coverage. Please review.

Project coverage is 89.78%. Comparing base (0affaa4) to head (cd8ce65).
Report is 3 commits behind head on dev.

Files Patch % Lines
src/MudBlazor/Utilities/MudColor.cs 96.26% 3 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##              dev    #8579      +/-   ##
==========================================
+ Coverage   89.70%   89.78%   +0.07%     
==========================================
  Files         411      411              
  Lines       11817    11840      +23     
  Branches     2362     2363       +1     
==========================================
+ Hits        10600    10630      +30     
+ Misses        689      682       -7     
  Partials      528      528              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

@danielchalmers danielchalmers left a comment

Choose a reason for hiding this comment

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

I really really dislike the idea of locking into one serializer. Why can't it work out of the box for all serializers? Please let me look into this more before merging

Btw I did some of that cleanup in my PRs so there will be conflicts

@ScarletKuro
Copy link
Member Author

ScarletKuro commented Apr 5, 2024

Why can't it work out of the box for all serializers?

Because MudColors is immutable, when you change something via method, it creates a new "readonly" instance.
This is especially important because it's using GetHasCode and the _valuesAsByte should not change once the instance is considered initialized, but you cannot guarantee that if the property setters won't be private or init only.

For it to make work with any serializers you need:
option 1: Make RGBA with setters and HIDE other properties, add an empty constructor, remove immutability
option 2: Make _valuesAsByte public + writable, hide everything else + empty constructor, remove immutability

only then this type will function properly and you won't have other junks in the serialized object aka HLS and APercentage. It should be either RGBA or byte representation only or string aka hex.

@ScarletKuro
Copy link
Member Author

ScarletKuro commented Apr 6, 2024

@danielchalmers I added more supports, now it supports Newtonsoft as well and others that respect ISerializable.
There is no better ways to make immutable classes to have flawless serialization.

@ScarletKuro ScarletKuro changed the title MudColor: Add XML comments and make it serializable with STJ MudColor: Add XML comments and make it serializable with STJ / NewtonsoftJson Apr 6, 2024
@danielchalmers
Copy link
Contributor

I added more supports, now it supports Newtonsoft as well and others that respect ISerializable.

Great, thank you.

Yeah, not ideal but I get the point. I don't think there's a nicer way of doing it. Other changes are great. Nice work
I made the same changes to the color picker tests in #8576 if you don't mind reverting that

@ScarletKuro
Copy link
Member Author

I made the same changes to the color picker tests in #8576 if you don't mind reverting that

I reverted ColorPickerTests

@ScarletKuro ScarletKuro merged commit 64c80f0 into MudBlazor:dev Apr 6, 2024
4 checks passed
@ScarletKuro ScarletKuro deleted the color_json branch April 6, 2024 10:45
danielchalmers pushed a commit to danielchalmers/MudBlazor that referenced this pull request Apr 7, 2024
biegehydra pushed a commit to biegehydra/MudBlazor that referenced this pull request Apr 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something does not work as intended/expected
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Deserialization of MudColor causes exception Support json deserialization of themes
3 participants