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

Feature - custom albums #123

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

beeradmoore
Copy link
Collaborator

@beeradmoore beeradmoore commented May 25, 2023

Description

Adds the ability to create albums when saving assets.

Related to issue

Fixes #122

API Changes

No breaking changes.
SaveAsync now has an additional argument albumName which is null by default.
If albumName is null then we continue doing what we do (iOS: No album, Android: Album created of the same name as the app).
If albumName is an empty string then no album will be created on both iOS and Android.
If albumName is anything else, then an album is created with that name on both iOS and Android.

Recommendations for testing

I didn't update the sample app as I wasn't sure how to express "keeping this blank will make an album on Android, no album on iOS". But the way I tested was by changing things in SaveVM.cs, from:

await MediaGallery.SaveAsync(type, stream, name);

to:

await MediaGallery.SaveAsync(type, stream, name, "My Album");

PR Checklist

  • All projects build
  • Has samples
  • Rebased onto current main

@dimonovdd
Copy link
Owner

Hi. I'm very busy right now, I'll watch it next week. Sorry

{
Predicate = NSPredicate.FromFormat($"title=\"{albumName}\"")
};
collection = PHAssetCollection.FetchAssetCollections(PHAssetCollectionType.Album, PHAssetCollectionSubtype.AlbumRegular, fetchOptions).FirstObject as PHAssetCollection;
Copy link
Owner

Choose a reason for hiding this comment

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

Should we request additional permissions? On which versions of iOS have you tested this?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

As far as I am aware no additional permissions are required to make an album. Our production app has only had NSPhotoLibraryUsageDescription in it for about 7 years.

It was only until we moved to using Xamarin.MediaGallery did we even realise there was a new NSPhotoLibraryAddUsageDescription permission.

I have only been testing the new code on iOS 16.5, but the source where I originally wrote would have been at least iOS 10 and up.

Copy link
Owner

Choose a reason for hiding this comment

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

If I specify the album name, it asks for additional permission. I don't really like this behavior. It will not be clear to users. Why should the user be able to select the photos to which he provides access when saving the photo?

2023-08-10.10.56.08.PM.mov

@beeradmoore
Copy link
Collaborator Author

Hi. I'm very busy right now, I'll watch it next week. Sorry

No worries. In the mean time I might also make a new branch (from main) and see if I can tidy up the structure and get multi targeting to work cleaner so its easier to test/build for going forwards.

I like that a lot of this library is now in MAUI Essentials, but it seems things work slow over there to get updated even when it is PR'd. Plus certain things they straight up said no to (seen your comments RE: saving to gallery and trying to get it in Xamarin.Essentails + MAUI)

@dimonovdd
Copy link
Owner

@beeradmoore Hi

can you update the sample? If filed is null or empty, we can use:

await MediaGallery.SaveAsync(type, stream, name);

@beeradmoore
Copy link
Collaborator Author

Do you want the a comment in the sample explaining what that property does or do you want me to hard code an album name in the sample so people know it's used?

@dimonovdd
Copy link
Owner

I want us to be able to change it in the sample app.

@beeradmoore
Copy link
Collaborator Author

Sorry for the delay on adding that. I was pulled onto other projects.

I think I have added how you wished. This is how it looks on iOS and Android.

IMG_2041 Large

screenshot-1689229499288 Large

Default Xamarin.Forms styling leaves a lot to be desired on Android 😅.

Regarding future changes (I can file a new issue to discuss rather than on this PR), would you have any objection to me tinkering with the project structure and see if we can get better XF/MAUI side by side support without having to nuke large portions of the project in order to get it to build?

@dimonovdd
Copy link
Owner

Sorry for the delay on adding that

@beeradmoore Please forgive me. I have a little health problems right now. And also my team is now migrating our project to MAUI. I don't have time at all. This is some kind of hell

would you have any objection to me tinkering with the project structure

I'll be only too glad

@dimonovdd
Copy link
Owner

dimonovdd commented Aug 10, 2023

Can you boost the version numbers? Or build a nuget? Let's publish version 2.2.2-preview1 right from this Pull Request
And update README.md please

@beeradmoore
Copy link
Collaborator Author

beeradmoore commented Aug 10, 2023

Hey @dimonovdd , no worries about the delay.

I'll be only too glad

Is there a preferred place you'd like me to make some proposals to changes, make a new issue, DM on Twitter, email?

This is some kind of hell

I can't wait to migrate my main app to MAUI 😅

I have added those changes you requested. Sorry about missing those .NET 6 things, I never built for .NET 6 so never picked that up. I am not sure of the process you use to build nuget packages so I can't test if the version changes to 2.2.2-preview1 is done how they are expected, nor could I test in a MAUI app.

Ironically part of changes I want to do is add some scripts to make it easier for people to build the packages (like the two small scripts I have here) :P

If you are free to leave some instructions for that below I can make sure it builds and then also test in the sample app on MAUI and Xamarin builds.

@dimonovdd
Copy link
Owner

dimonovdd commented Aug 13, 2023

@beeradmoore easiest way:

  1. Open Xamarim.MediaGallery.sln
  2. Delete Permission, Sample.Common, Sample, Sample.Android, Sample.iOS projects
  3. Remove that line from MediaGallery project
<TargetFrameworks>netstandard2.0;Xamarin.iOS10;MonoAndroid10.0;MonoAndroid11.0;MonoAndroid12.0;MonoAndroid13.0;</TargetFrameworks>
  1. Open Xamarim.MediaGallery.targets and set _UseNuget - false
  2. Closes: # IDE
  3. Clean bin, obj, .vs, .idea folders
  4. Open Xamarim.MediaGallery.sln
  5. Build Sample App

@beeradmoore
Copy link
Collaborator Author

Been fighting with it for a few days and I am getting weird errors like

msbuild.sdk.extras/3.0.44/Build/Workarounds.targets(5,5): Error: If you are building projects that require targets from full MSBuild or MSBuildFrameworkToolsPath, you need to use desktop msbuild ('msbuild.exe') instead of 'dotnet build' or 'dotnet msbuild' (Permission.Maui)

when I didn't change anything, and it randomly worked but the .nupkg it built appears to be net6.0 only, no legacy Xamarin support. Other times it gives me this error,

Microsoft.NET.TargetFrameworkInference.targets(127,5): error NETSDK1045: The current .NET SDK does not support targeting .NET Core 6.0. Either target .NET Core 5.0 or lower, or use a version of the .NET SDK that supports .NET Core 6.0.

I was looking for packages that multi-target old and new Xamarin/net6 stuff to see how they accomplish it and I came across James Montemagnos StoreReviewPlugin. How his project is structured seems good, but I downloaded it and attempted to build it and got similar issues as above.

I am unsure if my system is able to build the correct resulting nugets, and while tidying up the csproj/targets system in this project was something I wanted to do (make it easier for new people to jump in and build and then contribute) I am going to need to sort out if my system is being funky here.

@dimonovdd
Copy link
Owner

when I didn't change anything, and it randomly worked but the .nupkg it built appears to be net6.0 only, no legacy Xamarin support. Other times it gives me this error,

haha, Microsoft..... Use windows + console

I was looking for packages that multi-target old and new Xamarin/net6 stuff to see how they accomplish it and I came across James Montemagnos StoreReviewPlugin. How his project is structured seems good, but I downloaded it and attempted to build it and got similar issues as above.

it is very painful

@beeradmoore
Copy link
Collaborator Author

beeradmoore commented Aug 24, 2023

I had some luck updating to using Xamarin.Legacy.Sdk for the csproj type.

But on the other hand I was wondering what it'd take to convince you to release 2.2.2-preview1 and then branch for history sake, and turn main branch into v3.X .NET 6 only branch so things can be tidied up that way 👀

EDIT:

Use windows + console

I didn't know I could. I assumed anything that touched iOS required me to use my macOS machine for Xcode support, but I guess net6-ios does not. I'll try again this weekend.

@beeradmoore
Copy link
Collaborator Author

beeradmoore commented Aug 27, 2023

Update: Trying to build on windows didn't work -_-

@dimonovdd
Copy link
Owner

and turn main branch into v3.X .NET 6 only branch so things can be tidied up that way

I won't do it. Now we are trying to remake our application on MAUI. Attempts are unsuccessful.

See: dotnet/maui#13564

@dimonovdd
Copy link
Owner

I think I can write build scripts to create nugget

@dimonovdd
Copy link
Owner

@beeradmoore
Hi, the project has become much easier. I think we can get back to that PR. But let's fix all the little things you wrote about first.

@beeradmoore
Copy link
Collaborator Author

Sounds good. I'll see if I can rebase this onto main from my end. If not I can remake it.

@beeradmoore
Copy link
Collaborator Author

@dimonovdd , I attempted a rebase but had some odd conflicts. More than happy to cancel this PR and I can re-make those changes from current main this weekend.

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.

Album creation when saving assets
2 participants