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

Adding some of the pieces needed for supporting image assets #101

Merged
merged 6 commits into from
Apr 22, 2019

Conversation

simeoncran
Copy link
Contributor

Support for embedded images in Lottie files.
Introducing the WinUIXamlMediaData project so we can bring in LoadedImageSource for loading images.

Support for embedded images in Lottie files.
Introducing the WinUIXamlMediaData project so we can bring in LoadedImageSource for loading images.
eliezerpMS
eliezerpMS previously approved these changes Apr 19, 2019
Copy link
Contributor

@eliezerpMS eliezerpMS left a comment

Choose a reason for hiding this comment

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

:shipit:

@@ -452,6 +452,71 @@ Asset ParseAsset(JsonReader reader)
throw EofException;
}

static ImageAsset CreateImageAsset(string id, double width, double height, string imagePath, string fileName)
Copy link
Contributor

@eliezerpMS eliezerpMS Apr 19, 2019

Choose a reason for hiding this comment

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

static [](start = 8, length = 6)

Just curious, but why does this need to be static? #ByDesign

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Doesn't need to be, but I like to make methods static whenever they can be. It tells the reader that it does not depend on any object state, which helps local reasoning.


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

eliezerpMS
eliezerpMS previously approved these changes Apr 19, 2019
Copy link
Contributor

@eliezerpMS eliezerpMS left a comment

Choose a reason for hiding this comment

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

:shipit:

@simeoncran simeoncran requested a review from plaiMS April 19, 2019 20:34
The CompositionSurfaceBase made sense when the only implementations of ICompositionSurface
were also CompositionObjects. But with support for LoadedImageSurface that all changes.
This change is only part of the solution - in order to support LoadedImageSurface without
introducing a circular dependency between WinCompData and WinUIXamlData we will need to
add a new project for anything that needs to refer to both of them, i.e. the ObjectGraph,
Instantiator, Optimizer, codegen, and the dumpers.
Adding CleanObjs.cmd script to easily delete all obj directories. This can be useful when diagnosing build issues because the regular clean doesn't delete objs.
return GetCompositionVisualSurface((CompositionVisualSurface)obj);

case "LoadedImageSurface": // Not yet implemented.
Copy link
Contributor

@plaiMS plaiMS Apr 22, 2019

Choose a reason for hiding this comment

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

"LoadedImageSurface" [](start = 21, length = 20)

why not use nameof(LoadedImageSurface)? #ByDesign

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can't do that yet because it would create a dependency from WinCompData to WinUIXamlMediaData. After the next refactoring we can fix this.


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

Reference(from, (CompositionObject)obj);
return true;

case "LoadedImageSurface": // Not yet implemented.
Copy link
Contributor

@plaiMS plaiMS Apr 22, 2019

Choose a reason for hiding this comment

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

"LoadedImageSurface" [](start = 21, length = 20)

why not use nameof(LoadedImageSurface)? #ByDesign

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can't do that yet because it would create a dependency from WinCompData to WinUIXamlMediaData. After the next refactoring we can fix this.


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

new XAttribute(nameof(imageAsset.Width), imageAsset.Width),
new XAttribute(nameof(imageAsset.Height), imageAsset.Height),
new XAttribute(nameof(imageAsset.Format), imageAsset.Format),
new XAttribute(nameof(imageAsset.Bytes), imageAsset.Bytes.Length));
Copy link
Contributor

@plaiMS plaiMS Apr 22, 2019

Choose a reason for hiding this comment

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

nameof(imageAsset.Bytes), imageAsset.Bytes.Length [](start = 31, length = 49)

Length of byte array is outputted for the "Bytes" attribute. That's misleading. #Resolved

{ "Width", asset.Width },
{ "Height", asset.Height },
{ "Format", asset.Format},
{ "Bytes", asset.Bytes.Length},
Copy link
Contributor

@plaiMS plaiMS Apr 22, 2019

Choose a reason for hiding this comment

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

"Bytes", asset.Bytes.Length [](start = 18, length = 27)

"BytesArrayLength" to be exact? #Resolved

plaiMS
plaiMS previously approved these changes Apr 22, 2019
Copy link
Contributor

@plaiMS plaiMS left a comment

Choose a reason for hiding this comment

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

:shipit:

Copy link
Contributor

@plaiMS plaiMS left a comment

Choose a reason for hiding this comment

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

:shipit:

@simeoncran simeoncran merged commit 2fceb1c into master Apr 22, 2019
@delete-merged-branch delete-merged-branch bot deleted the LoadedImageSource branch April 22, 2019 21:45
@simeoncran simeoncran restored the LoadedImageSource branch April 22, 2019 21:54
@simeoncran simeoncran deleted the LoadedImageSource branch April 22, 2019 22:02
@michael-hawker michael-hawker added this to the 6.0 milestone Nov 13, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants