Changelog-Fixed: Save Image Permissions

[brett-doffing]

Currently, without permission, saving an image will fail silently and simply not save the image. This PR adds an ImageSaver class to handle permission failures when saving images, as well as display an alert upon failure.

[Fixes: #1150]

[jb55]

I believe this is a very non-standard way to do this. Almost every app I've ever used will re-prompt for access before trying to save. If we know it will fail before we try and we have a way to fix it we should attempt the fix first.

This change would still be useful for if they are out of space on disk, but if that was the case we should alert them of that specific problem.

[jb55]

cc @bryanmontz

[brett-doffing]

I believe this is a very non-standard way to do this. Almost every app I've ever used will re-prompt for access before trying to save. If we know it will fail before we try and we have a way to fix it we should attempt the fix first.

I would agree, and it was something I looked into. Not sure if it is just me, and something I will look further into, but in order to perform this check I had to change the permissions from the privacy photos “additions” permission, to the more general photos permissions, and I would wonder if that would be acceptable if it is indeed the case that I have to change it?

[jb55]

On Mon, Jul 31, 2023 at 09:29:34AM -0700, doffing.brett wrote: > I believe this is a very non-standard way to do this. Almost every > app I've ever used will re-prompt for access before trying to save. > If we know it will fail before we try and we have a way to fix it we > should attempt the fix first. I would agree, and it was something I looked into. Not sure if it is just me, and something I will look further into, but I had to change the permissions from the privacy photos “additions” permission, to the more general photos permissions, and I would wonder if that would be acceptable if it is indeed the case that I have to change it.

Seems weird we would need to switch to general permissions just to prompt for the additions permission, if this is what you're saying? 🤔

[brett-doffing]

It was me. I was asking for .readWrite with PHPhotoLibrary.requestAuthorization(for:) instead of .addOnly, and it was causing a crash. With the fact that I can check permission status, and give an alert based on that, I would ask if we should keep the ImageSaver class that I implemented for unsuccessful saving? I can otherwise get rid of it?

[brett-doffing]

I will keep it around and try to implement a check for a disk space error.

[bryanmontz]

Once you show the photos access system prompt to the user once, it will not show again. This is also true for the GPS location prompt and the notifications prompt. So showing it again is not an option.

The console explains why the app crashes when you try to request .readWrite permission:

"This app has crashed because it attempted to access privacy-sensitive data without a usage description. The app's Info.plist must contain an NSPhotoLibraryUsageDescription key with a string value explaining to the user how the app uses this data."

Overall, this PR seems like a reasonable way to handle it: try to save the image, and report to the user if it fails. We are showing the user the Save Image button in the pop-up menu, and we could already know that it will fail (using PHPhotoLibrary.requestAuthorization(for: .addOnly)). But I don't think we'd change the UI because of that. Still give the user the opportunity to Save Image, and handle the failure.

[bryanmontz]

We could also improve the user experience by sending them to the Damus settings view in Settings.app with this:

UIApplication.shared.open(URL(string: UIApplication.openSettingsURLString)!)

[brett-doffing]

What is unclear to me at this point is whether to implement both the permissions check via PHPhotoLibrary.requestAuthorization(for: .addOnly) and the handling of failures with the ImageSaver class, or to implement one or the other? As it stands currently in this PR, I could get rid of ImageSaver, implement the permissions check alone, and keep the same UI. The reason to keep ImageSaver would then be to handle other failure cases, functionality that did not previously, or currently exist.

[jb55]

On Tue, Aug 01, 2023 at 05:33:53AM -0700, Bryan Montz wrote: We could also improve the user experience by sending them to the Damus settings view in Settings.app with this: ```swift UIApplication.shared.open(URL(string: UIApplication.openSettingsURLString)!) ```

I think this is what I was remembering from other apps, not re-prompting for permissions like you mentioned which is not possible.

[jb55]

what's the status of this? did we come to a conclusion ?