refactor(mobile): cleanup image request pipeline on iOS #27481
refactor(mobile): cleanup image request pipeline on iOS #27481LeLunZ wants to merge 4 commits intobugfix-27365from
Conversation
…cancellation logic
|
Label error. Requires exactly 1 of: changelog:.*. Found: 📱mobile. A maintainer will add the required label. |
1 similar comment
|
Label error. Requires exactly 1 of: changelog:.*. Found: 📱mobile. A maintainer will add the required label. |
LeLunZ
changed the title
mertalev
left a comment
mertalev
left a comment
There was a problem hiding this comment.
This is making a lot of sweeping changes and the abstraction makes it harder to see that the behavior is the same. I'd prefer smaller, more targeted PRs for this code.
| if request.isCancelled { | ||
| return completion(ImageProcessing.cancelledResult) | ||
| } | ||
| if request.isCancelled { return } |
There was a problem hiding this comment.
Where does it respond to Pigeon in this case?
There was a problem hiding this comment.
Always in the cancel method of the request.
| if let cached = assetCache.object(forKey: assetId as NSString) { | ||
| return cached | ||
| } | ||
|
|
||
| guard let asset = PHAsset.fetchAssets(withLocalIdentifiers: [assetId], options: Self.fetchOptions).firstObject | ||
| else { return nil } | ||
| assetQueue.async { assetCache.setObject(asset, forKey: assetId as NSString) } | ||
| assetCache.setObject(asset, forKey: assetId as NSString) | ||
| return asset |
There was a problem hiding this comment.
The dictionary isn't thread-safe.
There was a problem hiding this comment.
Isn’t the NSCache thread save, the docs are linked above?
| lock.lock() | ||
| defer { lock.unlock() } |
| return _isCancelled | ||
| } | ||
|
|
||
| init(callback: @escaping (Result<[String: Int64]?, any Error>) -> Void) { |
There was a problem hiding this comment.
This probably shouldn't be nullable.
|
Will see what I can do about splitting this into smaller PRs. |
2 participants
Description
While looking into #27471 I saw some opportunities to abstract a few things, which I think makes this a bit easier to work with.
Fixes:
Improvement:
isCancelledand has to exist, nothing more.Refactoring:
@mertalev if you find the UnfairLock unnecessary I can also remove it, I just included it because I found references which said not to use one 10 years ago :)
How Has This Been Tested?
Checklist:
src/services/uses repositories implementations for database calls, filesystem operations, etc.src/repositories/is pretty basic/simple and does not have any immich specific logic (that belongs insrc/services/)Please describe to which degree, if any, an LLM was used in creating this pull request.
Claude Code did the UnfairLock implementation, and part of this description.