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

MediaUIView - Fix image download bug (#2131) #2215

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

Conversation

Taufi
Copy link
Contributor

@Taufi Taufi commented Oct 23, 2024

If the user has not yet granted permission to download photos, they will be asked to do so: PHPhotoLibrary.requestAuthorization(for: .addOnly). Afterward, the photos can also be downloaded to the desktop. Previously, broader access was granted (requestAuthorization(_:)), but this method is now deprecated. Apparently, Mac Catalyst support for more advanced permission checks is still not fully developed. Once permission is denied, it can no longer be granted in the settings. You have to do it via a terminal command: 'tccutil reset PhotosAdd <your.app.bundle.identifier>'.

@@ -180,11 +183,19 @@ private struct SavePhotoToolbarItem: ToolbarContent, @unchecked Sendable {
}
return nil
}


private func saveImage(url: URL) async -> Bool {

Choose a reason for hiding this comment

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

Is it possible to change the var status into a let and use a switch case? Or is the if statement necessary after the requestAuthorization for addOnly? I had something in mind like this:

private func saveImage(url: URL) async -> Bool {
        guard let image = try? await uiimageFor(url: url) else { return false }

        let status: PHAuthorizationStatus
        switch PHPhotoLibrary.authorizationStatus(for: .addOnly) {
        case .authorized:
            UIImageWriteToSavedPhotosAlbum(image, nil, nil, nil)
        default:
            await PHPhotoLibrary.requestAuthorization(for: .addOnly)
            status = PHPhotoLibrary.authorizationStatus(for: .addOnly)
        }
        return true
    }

Keeps it more readable without the if statements

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you, D. Prodano, for your help. The guard statement is definitely a better solution than my if clause. Regarding the other if statements, my intention was to initiate the image download immediately after requesting authorization. Otherwise, the user would have to manually restart the download after granting authorization. What are your thoughts on this approach?

@@ -1545,6 +1545,7 @@
INFOPLIST_KEY_LSApplicationCategoryType = "public.app-category.social-networking";
INFOPLIST_KEY_NSCameraUsageDescription = "Upload photos & videos to attach to your Mastodon posts.";
INFOPLIST_KEY_NSHumanReadableCopyright = "© 2024 Thomas Ricouard";
INFOPLIST_KEY_NSPhotoLibraryAddUsageDescription = "";

Choose a reason for hiding this comment

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

Are we going to show empty description?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for your question, Ankush.

When I leave the description empty, the pop-up displays the standard message: “‘Ice Cubes’ would like to add photos.” To me, this is sufficient, as the user has just pressed the button to download the photo, so everything should be clear.

simple_access_request

However, when I add a description, the pop-up feels somewhat redundant:

Simple_access_query_with_explanation

What would you prefer?

Copy link

@ankushkushwaha ankushkushwaha Nov 14, 2024

Choose a reason for hiding this comment

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

Okay thanks for the explanation, Its better to have some description for the user.

@Taufi
Copy link
Contributor Author

Taufi commented Nov 14, 2024

I worked on improving the PR based on the reviewers' recommendations:

First, I updated the saveImage(url:) function in MediaUIView.swift as recommended by D. Prodano. I kept the second if clause to initiate the image download immediately after requesting authorization. I can change this if preferred, but it would then require the user to manually restart the download after granting authorization.

Additionally, I added the NSPhotoLibraryAddUsageDescription, following Ankush's recommendation.

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.

3 participants