Skip to content

Essentials: Port Xamarin.MediaGallery#13564

Closed
dimonovdd wants to merge 13 commits intodotnet:mainfrom
dimonovdd:mediaGallery
Closed

Essentials: Port Xamarin.MediaGallery#13564
dimonovdd wants to merge 13 commits intodotnet:mainfrom
dimonovdd:mediaGallery

Conversation

@dimonovdd
Copy link
Copy Markdown
Contributor

@dimonovdd dimonovdd commented Feb 25, 2023

Description of Change

Xamarin.MediaGallery was created as temporary solution. During its existence, that project has helped many developers. PickAsync, CapturePhotoAsync and SaveAsync were debugged and tested in it in many projects.

It's time to get rid of this workaround.

API Changes

  1. All members from IMediaPicker and MediaPicker marked as [Obsolete("Use MediaGallery")]
  2. All logic is moved to new IMediaGallery and MediaGallery
interface IMediaGallery
{
    bool IsSupported { get; }
    bool CheckCaptureSupport(MediaFileType type);
    Task<MediaPickResult> CaptureAsync(MediaFileType type, CancellationToken token = default);
    Task<MediaPickResult> PickAsync(int selectionLimit = 1, params MediaFileType[] types);
    Task<MediaPickResult> PickAsync(MediaPickRequest request, CancellationToken token = default);
    MultiPickingBehaviour GetMultiPickingBehaviour();
    Task SaveAsync(MediaFileType type, Stream fileStream, string fileName);
    Task SaveAsync(MediaFileType type, byte[] data, string fileName);
    Task SaveAsync(MediaFileType type, string filePath);
}

Behavioral Changes

iOS

  • PHPickerViewController for iOS above 13

Android

  • New Photo picker for Android above API 30 with ExtensionVersion >= 2
  • Removed the use of IntermediateActivity. This is a very bad disease of Essentials. IntermediateActivity to an increase in the probability of destruction of MainActivity, a need to request unnecessary permissions, ugly visual behavior together with Intent.CreateChooser and etc.

Related with

  1. Xamarin.MediaGallery
  2. [Enhancement] Save To Gallery xamarin/Essentials#1628
  3. Impl PickerResultsToMediaFile for #1475 xamarin/Essentials#1750 and Implement the MediaPicker with PHPickerConfiguration xamarin/Essentials#1475
  4. and more issues

Status

@ghost ghost added the community ✨ Community Contribution label Feb 25, 2023
@ghost
Copy link
Copy Markdown

ghost commented Feb 25, 2023

Hey there @dimonovdd! Thank you so much for your PR! Someone from the team will get assigned to your PR shortly and we'll get it reviewed.

@dimonovdd dimonovdd marked this pull request as draft February 25, 2023 21:31
@github-actions
Copy link
Copy Markdown
Contributor

Thank you for your pull request. We are auto-formating your source code to follow our code guidelines.

@github-actions
Copy link
Copy Markdown
Contributor

Thank you for your pull request. We are auto-formating your source code to follow our code guidelines.

@dimonovdd
Copy link
Copy Markdown
Contributor Author

@jfversluis Hi

I think the API should not contain the ability to edit files (Rotate, Quality and etc.). We can do it as extension methods in Community Toolkit.

public static void Rotate(this MediaFileResult file)
{

}

https://gist.github.com/jfversluis/431488a54ab67b56cf54368e0ef1eb35

@github-actions
Copy link
Copy Markdown
Contributor

Thank you for your pull request. We are auto-formating your source code to follow our code guidelines.

1 similar comment
@github-actions
Copy link
Copy Markdown
Contributor

Thank you for your pull request. We are auto-formating your source code to follow our code guidelines.

@drasticactions
Copy link
Copy Markdown
Contributor

I don't know the advantages of merging this code into the mainline of MAUI versus leaving it as a separate library or moving it into the community toolkit instead. Moving it into MAUI would tie this code to its release schedule, making it harder to iterate on changes or release fixes should the need arise. Couldn't we obsolete the existing Media Picker control and point people to this library or the community toolkit instead?

@dimonovdd
Copy link
Copy Markdown
Contributor Author

dimonovdd commented Feb 27, 2023

Couldn't we obsolete the existing Media Picker control and point people to this library or the community toolkit instead?

What about FilePicker, Contacts and etc. They all have problems.

I'm not against your suggestion. But I want it to be systematic

@drasticactions
Copy link
Copy Markdown
Contributor

drasticactions commented Feb 27, 2023

Couldn't we obsolete the existing Media Picker control and point people to this library or the community toolkit instead?

What about FilePicker, Contacts and etc. They all have problems.

I'm not against your suggestion. But I want it to be systematic

Note that I'm not on the MAUI team, these are just my opinions.

In general, yeah, I think so. In particular where there are opinionated UI's being generated, such as what the original Essentials media picker did and what your PR does.

MAUI inherited a ton of functionality with the merging of Xamarin.Esentials. But with that merging, that introduced technical debt; features within in may not have been the most up to date or "best" ways of doing what each implementation set out to do. And, given the size of the team and the shear amount of code to maintain, I think it is worth going back to each of these implementations as they exist and making the call of

  • How "large" is the surface of the API
  • The cost of maintaining it

For example, a standardized cross-platform file access API for reading and writing files could be locked down to the "essentials" that could be contained. If and when breaking changes occur in the underlying platforms, the cost for fixing them may be small. On the inverse, this control is sprawling, like the custom UIs for showing media galleries. When you mention potential hooks for the CommunityToolkit for adding functionality all I can see are potential debt, like the cost of maintaining an API surface that others can use.

These are controls and APIs that need to be maintained by this team, under no assumption that volunteers are there to help. With that in mind, I think it's worth considering taking on the debt of maintaining this control in MAUI verses in the Community Toolkit. It may well be worth it, but that's the cost someone on the MAUI team may need to pay in the future.

@dimonovdd
Copy link
Copy Markdown
Contributor Author

@mandel-macaque thanks. But this is draft

@dimonovdd
Copy link
Copy Markdown
Contributor Author

It may well be worth it, but that's the cost someone on the MAUI team may need to pay in the future.

I think someone thought about it when API was added to Essentials

@drasticactions
Copy link
Copy Markdown
Contributor

It may well be worth it, but that's the cost someone on the MAUI team may need to pay in the future.

I think someone thought about it when API was added to Essentials

I would argue the opposite, hence why it is in the condition it's in.

@Eilon Eilon added the area-essentials Essentials: Device, Display, Connectivity, Secure Storage, Sensors, App Info label Mar 2, 2023
@dimonovdd
Copy link
Copy Markdown
Contributor Author

@jfversluis @mattleibow Hi
Do you have any ideas or comments?

@jfversluis
Copy link
Copy Markdown
Member

jfversluis commented Mar 6, 2023

Sorry it took a little while to get back to you! We had to discuss a couple of things about this. Here is our/my thoughts.

In it's current form I don't think this is something we want to do.

What I like about this PR is that we're going to MediaGallery which opens up possibilities for the future. You've already added functionality to save something to the gallery for instance, which wouldn't make sense with the MediaPicker name. So that's great!

However, I don't think enough value is added to warrant the deprecation of MediaPicker and move to MediaGallery. What happens with this PR is that we present people with a breaking change, but ultimately they don't get much functionality in return.

Don't get me wrong, I see the use of the more modern APIs, picking multiple files, saving files and all that. But from all the feedback that I've been seeing what people really want is the things I've mentioned to you before and described in the spec I was working on: rotation, or at least the meta data so they can do it themselves, compression, etc.

That is also where we have different opinions. I don't agree with the fact that the functionality to get the correct rotation should be in the .NET MAUI Community Toolkit. With a functionality like this I, as a developer, assume that I will be able to get the media in a way that I don't have to do all kinds of processing myself, certainly not when that processing functionality also involves taking on another dependency.

So back to this PR. Right now for us, unfortunately, this is not a priority. So you helping us out would definitely be amazing and really helpful to us. But you have also already stated that your time is limited which is. of course, understandable. If you are not able to do anymore work on this than you have already done, I think we should close this for now and revisit later.

If you do want to spend some more time on it I think here is some things that we want to consider:

  • Add more value in the form of more functionality:
    • Add the option to get the rotated version
    • And/or provide the meta data with the picked file so people can rotate themselves
    • Add the option to resize/compress
    • Potentially other things in the spec I wrote, but the above I think is minimal
  • I don't think deprecation and removal should be done in 1 version. We want to deprecate MediaPicker first just by adding the [Obsolete] tag, keep the functionality so people will have some time to actually make the switch from MediaPicker to MediaGallery, instead of being forced to update their app because MediaPicker is already deleted

Or, there is the whole thing that Tim mentioned above. Maybe we have to face that MediaPicker isn't living up to the quality that we'd like to and we can't give it the attention it deserves right now and we just need to deprecate it as a whole and provide it as a third-party package for the time being. Personally I'd hate that, but that does allow us to move a bit quicker and also already provide this for .NET 7.

The .NET MAUI Community Toolkit would then be a great fit I think to keep it close to the .NET MAUI main package, but we can take this functionality in as a whole, start building on top of it, shape it to what we want and hopefully in the future bring it back to .NET MAUI but now as a more complete solution.

In either case, thank you for your great work on the MediaGallery plugin throughout the years and your willingness to bring this to us in the form of this PR. I hope you can understand our point of view as well.

Let me know your thoughts! Or if anyone else wants to chime in that is appreciated as well of course. 😄

@dimonovdd
Copy link
Copy Markdown
Contributor Author

dimonovdd commented Mar 6, 2023

@jfversluis I understand everything, but I don't understand anything. Maybe we should do a survey.

I don't agree with the fact that the functionality to get the correct rotation
And/or provide the meta data with the picked file so people can rotate themselves

Unlike the current implementation as a result of these changes, all photos will contain metadata. Photos come with the correct orientation which they are stored on the device. If we start turning them ourselves, should we change the metadata?

https://github.com/dimonovdd/Xamarin.MediaGallery#q-why-does-a-image-have-wrong-orientation

Add the option to get the rotated version
Add the option to resize/compress

This PR is aimed at correcting current problems. I just added a save to a device gallery.

Don't get me wrong, I see the use of the more modern APIs, picking multiple files, saving files and all that.

And also there are no unnecessary useless requests for permissions. And I'm still thinking how to get rid of IntermediateActivity. And etc.

@dimonovdd
Copy link
Copy Markdown
Contributor Author

I don't think deprecation and removal should be done in 1 version. We want to deprecate MediaPicker first just by adding the [Obsolete] tag, keep the functionality so people will have some time to actually make the switch from MediaPicker to MediaGallery, instead of being forced to update their app because MediaPicker is already deleted

No one has deleted it

[Obsolete(MediaPicker.ObsoleteMessage)]
public async Task<FileResult?> PickPhotoAsync(MediaPickerOptions? options)
{
      var res = await MediaGallery.PickAsync(
      	  new(options?.Title, 1, default, MediaFileType.Image));
      return res?.Files?.FirstOrDefault();
}

@jfversluis jfversluis self-assigned this Mar 6, 2023
@samhouts samhouts added this to the Under Consideration milestone Aug 17, 2023
@samhouts samhouts added the stale Indicates a stale issue/pr and will be closed soon label Sep 11, 2023
@jfversluis
Copy link
Copy Markdown
Member

OK, so before we reach the 1 year birthday of this PR let's deal with it. Unfortunately, in this case that means closing it for right now.

At this time we have other things that we have on our roadmap and where we want to spend our time. A big PR like this will take up a big chunk of our time reviewing, maybe write some code ourselves, testing, etc. Also, I do think I would want to make some changes to the API before we add this to .NET MAUI directly, which means we need to spend even more time on it. Since this is not something that we determined to be a higher priority right now, we can't do justice to your work and give it the attention it deserves, I'm going to have to close this for the time being.

Additionally, this code is already in a great plugin that is available for people and I hope that will continue to serve people well. As much as I, personally, want to improve this part of .NET MAUI out-of-the-box just as you, this is not the right time.

Thank you so much for the time and effort you have spend on this.

In order to still make this happen, for .NET MAUI, I think a good approach would be to divide it up significantly in many smaller PRs that are easier to digest for us as a team. However, I don't want to waste your time, so if you even are considering doing any more, please contact us first and talk about a plan.

Again, thank you and although this is probably not the outcome you had hoped it to be, at least it will bring some clarity for the time being.

@jfversluis jfversluis closed this Dec 1, 2023
@github-actions github-actions bot locked and limited conversation to collaborators Jan 1, 2024
@samhouts samhouts removed this from the Under Consideration milestone Jul 1, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

area-essentials Essentials: Device, Display, Connectivity, Secure Storage, Sensors, App Info community ✨ Community Contribution stale Indicates a stale issue/pr and will be closed soon

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants