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

Fix animated gifs in Release builds #22874

Merged
merged 1 commit into from
Jun 18, 2024
Merged

Fix animated gifs in Release builds #22874

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions src/Controls/tests/CustomAttributes/Test.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -393,6 +393,7 @@ public enum Image
{
Source,
Source_FontImageSource,
IsAnimationPlaying,
Aspect,
IsOpaque,
IsLoading,
Expand Down
Binary file added ...estCases.Android.Tests/snapshots/android/ImageUITests_IsAnimationPlaying_No.png
View file
Open in desktop
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Binary file added ...stCases.Android.Tests/snapshots/android/ImageUITests_IsAnimationPlaying_Yes.png
View file
Open in desktop
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
12 changes: 12 additions & 0 deletions src/Controls/tests/TestCases.Shared.Tests/Tests/ImageUITests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,4 +36,16 @@ public void Source_FontImageSource()
remote.TapStateButton();
VerifyScreenshot("ImageUITests_Source_FontImageSource_FontAwesome");
}

[Test]
public async Task IsAnimationPlaying()
{
var remote = GoToStateRemote();
await Task.Delay(500); // make sure the gif is NOT playing
VerifyScreenshot("ImageUITests_IsAnimationPlaying_No");

remote.TapStateButton();
await Task.Delay(500); // make sure the gif IS playing
VerifyScreenshot("ImageUITests_IsAnimationPlaying_Yes");
}
}
Binary file added .../TestCases.WinUI.Tests/snapshots/windows/ImageUITests_IsAnimationPlaying_No.png
View file
Open in desktop
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Binary file added ...TestCases.WinUI.Tests/snapshots/windows/ImageUITests_IsAnimationPlaying_Yes.png
View file
Open in desktop
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Binary file added .../tests/TestCases.iOS.Tests/snapshots/ios/ImageUITests_IsAnimationPlaying_No.png
View file
Open in desktop
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Binary file added ...tests/TestCases.iOS.Tests/snapshots/ios/ImageUITests_IsAnimationPlaying_Yes.png
View file
Open in desktop
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
1 change: 1 addition & 0 deletions src/Controls/tests/TestCases/Controls.TestCases.App.csproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
<OutputType>Exe</OutputType>
<SingleProject>true</SingleProject>
<IsPackable>false</IsPackable>
<ImplicitUsings>enable</ImplicitUsings>
<!-- Disable multi-RID builds to workaround a parallel build issue -->
<RuntimeIdentifier Condition="$(TargetFramework.Contains('-maccatalyst'))">maccatalyst-x64</RuntimeIdentifier>
<RuntimeIdentifier Condition="$(TargetFramework.Contains('-maccatalyst')) and '$([System.Runtime.InteropServices.RuntimeInformation]::OSArchitecture)' == 'arm64'">maccatalyst-arm64</RuntimeIdentifier>
Expand Down
15 changes: 15 additions & 0 deletions src/Controls/tests/TestCases/Elements/ImageCoreGalleryPage.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,5 +33,20 @@ protected override void Build()
};
Add(familyContainer);
}

{
var image = new Image
{
Source = "red_is_good.gif",
WidthRequest = 100,
HeightRequest = 100,
};
var container = new StateViewContainer<Image>(Test.Image.IsAnimationPlaying, image);
container.StateChangeButton.Clicked += (s, a) =>
{
image.IsAnimationPlaying = !image.IsAnimationPlaying;
};
Add(container);
}
}
}
Binary file added src/Controls/tests/TestCases/Resources/Images/red_is_good.gif
View file
Open in desktop
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
4 changes: 3 additions & 1 deletion src/Core/src/Platform/Android/ImageViewExtensions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
using Android.Graphics.Drawables;
using Android.Views;
using Android.Widget;
using Bumptech.Glide.Load.Resource.Gif;

namespace Microsoft.Maui.Platform
{
Expand All @@ -29,7 +30,8 @@ public static void UpdateIsAnimationPlaying(this ImageView imageView, IImageSour

public static void UpdateIsAnimationPlaying(this Drawable? drawable, IImageSourcePart image)
Copy link
Member

Choose a reason for hiding this comment

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

I had to fix something similar earlier this year: dotnet/android#8594 I think you can fix this with a few [DynamicDependency] attributes to this method:

[DynamicDependency(nameof(IAnimatable))]
[DynamicDependency(nameof(AnimationD)rawable)]
[DynamicDependency(nameof(GifDrawable))]
[DynamicDependency(nameof(AnimatedImageDrawable))]

Copy link
Member

Choose a reason for hiding this comment

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

Does no C# code use any of the above types? What @simonrozsival suggests seems like the right idea.

Something similar also happens here:

Since no C# code uses NinePatchDrawable it's linked away. Mono.Android.dll and all the AndroidX libraries mark themselves trimmable.

Copy link
Member Author

Choose a reason for hiding this comment

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

The issue is that I don't know what types are being used - I know of some but it could be any type of drawable. I would have to list all of the known ones and then it would still not be super perfect because of third parties.

Also, since we don't use the type in C# - all the loading code is in Java now - we are otentially forcing a whole bunch of C# types to exist when we don't really need it.

Copy link
Member

@simonrozsival simonrozsival Jun 10, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

It doesn't seem like [DynamicDependency] is applicable in this case then. Doing the JavaCast might be the best way after all. Hopefully Proguard won't be messing up the Java side of things :)

{
if (drawable is IAnimatable animatable)
var animatable = drawable.TryJavaCast<IAnimatable>();
if (animatable is not null)
{
if (image.IsAnimationPlaying)
{
Expand Down
16 changes: 16 additions & 0 deletions src/Core/src/Platform/Android/JavaObjectExtensions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,10 +1,13 @@
using System;
using System.Diagnostics.CodeAnalysis;
using Android.Runtime;

namespace Microsoft.Maui
{
static class JavaObjectExtensions
{
const DynamicallyAccessedMemberTypes Constructors = DynamicallyAccessedMemberTypes.PublicConstructors | DynamicallyAccessedMemberTypes.NonPublicConstructors;

public static bool IsDisposed(this Java.Lang.Object obj)
{
return obj.Handle == IntPtr.Zero;
Expand All @@ -30,5 +33,18 @@ public static bool IsAlive([NotNullWhen(true)] this global::Android.Runtime.IJav

return !obj.IsDisposed();
}

public static TResult? TryJavaCast<[DynamicallyAccessedMembers (Constructors)] TResult>(this IJavaObject? instance)
where TResult : class, IJavaObject
{
try
{
return instance.JavaCast<TResult>();
}
catch
{
return null;
}
Comment on lines +44 to +47
Copy link
Member

Choose a reason for hiding this comment

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

I'm not a fan of an empty try-catch with no logging.

Does a [DynamicDependency] like @simonrozsival suggested also solve it? That would have no performance cost, and I think things would still be trimmed away if you had 0 animated gifs in your app.

Copy link
Member Author

Choose a reason for hiding this comment

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

I logged an issue with a feature request or something: dotnet/android#9038

}
}
}