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

Fix crashes in Release build #63

Closed
wants to merge 2 commits into from
Closed

Fix crashes in Release build #63

wants to merge 2 commits into from

Conversation

hez2010
Copy link
Contributor

@hez2010 hez2010 commented Aug 18, 2021 •
edited
Loading

Seems that it's a bug of .NET Native compiler. It failed to propagate JsonConverter from interface to implementation classes.

Also, I bumped some dependencies.

Fixes #60.

@hez2010 hez2010 changed the title Fix style serialization Fix crashes in Release build Aug 18, 2021
@hez2010 hez2010 mentioned this pull request Aug 18, 2021
Merged
@hez2010
Copy link
Contributor Author

hez2010 commented Oct 10, 2021 •
edited
Loading

Any updates? It think this fix is simpler than putting lots of JsonIgnore.

hawkerm
hawkerm reviewed Oct 11, 2021
View reviewed changes
MonacoEditorTestApp/MainPage.xaml.cs
Comment on lines +363 to 364
Editor.Markers.Clear();
await Editor.SetModelMarkersAsync("CodeEditor", Array.Empty<IMarkerData>());
Copy link
Owner

Choose a reason for hiding this comment

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

Wouldn't we need only one and not the other? Trying to remember why the original line was commented out now.

Copy link
Owner

Choose a reason for hiding this comment

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

Think this is the resolution to #62, but not sure why the next line wasn't accomplishing the same thing...

@hawkerm
Copy link
Owner

hawkerm commented Oct 11, 2021

Got busy at work last month, but want to get back to this soon. Just not sure how to approach reconciling the two PRs.

Would be a bit easier I think to have this based off of the existing changes in #48 and merge into that first instead.

The JsonIgnores are still useful as we don't need to be serializing those properties anyway.

@hez2010
Copy link
Contributor Author

hez2010 commented Oct 11, 2021 •
edited
Loading

The JsonConverter CssStyleConverter will ignore other properties automatically while doing serialization.

@hawkerm
Copy link
Owner

hawkerm commented Oct 21, 2021

Rebased and merged this in to my prep branch. Thanks!

@hawkerm hawkerm closed this Oct 21, 2021
This was referenced Oct 21, 2021
Closed
Closed
hawkerm added a commit that referenced this pull request Oct 21, 2021
@hawkerm
Verify that #62 was fixed by #63, remove duplicate line code in sample
97314ea
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hawkerm hawkerm hawkerm left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@hez2010 @hawkerm