dimonovdd / Xamarin.MediaGallery

This plugin is designed to picking and save images and video files from native gallery of Android and iOS devices and capture photos
MIT License
151 stars 18 forks source link

Improve getting the current context #24

Closed dimonovdd closed 3 years ago

dimonovdd commented 3 years ago

Summary

Getting the current activity is not done quite correctly.

public static partial class Platform
{
    static Activity currentActivity;

    internal static Activity AppActivity
         => currentActivity
        ?? throw ExeptionHelper.ActivityNotDetected;
}

Permalink

Maybe we could replace this with a IActivityLifecycleCallbacks or the Android.App.Application.Context

pictos commented 3 years ago

@dimonovdd is it easy to reproduce on the sample app?

SkyeHoefling commented 3 years ago

I have used this technique in the past but if I recall correctly, I had a discussion with @pictos during a code review at XCT that it is prone to performance and memory leaks. When we needed to add it to that project we used the following approach

/// <summary>
/// Gets the <see cref="Context"/>.
/// </summary>
internal static Context Context
{
    get
    {
        var page = Forms.Application.Current.MainPage;
        var renderer = page.GetRenderer();
        return renderer.View.Context ?? throw new NullReferenceException($"{nameof(Context)} cannot be null");
    }
}

This solution won't tightly couple any downstream project to the MainActivity or other Activity objects that they use throughout the lifecycle of their application.

SkyeHoefling commented 3 years ago

That won't work because this does not take a dependency on Xamarin.Forms. I am going to try and submit a PR for you all to look at that might solve this problem.

SkyeHoefling commented 3 years ago

Some notes to share after a little bit of research. I don't think what you are doing in this library is necessarily wrong or that there is a better way to solve the problem. You have some trade-offs to consider. First off I don't think you are going to remove any android init code as there is no easy way to get the current activity without setting it like you are.

I just looked into how Xamarin Essentials does this and they created an implementation to the IActivityLifecycleCallbacks to get a complete hook into the lifecycle events as well as the current activity. I think that level of API access isn't necessary for what you are doing in this library. https://github.com/xamarin/Essentials/blob/main/Xamarin.Essentials/Platform/Platform.android.cs

Another option would be using the Android.App.Application.Context which will return the application context. You can't just cast that to an Activity and hope for the best as that will be null. You may be able to make some assumptions with an application context and crawl through the nested activities until you find the correct current Activity, but that can get messy.

The major thing that can be done to improve this is assessing your risk for memory leaks and nulling out the Activity reference when it is no longer needed.

pictos commented 3 years ago

Another option would be using the Android.App.Application.Context which will return the application context

If I remember well this is always null, at least for my tries on XCT.

He's setting the Activity using an Init method, which should never change, but since he opened an Activity to pick some media, that could be the issue. (I'm guessing here)

dimonovdd commented 3 years ago
var page = Forms.Application.Current.MainPage;

@pictos @ahoefling Hi. Thanks. I would not like to add a dependency on Xamarin.Forms to the plugin I could use a Android.Content.Context but then I won't be able to call StartActivityForResult method

I have one idea, I will try to do it soon. I'll be happy if you watch it.

SIDOVSKY commented 3 years ago

@ahoefling Could you please describe the API level access problem of the IActivityLifecycleCallbacks in more detail?

@dimonovdd I think you can simply use the Xamarin.Essentials.Platform.CurrentActivity api since you reference Essentials or borrow the ActivityLifecycleContextListener class if you plan to drop the reference.

Moreover, you can use the Activity Result API from AndroidX.Activity in the lifecycle callback listener and avoid the NativeMedia.Platform.OnActivityResult requirement.