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.

Sign up for GitHub

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
+19 −2
Open

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

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
fd88496
fix image download bug (#2131)
Taufi Oct 22, 2024
49899cc
Improved PR following reviewer recommendations
Taufi Nov 14, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions IceCubesApp.xcodeproj/project.pbxproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 = "";
Copy link

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 •
edited
Loading

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.

INFOPLIST_KEY_NSPhotoLibraryUsageDescription = "Upload photos & videos to Mastodon";
INFOPLIST_KEY_UIApplicationSceneManifest_Generation = YES;
"INFOPLIST_KEY_UIApplicationSceneManifest_Generation[sdk=iphoneos*]" = YES;
Expand Down Expand Up @@ -1611,6 +1612,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 = "";
INFOPLIST_KEY_NSPhotoLibraryUsageDescription = "Upload photos & videos to Mastodon";
INFOPLIST_KEY_UIApplicationSceneManifest_Generation = YES;
"INFOPLIST_KEY_UIApplicationSceneManifest_Generation[sdk=iphoneos*]" = YES;
Expand Down
2 changes: 2 additions & 0 deletions IceCubesApp/App/IceCubesApp-release.entitlements
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,8 @@
<true/>
<key>com.apple.security.network.client</key>
<true/>
<key>com.apple.security.personal-information.photos-library</key>
<true/>
<key>keychain-access-groups</key>
<array>
<string>$(AppIdentifierPrefix)$(BUNDLE_ID_PREFIX).IceCubesApp</string>
Expand Down
2 changes: 2 additions & 0 deletions IceCubesApp/App/IceCubesApp.entitlements
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,8 @@
<true/>
<key>com.apple.security.network.client</key>
<true/>
<key>com.apple.security.personal-information.photos-library</key>
<true/>
<key>keychain-access-groups</key>
<array>
<string>$(AppIdentifierPrefix)$(BUNDLE_ID_PREFIX).IceCubesApp</string>
Expand Down
17 changes: 14 additions & 3 deletions Packages/MediaUI/Sources/MediaUI/MediaUIView.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ import Models
import Nuke
import QuickLook
import SwiftUI
import Photos

public struct MediaUIView: View, @unchecked Sendable {
private let data: [DisplayData]
Expand Down Expand Up @@ -144,6 +145,8 @@ private struct SavePhotoToolbarItem: ToolbarContent, @unchecked Sendable {
state = .unsaved
}
}
} else {
state = .unsaved
}
}
} label: {
Expand Down Expand Up @@ -180,11 +183,19 @@ private struct SavePhotoToolbarItem: ToolbarContent, @unchecked Sendable {
}
return nil
}

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

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?

var status = PHPhotoLibrary.authorizationStatus(for: .addOnly)

if let image = try? await uiimageFor(url: url) {
UIImageWriteToSavedPhotosAlbum(image, nil, nil, nil)
return true
if status != .authorized {
await PHPhotoLibrary.requestAuthorization(for: .addOnly)
status = PHPhotoLibrary.authorizationStatus(for: .addOnly)
}
if status == .authorized {
UIImageWriteToSavedPhotosAlbum(image, nil, nil, nil)
return true
}
}
return false
}
Expand Down
Loading