1and2papa / CTAssetsPickerController

iOS control that allows picking multiple photos and videos from user's photo library.
MIT License
2.15k stars 550 forks source link

showsEmptyAlbums = NO not respected #174

Closed germs5 closed 8 years ago

germs5 commented 8 years ago

I seem to be displaying all assetCollections, even empty ones, even when showsEmptyAlbums is set to NO. If I change CTAssetCollectionViewController:204 from: NSInteger count = (assetCollection.estimatedAssetCount) ? assetCollection.estimatedAssetCount : 0; to: NSUInteger count = (assetCollection.estimatedAssetCount != NSNotFound) ? assetCollection.estimatedAssetCount : 0; the problem seems to go away.

Sega-Zero commented 8 years ago

Seems like assetCollection.estimatedAssetCount may return NSNotFound even when there are some assets in the collection. Facing this behavior on ios 9.1 simulator

germs5 commented 8 years ago

I agree that assetCollection.estimatedAssetCount skips too many collections as empty, even on actual devices. A better metric is needed. But without the code change at the the top of this issue, there is no difference between showsEmptyAlbums = NO and showsEmptyAlbums = YES.

1and2papa commented 8 years ago

Hi both,

Photos.framework is complex yet not has many ways to deal with it. estimatedAssetCount is not reliable as I mentioned in https://github.com/chiunam/CTAssetsPickerController/pull/148#issuecomment-141910188.

From my observation, it returns the count asynchronously. It first returns NSNotFound when the count is not ready and will return an estimated number after several seconds. So it might be the reason of skipping asset collections as empty. Using it to check whether the album is empty was a performance trade-off as described in the above issue.

Given the existing APIs, it seems that there is no a quick way to retrieve the current number of assets in the collection. And it goes more complex if you filter the assets by assigning assetsFetchOptions.

To hide empty albums (The albums really contains nothing in it, NOT nothing after filtering assets), the following code should works:-

 // create options for fetching non-empty albums
PHFetchOptions *assetCollectionFetchOptions = [PHFetchOptions new];
assetCollectionFetchOptions.predicate = [NSPredicate predicateWithFormat:@"estimatedAssetCount > 0"];

// assign options
picker.assetCollectionFetchOptions = assetCollectionFetchOptions;

Please do let me know if it works properly. I will then update the picker.

Sega-Zero commented 8 years ago

Tried what you suggest, all the albums are now hidden (due to NSNotFound i guess). In #175 I've tried to do count checking requests quick enough by setting fetchLimit to 1 and haven't noticed any slowdowns.

1and2papa commented 8 years ago

@Sega-Zero Oh, I missed your PR. That's brilliant move indeed. I just thinking if it is good to drop support of iOS 8 as fetchLimit is iOS 9 only.

Sega-Zero commented 8 years ago

Missed that point. Hmm. Well then, there is nothing to do then to put a define over this line, dropping iOS 8 support is not an option for this year, maybe when iOS10 will be released =) Is there anything that can be done to fix the problem?

germs5 commented 8 years ago

Not working properly. It behaves the same assetCollection.estimatedAssetCount != NSNotFound. That is, too many collections are skipped. After all, it is still evaluating estimatedAssetCount, just in a predicate.

Sega-Zero commented 8 years ago

Seems like fetchLimit exists and working in iOS8.3+, just wasn't on SDK yet. Lower versions are throwing NSInvalidArgumentException

changed #175 to respect setFetchLimit: selector existence

1and2papa commented 8 years ago

@Sega-Zero Great. Will merge your PR. @germs5 Sega-Zero's solution should fix the bug. It will be updated in the next release.

germs5 commented 8 years ago

@Sega-Zero @chiunam using respondsToSelector is reasonable, but only if there is also a property in the library like: @property (nonatomic, readonly) BOOL supportsSkippingEmptyAlbums; whose implementation performs your respondsToSelector. Then my client code can decide to skip empty albums when it will be calculated correctly, and to show empty albums otherwise, with, say: picker.showsEmptyAlbums = !picker. supportsSkippingEmptyAlbums;

Sega-Zero commented 8 years ago

@germs5 fetchLimit is just for code speedup, it will still work without it, just a little bit slower

1and2papa commented 8 years ago

@Sega-Zero is right.

germs5 commented 8 years ago

So, is @Sega-Zero 's pull request going to be merged into master any time soon, or should I get the fix from his fork?

1and2papa commented 8 years ago

@germs5, sorry for being late. The PR has been merged into master. Cocoapod version will be updated soon.

1and2papa commented 8 years ago

@Sega-Zero I found that the estimatedAssetCount cannot reflect the count after filtering assets so I decide to remove the code of it and I will modify your code a bit. Hope you don't mind.

Sega-Zero commented 8 years ago

not at all :) waiting for cocoapod update to switch on the main repo =)