Accept limited photo library authorization

[JadenGeller]

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 return true here instead.

[mazzzystar]

@JadenGeller I've tested your modification, in my case, when I selected limited photos to build index, it will still show -1 photos, and when you tap "Index Photos" button, it will immediately 'build finished', only until you search the next time, it ask you to build index again, then it shows the number of selected photos.

So maybe we need to refresh the photo assets instead of direcetly return true.

[JadenGeller]

I'll take another stab at this and see if I can fix that!

[JadenGeller]

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 photoLibraryDidChange is being called when photo selection is updated, but I think the problem is this path doesn't call into PhotoSearcher.fetchPhotos, which calls fetchUnIndexedPhotos.

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?

[mazzzystar]

@JadenGeller The bug happens in this situation:

• I deleted the app, and re-install in Xcode.
• When I tap Start, it asked request for photos, and I granted limited photo permission, and seleted a few photos, and tap "Done" in top right button.
• It shows -1 photos, and I tap build index, it finished immediately, and back to search page.
• I search a new query, it shows the build index introduction again, and when I tap start, this time I can see the number of phtoos.

I'm on iPhone 12 mini, iOS 17

https://github.com/mazzzystar/Queryable/assets/6824141/4a9dde71-ffa6-4242-a263-cf304d7f2525

[JadenGeller]

Got it! I think my issue w/ reproducing is the photo permission is not being cleared, so it asks on app launch (vs. on start press). I deleted app and reinstalled via Xcode, but it doesn't seem like that's enough to clear the permissions?

I can probably just use the simulator to debug this issue anyway, so I'm not blocked. I am curious how to fully clear out photo permissions though. I'm surprised that uninstalling doesn't do work.

[mazzzystar]

@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? the fetchPhotos code is here: https://github.com/mazzzystar/Queryable/blob/c614f68d6844d15d3540f3ce43d2f36018c1ae79/Queryable/Queryable/ViewModel/PhotoSearcher.swift#L119-L151

[mazzzystar]

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.

[JadenGeller]

Opening the app and immediately popping up a photo request might not be a good idea.

I think you may be misunderstanding the behavior here:

1. If the user has never used the app before, photo permissions will not be requested until fetchPhotos is called, i.e. when "Get Started" is pressed.
2. If the user has granted limited access before, iOS "automatically prompts the user to update their limited-library selection once per app life cycle".

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 presentLimitedLibraryPicker.

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:

• Where should we add this button? To the toolbar? Under the search bar? Hidden in settings? Note that it should only be visible for users who have granted limited access.
• Is it a requirement that the button correctly reacts to permission changes via Settings.app. Technically, it should show/hide based on changes elsewhere, but this adds complexity, and it's not clear it is worth handling that edge case properly.
• We'll have to call presentLimitedLibraryPicker with a UIViewController as argument. I think we can do this in SwiftUI using UIViewControllerRepresentable, but it wasn't designed for SwiftUI, so it may take some figuring out.

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.)

[mazzzystar]

I've tested on my iPhone 12 mini real phone and you're right. After granted full access and deleted the app and re-install, it will not pop up prompt. So Apple remember my last choice and pop up prompt if I've granted limited permission.

Then, I think adding the setting below would be nice.

Where to add the button that invokes presentLimitedLibraryPicker?

How about here?

When user has granted limited access, this place will not show anything currently (as no new photos), so we could show add more photos(or similar) hint, and when he tap the link, he can add more photos.

[JadenGeller]

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!

https://github.com/mazzzystar/Queryable/assets/4544152/138f9f1d-9aa2-4c2f-a1a0-180ccb31c8ab

[mazzzystar]

@JadenGeller Wow! That's nice. And it's really a good idea to keep text unchanged.

[JadenGeller]

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.

[mazzzystar]

Okay, I'm testing your PR code on my local devices right now.

[mazzzystar]

I found one issue: I granted limited permission, selected 10 photos, and built an index, then searched. In the Xcode log, the sim score for every photo with my query, a man with a dog, is the same. This causes the top1 result to change every time I search using the same query.

Searching...
Has indexed data, now begin to search.
Test if I can fetch all photos: 10
0 keys in savedEmbedding has been deleted.
1.2993812561035156e-05 seconds used for save the updated embedding to file.
Searching query = A man with a dog
-0.18408203 0.24230957 -0.23254395 0.13256836 0.09765625 0.11328125 -0.016448975 -0.7998047 -0.1928711 0.48901367 -0.43969727 -0.19067383 0.1361084 0.0793457 0.25952148 -0.021911621 0.18896484 -0.09887695 -0.18859863 -0.003112793 0.5371094 0.52197266 0.17871094 0.11853027 ....

Total 6 pieces.
0
2
2
2
2
2
0.0071489810943603516 seconds used for calculat sim between 10 embs.
2.4080276489257812e-05 seconds used for find top10 sim in 10 scores.
13F7ABE4-8E6D-48AB-9502-F5D1A6BA3475/L0/001 1.1406965
5EEE6D27-CFB4-482B-8C6D-7984745C717A/L0/001 1.1406965
AA566CEF-6334-42F7-B7AD-4E1C3830F854/L0/001 1.1406965
6A6C6EE9-B2D9-4177-9CE6-1D865CCECFFB/L0/001 1.1406965
EF5B5935-5CFF-4AFF-B288-6EA8D72F963B/L0/001 1.1406965
5974DEAF-31FA-4992-815D-710520DEA6C8/L0/001 1.1406965
FDEE6B27-EECD-4289-B438-2159B8996449/L0/001 1.1406965
D53CD54B-F9F9-4103-BFCB-50AC8459DA86/L0/001 1.1406965
A142C8A7-D5DF-4250-BBA6-AAA436855856/L0/001 1.1406965
46D19F68-E83A-4D93-845E-822B38B027A2/L0/001 1.1406965
0.03130996227264404 seconds used for download top10 sim images.
Could not open PLPositionalImageTable at path /var/mobile/Media/PhotoData/Thumbnails/4531.ithmb: Operation not permitted (1)
Could not open PLPositionalImageTable at path /var/mobile/Media/PhotoData/Thumbnails/4531.ithmb: Operation not permitted (1)
Could not open PLPositionalImageTable at path /var/mobile/Media/PhotoData/Thumbnails/4531.ithmb: Operation not permitted (1)

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.

[JadenGeller]

Oh! I wonder why that is. Please don't rush to debug! This can wait to merge. Debug whenever is convenient to you 😄

[JadenGeller]

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.

[mazzzystar]

Oh, I fout the saved embedding for each photo is all the same:

0.0005530118942260742 seconds used for loading embeddingData
Optional(Float32 1 × 512 matrix
[10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10])
Optional(Float32 1 × 512 matrix
[10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10

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 : )

[mazzzystar]

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.

Queryable resizes the input photos to 224x224, but some photos are either too large (can't be loaded) or too small (like 128x128), or have a strange shape (like long screenshots), so sometimes the preprocessing code may fail. In such cases, I'm using an SF symbol to replace its value, which is a compromise. https://github.com/mazzzystar/Queryable/blob/c614f68d6844d15d3540f3ce43d2f36018c1ae79/Queryable/Queryable/Model/UIImage%2BExtension.swift#L13-L20

[mazzzystar]

I believe I've identified the cause of the issue. When I set the photo library access to .limited and grant access to, say, 10 photos, as I build the index for these 10 photos, each photo's _curIndexingPhoto value turns out to be null.

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 MLMultiArray is being used instead,

_emb![key] = 10.0

leading to the logged message here.

I've also found error in Xcode:

Could not open PLPositionalImageTable at path 
/var/mobile/Media/PhotoData/Thumbnails/4531.ithmb: Operation not permitted (1)

This suggests that when switching to .limited permission, the photoCollection.cache request is also affected, preventing the use of thumbnails. Since the CachedImageManager.swift code (originally from Apple's documentation) involves aspects of the Photos library and its caching system that I'm not very familiar with, I'll need to investigate further to fully understand why this is happening.

[mazzzystar]

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:

https://github.com/mazzzystar/Queryable/blob/159a58cde82e4f4137ade386593ae7c3aa74c95a/Queryable/Queryable/PhotoHelper/CachedImageManager.swift#L59-L81

I could not handle this issue, so sorry for currently not able to merge your PR, hope other contributors(maybe @yujinqiu ?) could fix this.

[JadenGeller]

I don't intend to continue to debug this, so I'm going to close, sorry