-
Notifications
You must be signed in to change notification settings - Fork 416
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
Accept limited photo library authorization #22
Conversation
@JadenGeller I've tested your modification, in my case, when I selected limited photos to build index, it will still show So maybe we need to refresh the photo assets instead of direcetly |
I'll take another stab at this and see if I can fix that! |
Hmm, I'm having trouble reproducing exactly what you're seeing. Here's a bug I can repro tho: When I select additional photos, the app doesn't offer to update the index until after I quit the app and relaunch it. (Funnily enough, Instagram also has this issue, where newly selected photos don't show up until the app is relaunched.) It seems like I unfortunately don't feel familiar enough with the state machine to fix this bug though. The one you mentioned might be easier to fix, but I can't repro it. On first launched, the app immediately asks me to select photos, and it seems to detect those when I navigate to the index page. Is the bug more subtle? |
@JadenGeller
I'm on limitedPhotos.mov |
switch PHPhotoLibrary.authorizationStatus(for: .readWrite) { | ||
case .authorized: | ||
logger.debug("Photo library access authorized.") | ||
return true | ||
case .notDetermined: | ||
logger.debug("Photo library access not determined.") | ||
return await PHPhotoLibrary.requestAuthorization(for: .readWrite) == .authorized | ||
return await checkAuthorization(status: PHPhotoLibrary.requestAuthorization(for: .readWrite)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, I think this fixes the issue! I was only considering limited authorization when checking if already authorized. I also needed to accept limited authorization at the time of request.
Let me know if you want me to restructure this code somehow. I think the recursive nature is a bit weird, but I assume the OS won't return notDetermined
repeatedly. (Maybe they'll add a cancel button in the future that changes that though?)
It's worth noting there's a minor logging behavior change here. Previously, you wouldn't log the new status after requesting authorization. Now, we do.
If you want me to refactor this, let me know how! If logging new status after request isn't your preference, I can just do something simple like [.authorized, .limited].contains(PHPhotoLibrary.requestAuthorization(for: .readWrite))
.
@@ -7,7 +7,7 @@ import os.log | |||
|
|||
class PhotoLibrary { | |||
static func checkAuthorization(status: PHAuthorizationStatus = PHPhotoLibrary.authorizationStatus(for: .readWrite)) async -> Bool { | |||
switch PHPhotoLibrary.authorizationStatus(for: .readWrite) { | |||
switch status { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
whoops almost forgot to swap this out
I think I don't like using the default argument in this way, but waiting for feedback re: logs & style before changing anything
@JadenGeller Opening the app and immediately popping up a photo request might not be a good idea. The previous code could avoid this situation. I think maybe we just need to refetch the photos after selecting limited photos? Queryable/Queryable/Queryable/ViewModel/PhotoSearcher.swift Lines 119 to 151 in c614f68
|
I found, when setting case .limited:
logger.debug("Photo library access limited.")
return true It will cause immediately popping up request when opening the app. |
I think you may be misunderstanding the behavior here:
I believe this is currently working as intended, and we just need to disable this default via Info.plist "Prevent limited photos access alert". I'm happy to disable, but if we do so, we should add a button to the UI that invokes I'd argue we shouldn't block merge on this UI because this is already a strict improvement (the new behavior is only for people who choose to grant limited access), and because implementing a limited library picker correctly is non-trivial. If you'd like, I can investigate implementing a limited library picker. Here are some considerations:
Let me know what you think is the right course of action! (And let me know if you think I'm incorrect about the current behavior. I tested a fresh install of the app in simulator, and it didn't request access on first launch like you described. This only happens on subsequent launches as is system behavior.) |
I implemented the behavior you suggested. I didn't modify the button text both because (a) I didn't want new text to localize and (b) it seemed reasonable enough to keep the current text and just show the photo picker prior to navigation. Let me know if you're happy with that! limited_photo_selection_flow_low_quality.mov |
@JadenGeller Wow! That's nice. And it's really a good idea to keep text unchanged. |
@@ -104,13 +106,17 @@ | |||
94E917D82A5A47A300324937 /* it */ = {isa = PBXFileReference; lastKnownFileType = text.plist.strings; name = it; path = Localizable.strings; sourceTree = "<group>"; }; | |||
94E917DB2A5A47A300324937 /* es */ = {isa = PBXFileReference; lastKnownFileType = text.plist.strings; name = es; path = Localizable.strings; sourceTree = "<group>"; }; | |||
94ECA38E2A5D52050066F64B /* Assets.xcassets */ = {isa = PBXFileReference; lastKnownFileType = folder.assetcatalog; path = Assets.xcassets; sourceTree = "<group>"; }; | |||
D78D1FE42AF073F8004B45E4 /* LimitedLibraryPicker.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = LimitedLibraryPicker.swift; sourceTree = "<group>"; }; | |||
D78D1FE72AF0790F004B45E4 /* PhotosUI.framework */ = {isa = PBXFileReference; lastKnownFileType = wrapper.framework; name = PhotosUI.framework; path = System/Library/Frameworks/PhotosUI.framework; sourceTree = SDKROOT; }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For some reason, "Link Frameworks Automatically" doesn't work in this case.
} | ||
|
||
class Coordinator { | ||
var isPresented = false |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Coordinator tracks whether or not the picker is actually presented. Binding tracks whether or not the view wants it to be presented.
This allows us to: (a) prevent attempting to present the picker again when it is already presented, and (b) update the binding value back to true
if the picker is still presented (since we cannot programmatically dismiss).
Technically, neither of these code paths are exercised by the app, but it seemed worth implementing this properly in case the calling code changes.
|
||
extension View { | ||
func limitedLibraryPicker(isPresented: Binding<Bool>) -> some View { | ||
overlay(LimitedLibraryPicker(isPresented: isPresented)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It doesn't actually matter if this is an overlay, background, etc.
It just matters that the UIViewControllerRepresentable is part of the view hierarchy so it can get a reference to a UIViewController to present the limited library picker.
@@ -64,7 +64,7 @@ struct SearchResultsView: View { | |||
case .HAS_RESULT: | |||
// Has result | |||
VStack { | |||
if photoSearcher.totalUnIndexedPhotosNum > 0 { | |||
if photoSearcher.totalUnIndexedPhotosNum > 0 || PHPhotoLibrary.authorizationStatus(for: .readWrite) == .limited { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Always show the button if the library has a limited authorization status, since it's always true that there may be more photos the user could index.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reasonable.
goToIndexView = true | ||
} | ||
} | ||
.onChange(of: showLimitedLibraryPicker) { showLimitedLibraryPicker in |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a bit of a confusing state machine, but I don't know of a better way to represent this here.
If the authorization is limited, instead of immediately going to index view, show the picker. Then, when the picker is dismissed, refetch the photos and go to the index view.
Let me know if you have further feedback, but otherwise I think this is ready for merge! I'll quickly note that there's a bug when photos are deselected from the library. If they were already visible in the search results, they're replaced with a progress indicator. I don't think this is a big deal, since it's (a) not a common flow and (b) resolves once another search is made, but let me know if there's an easy fix you'd like to see implemented. |
Okay, I'm testing your PR code on my local devices right now. |
I found one issue: I granted limited permission, selected 10 photos, and built an index, then searched. In the Xcode log, the
I think this issue may not be caused by your code. I'll try to debug it tonight. Sorry for not being able to merge your PR at the moment. |
Oh! I wonder why that is. Please don't rush to debug! This can wait to merge. Debug whenever is convenient to you 😄 |
This is probably not the issue, but I noticed this line of code: self.curIndexingPhoto = _curIndexingPhoto ?? UIImage(systemName: "photo") I'm wondering why it embeds an SF symbol when the photo doesn't load synchronously as expected. It may be better to crash in this case (or at least log so this condition can be noticed). It's probably not relevant here (unless limited access libraries call the completion handler async where it would otherwise be called sync), but I thought I'd mention just in case. |
Oh, I fout the saved
So, there must be something wrong with the index building in limited photo mode. I'll be out soon and will debug it when I get back tonight : ) |
Queryable resizes the input photos to Queryable/Queryable/Queryable/Model/UIImage+Extension.swift Lines 13 to 20 in c614f68
|
I believe I've identified the cause of the issue. When I set the photo library access to do {
self.curIndexingPhoto = _curIndexingPhoto ?? UIImage(systemName: "photo")! // <---- This line
if let _curIndexingPhoto {
let img_emb = try await self.imageEncoder?.encode(image: _curIndexingPhoto)
self.buildingEmbedding[asset.id] = MLMultiArray(img_emb!)
} else {
self.buildingEmbedding[asset.id] = MLMultiArray(defaultEmbedding())
}
} So, the default _emb![key] = 10.0 leading to the logged message here. I've also found error in Xcode:
This suggests that when switching to |
Hi @JadenGeller I'm not sure why but I've tested a few times on my phone, after I selected 3 photos and start to build index, It could not successfully load the thumbnails, which cause the bug above. This part code is: Queryable/Queryable/Queryable/PhotoHelper/CachedImageManager.swift Lines 59 to 81 in 159a58c
I could not handle this issue, so sorry for currently not able to merge your PR, hope other contributors(maybe @yujinqiu ?) could fix this. |
I don't intend to continue to debug this, so I'm going to close, sorry |
I have a huge photo library, so I granted limited access to only a subset of the photos. The app silently failed to index (showing a count of -1 photos). I debugged this, and it seems the issue is that a limited authorization was considered not authorized, so
fetchPhotos
returns early. It seems to fix the bug when I returntrue
here instead.