🏆 Live Soccer TV: CommunityToolkit/Lottie Windows/pull/

simeoncran · 2020-10-01T22:44:35Z

This enables C# 8 nullable reference types. Many files have nullable disabled in order to get this started. Enabling nullable in all files will be done opportunistically over time.

This enables C# 8 nullable reference types in the parts of the codebase where we always build with C# 8. Most of the codebase is built using into winrt components and apps and therefore can't use C# 8 yet. That leaves the LottieGen command line interface and the code generator as the only parts that can use C# 8.

simeoncran · 2020-10-01T22:58:34Z

+
 using System;
 using System.Collections.Generic;
+using System.ComponentModel.DataAnnotations;


Accidental. Remove. #Closed

derobinsMSFT · 2020-10-02T17:29:55Z

            // for the current character already. If there is, use it, if not, add one.
            bool matched = false;
-            foreach (TrieNode n in currrentNode.Children)
+            foreach (var node in currrentNode.Children)


var [](start = 21, length = 3)

Nit: might as well make the char above a var. #Closed

derobinsMSFT · 2020-10-02T17:30:31Z

-                    else if (n.Keyword.HasValue && n.Keyword.Value.CompareTo(id) != 0)
+
+                    // If the node is a terminal (i.e. it has no children) then this keyword
+                    // cannot be added as it the same as an existing keyword.


it [](start = 42, length = 2)

"it is" || "it's" #Closed

derobinsMSFT · 2020-10-02T17:33:29Z

 // The .NET Foundation licenses this file to you under the MIT license.
 // See the LICENSE file in the project root for more information.

+#nullable enable


Just to check my understanding-- are these temporary until the rest of the codebase is compatible with C# 8.0? Looks like it's possible to enable nullable refs for a whole project vs. file-by-file. #Closed

I had thought we couldn't enable nullables on .net native, but it looks like we can now. I'll make the projects default to nullable enable, and add #nullable disable to all the files I haven't modified yet. That will open a can of worms... so I'll grab my worm fork.

In reply to: 498956805 [](ancestors = 498956805)

derobinsMSFT · 2020-10-02T17:53:44Z

+            public string? Name { get; set; }

-            public string FieldName => RequiresStorage ? CamelCase(Name) : null;
+            public string? FieldName => RequiresStorage ? CamelCase(Name!) : null;


Name! [](start = 68, length = 5)

Should this check if Name is null? If Name is null then FieldName would just be null anyway-- unless you explicitly want to raise an Exception here if Name is null. #Closed

I think this one can use one of the attributes. What do you think is the right thing to do here?

In reply to: 498966203 [](ancestors = 498966203)

The code as written is indicating that we expect the Name value is non-null at this point. Should we add code to check that when CamelCase will blow up anyway if it's not true. There is a balance between making errors easy to find and the cost of extra code for checking errors. If the errors are going to be detected by some other mechanism (CamelCase blowing up in this case) then the extra code may not be worth it.

In reply to: 499018274 [](ancestors = 499018274,498966203)

It depends on whether it's okay to have a null FieldName due to Name being null. There's already a case where it's okay for FieldName to be null (!RequiresStorage), but would there ever be a use case where I don't have a Name but still want to use the (null) FieldName anyway? If not, then I think it's fine to just let CamelCase crash.

In reply to: 499030990 [](ancestors = 499030990,499018274,498966203)

Name should never be null if the object is actually going to be used - we do a pass over the ObjectData's to set their Names. But that happens after they've been constructed. As a result Name has to be nullable.
This is a general problem of using stateful objects: if you can't set the state in the constructor, the reference-typed state of an object has to be nullable.
Nullable reference types enables the compiler to help you remove unnecessary null state, but some state, like this, is just part of the design.
More powerful languages (e.g. Rust) probably would enable ObjectData to be written in a way that it is stateless, but I don't know how to solve the problem in C#.

In reply to: 499092239 [](ancestors = 499092239,499030990,499018274,498966203)

derobinsMSFT · 2020-10-02T17:55:47Z

+                hText: generator.GenerateHeaderText(),
+                idlFilename: generator._idlFileName,
+                idlText: generator.GenerateIdlText(),
+                assets: generator.GetAssetsList()


assets: generator.GetAssetsList() [](start = 16, length = 33)

Nit: put back at front of list? #Closed

I guess so. It matches the constructor. The order was kind of based on some notion of meatiness, not alphabetical.

In reply to: 498967206 [](ancestors = 498967206)

Actually, I won't fix that. I don't understand why it should be organized with the assets at the front.

In reply to: 498981413 [](ancestors = 498981413,498967206)

I see this is how it's done elsewhere, too. Never mind! Was the non-alphabetization that caught my eye, but this ordering is also logical.

In reply to: 499027796 [](ancestors = 499027796,498981413,498967206)

derobinsMSFT · 2020-10-02T18:23:20Z


    internal CommandlineTokenizer<TKeywordId> AddPrefixedKeyword(TKeywordId id) =>
-        AddPrefixedKeyword(id, Enum.GetName(typeof(TKeywordId), id));
+        AddPrefixedKeyword(id, Enum.GetName(typeof(TKeywordId), id)!);


Enum.GetName(typeof(TKeywordId), id)! [](start = 31, length = 37)

I don't know what the style guidelines/recommendations about this are, but generally it feels like there's an overuse of the null-forgiveness operator in this PR. It seems counterproductive to notify the compiler about all of these nullable objects, but then also have so many instances of telling the compiler, "Trust me, this definitely won't be null here."

Should some of these nullable objects not be nullable after all?

Could there be nullable and non-nullable versions of some of these objects?

Could some of these classes have methods that abstract away checking if a field is null and guarantee returning a non-nullable type?

The nullable reference type blog post mentioned that future changes are coming to help minimize the need for the null-forgiveness operator, so maybe having to use the operator everywhere is just a temporary necessity. Let me know your thoughts. #Closed

I agree in principle. I actually thought this was safe, but maybe it isn't.
Please add comments for any use of ! that you think is invalid (and explain why they're invalid, and if possible suggest the fix).

In reply to: 498981763 [](ancestors = 498981763)

derobinsMSFT · 2020-10-02T18:24:32Z

    readonly string _className;
    bool _reportedErrors;
    bool? _isTranslatedSuccessfully;
-    LottieComposition _lottieComposition;


LottieComposition _lottieComposition; [](start = 4, length = 37)

Curious about this change-- why make the LottieComposition a passed-around parameter instead of a field? #Closed

This is one of the nice things about nullables - it makes you think about where a field might be set to null. In this case it is easier to pass around a non-nullable than it is to reason about all the possible paths that might result in _lottieComposition being dereferenced when it's null.

In reply to: 498982355 [](ancestors = 498982355)

Many files have nullables disabled so I don't have to fix everything at once. I'll get them all enabled bit by bit. All new files default to nullable now.

derobinsMSFT · 2020-10-03T00:11:31Z

                if (marker.DurationInFrames > 0)
                {
-                    yield return new NamedConstant(marker.EndConstant, $"Marker: {marker.Name}.", ConstantType.Float, (float)marker.EndProgress);
+                    yield return new NamedConstant(marker.EndConstant!, $"Marker: {marker.Name}.", ConstantType.Float, (float)marker.EndProgress);


marker.EndConstant! [](start = 51, length = 19)

A marker's EndConstant should be non-null if it has a non-zero duration? This seems like something the marker itself should check-- would remove the need for the operator here. #Closed

How? I assume you mean there is an attribute for it, but I don't think there is.

In reply to: 499094725 [](ancestors = 499094725)

Look into the NotNullWhenAttribute.

Perhaps somewhere you can use the attribute like so:
[NotNullWhen(DurationInFrames > 0]

In reply to: 499096701 [](ancestors = 499096701,499094725)

The docs for NotNullWhenAttribute says it is to enforce that a value is NOT null, not that it is allowed to be null. MaybeNullWhenAttribute looks closer to what we'd want, but it only works on methods, and both of these attributes relate a boolean return value to a parameter.

In reply to: 499804351 [](ancestors = 499804351,499096701,499094725)

derobinsMSFT · 2020-10-03T00:21:28Z

@@ -917,11 +921,11 @@ static LoadedImageSurfaceInfo LoadedImageSurfaceInfoFromObjectData(ObjectData no
            var bytes = (node.Object as Wmd.LoadedImageSurfaceFromStream)?.Bytes;
            return new LoadedImageSurfaceInfo(


LoadedImageSurfaceInfo [](start = 23, length = 22)

This might be a bit much, but one idea for avoiding the operators here would be creating an ObjectData that doesn't accept nullables, and use that as the type of the CanonicalNode (the only node passed to this method). #Closed

Sounds good. Can you explain how to do that?

In reply to: 499096088 [](ancestors = 499096088)

DIscussed offline-- based on the setup/nature of ObjectData using operators here is cleaner than trying to create a derived class or class specifically for internal use by ObjectData.

In reply to: 499096819 [](ancestors = 499096819,499096088)

derobinsMSFT

simeoncran requested a review from a team as a code owner October 1, 2020 22:44

simeoncran commented Oct 1, 2020

View reviewed changes

Tweaks.

46ad14d

derobinsMSFT reviewed Oct 2, 2020

View reviewed changes

Simeon Cran added 3 commits October 2, 2020 12:36

Enable nullable for all the projects.

1035c9d

Many files have nullables disabled so I don't have to fix everything at once. I'll get them all enabled bit by bit. All new files default to nullable now.

Missed some files.

2ab5b09

Neaten.

1968d7e

derobinsMSFT reviewed Oct 3, 2020

View reviewed changes

derobinsMSFT approved these changes Oct 6, 2020

View reviewed changes

simeoncran merged commit a79faa9 into master Oct 6, 2020

		@@ -917,11 +921,11 @@ static LoadedImageSurfaceInfo LoadedImageSurfaceInfoFromObjectData(ObjectData no
		var bytes = (node.Object as Wmd.LoadedImageSurfaceFromStream)?.Bytes;
		return new LoadedImageSurfaceInfo(

Conversation

simeoncran commented Oct 1, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

simeoncran Oct 1, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

derobinsMSFT Oct 2, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

derobinsMSFT Oct 2, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

derobinsMSFT Oct 2, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

derobinsMSFT Oct 2, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

derobinsMSFT Oct 2, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

derobinsMSFT Oct 2, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

derobinsMSFT Oct 2, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

derobinsMSFT Oct 3, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

derobinsMSFT Oct 3, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

derobinsMSFT left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Uh oh!

Assignees

simeoncran commented Oct 1, 2020 •

edited

Loading

simeoncran Oct 1, 2020 •

edited

Loading

derobinsMSFT Oct 2, 2020 •

edited

Loading

derobinsMSFT Oct 2, 2020 •

edited

Loading

derobinsMSFT Oct 2, 2020 •

edited

Loading

derobinsMSFT Oct 2, 2020 •

edited

Loading

derobinsMSFT Oct 2, 2020 •

edited

Loading

derobinsMSFT Oct 2, 2020 •

edited

Loading

derobinsMSFT Oct 2, 2020 •

edited

Loading

derobinsMSFT Oct 3, 2020 •

edited

Loading

derobinsMSFT Oct 3, 2020 •

edited

Loading