Bgerstle has uploaded a new change for review. https://gerrit.wikimedia.org/r/226233
Change subject: fix list crashes, history saves from search, & KVO cycles ...................................................................... fix list crashes, history saves from search, & KVO cycles History list controller was causing crashes when getting KVO callbacks before the collectionView was loaded. Search viewController wasn't configuring its results controller w/ saved page list or recent page list, so history updates weren't occurring from the search view. Broke cycles between self -> KVOController -> observationBlock -> self by using KVOControllerUnretained (doesn't retain self) and/or using the "observer" argument passed to the block to avoid referencing self. Bug: T106420 Change-Id: I404db20c89225763f683282fd790ed8baeec36f3 --- M MediaWikiKit/MediaWikiKit/MWKList.m M Wikipedia/UI-V5/WMFAppViewController.m M Wikipedia/UI-V5/WMFArticleListCollectionViewController.m M Wikipedia/UI-V5/WMFRecentPagesDataSource.m M Wikipedia/UI-V5/WMFSavedPagesDataSource.m M Wikipedia/UI-V5/WMFSearchViewController.m 6 files changed, 77 insertions(+), 50 deletions(-) git pull ssh://gerrit.wikimedia.org:29418/apps/ios/wikipedia refs/changes/33/226233/1 diff --git a/MediaWikiKit/MediaWikiKit/MWKList.m b/MediaWikiKit/MediaWikiKit/MWKList.m index 1f2a743..fb9757a 100644 --- a/MediaWikiKit/MediaWikiKit/MWKList.m +++ b/MediaWikiKit/MediaWikiKit/MWKList.m @@ -136,7 +136,6 @@ } - (void)performSaveWithCompletion:(dispatch_block_t)completion error:(WMFErrorHandler)errorHandler { - //nonop if (errorHandler) { errorHandler([NSError wmf_unableToSaveErrorWithReason:@"Save is unimplemented for this list"]); } diff --git a/Wikipedia/UI-V5/WMFAppViewController.m b/Wikipedia/UI-V5/WMFAppViewController.m index 4380f7c..5a5b2af 100644 --- a/Wikipedia/UI-V5/WMFAppViewController.m +++ b/Wikipedia/UI-V5/WMFAppViewController.m @@ -64,12 +64,18 @@ - (void)configureSavedViewController { [self configureArticleListController:self.savedArticlesViewController]; - self.savedArticlesViewController.dataSource = [[WMFSavedPagesDataSource alloc] initWithSavedPagesList:[self userDataStore].savedPageList]; + if (!self.savedArticlesViewController.dataSource) { + self.savedArticlesViewController.dataSource = + [[WMFSavedPagesDataSource alloc] initWithSavedPagesList:[self userDataStore].savedPageList]; + } } - (void)configureRecentViewController { [self configureArticleListController:self.recentArticlesViewController]; - self.recentArticlesViewController.dataSource = [[WMFRecentPagesDataSource alloc] initWithRecentPagesList:[self userDataStore].historyList]; + if (!self.recentArticlesViewController.dataSource) { + self.recentArticlesViewController.dataSource = + [[WMFRecentPagesDataSource alloc] initWithRecentPagesList:[self userDataStore].historyList]; + } } #pragma mark - Public diff --git a/Wikipedia/UI-V5/WMFArticleListCollectionViewController.m b/Wikipedia/UI-V5/WMFArticleListCollectionViewController.m index 6ec6410..e50f8b4 100644 --- a/Wikipedia/UI-V5/WMFArticleListCollectionViewController.m +++ b/Wikipedia/UI-V5/WMFArticleListCollectionViewController.m @@ -89,6 +89,22 @@ #pragma mark - Accessors +#define WMFWarnIfNilOnReturn(prop, type) \ + - (type*)prop { \ + if (!_ ## prop) { DDLogWarn(@"%@ not configured with " #prop "!", [self debugDescription]); } \ + return _ ## prop; \ + } + +WMFWarnIfNilOnReturn(dataStore, MWKDataStore) + +WMFWarnIfNilOnReturn(recentPages, MWKHistoryList) + +WMFWarnIfNilOnReturn(savedPages, MWKSavedPageList) + +- (NSString*)debugDescription { + return [NSString stringWithFormat:@"%@ dataSourceClass: %@", self, [self.dataSource class]]; +} + - (TGLStackedLayout*)stackedLayout { if (!_stackedLayout) { TGLStackedLayout* layout = [[TGLStackedLayout alloc] init]; @@ -129,21 +145,9 @@ [self updateListForMode:self.mode animated:NO completion:NULL]; } -- (void)viewWillAppear:(BOOL)animated { - [super viewWillAppear:animated]; -} - -- (void)viewDidAppear:(BOOL)animated { - [super viewDidAppear:animated]; -} - - (void)viewDidLayoutSubviews { [super viewDidLayoutSubviews]; [self updateCellSizeBasedOnViewFrame]; -} - -- (BOOL)shouldAutorotate { - return YES; } - (NSUInteger)supportedInterfaceOrientations { @@ -191,7 +195,8 @@ if (cell.viewController == nil) { WMFArticleViewController* vc = - [WMFArticleViewController articleViewControllerWithDataStore:self.dataStore savedPages:self.savedPages]; + [WMFArticleViewController articleViewControllerWithDataStore:self.dataStore + savedPages:self.savedPages]; [vc setMode:WMFArticleControllerModeList animated:NO]; [cell setViewControllerAndAddViewToContentView:vc]; } @@ -243,7 +248,8 @@ [[UIApplication sharedApplication] sendAction:@selector(resignFirstResponder) to:nil from:nil forEvent:nil]; [self presentViewController:vc animated:YES completion:^{ - [self.recentPages addPageToHistoryWithTitle:cell.viewController.article.title discoveryMethod:[self.dataSource discoveryMethod]]; + [self.recentPages addPageToHistoryWithTitle:cell.viewController.article.title + discoveryMethod:[self.dataSource discoveryMethod]]; [self.recentPages save]; }]; } @@ -269,52 +275,56 @@ #pragma mark - DataSource KVO - (void)observeDataSource { - [self.KVOController observe:_dataSource - keyPath:WMF_SAFE_KEYPATH(_dataSource, articles) - options:0 - block:^(id observer, id object, NSDictionary* change) { + [self.KVOControllerNonRetaining observe:_dataSource + keyPath:WMF_SAFE_KEYPATH(_dataSource, articles) + options:0 + block:^(id observer, id object, NSDictionary* change) { NSKeyValueChange changeKind = [change[NSKeyValueChangeKindKey] unsignedIntegerValue]; NSArray* indexPaths = indexPathsWithIndexSet(change[NSKeyValueChangeIndexesKey], 0); - [self updateCellsAtIndexPaths:indexPaths change:changeKind]; + [observer updateCellsAtIndexPaths:indexPaths change:changeKind]; }]; } - (void)unobserveDataSource { - [self.KVOController unobserve:_dataSource]; + [self.KVOControllerNonRetaining unobserve:_dataSource]; } #pragma mark - Process DataSource Changes - (void)updateCellsAtIndexPaths:(NSArray*)indexPaths change:(NSKeyValueChange)change { - [self.collectionView performBatchUpdates:^{ - switch (change) { - case NSKeyValueChangeInsertion: - [self insertCellsAtIndexPaths:indexPaths]; - break; - case NSKeyValueChangeRemoval: - [self deleteCellsAtIndexPaths:indexPaths]; - break; - case NSKeyValueChangeReplacement: - [self reloadCellsAtIndexPaths:indexPaths]; - break; - case NSKeyValueChangeSetting: - [self.collectionView reloadData]; - break; - default: - break; - } - } completion:NULL]; + if (![self isViewLoaded]) { + return; + } + switch (change) { + case NSKeyValueChangeInsertion: + [self insertCellsAtIndexPaths:indexPaths]; + break; + case NSKeyValueChangeRemoval: + [self deleteCellsAtIndexPaths:indexPaths]; + break; + case NSKeyValueChangeReplacement: + [self reloadCellsAtIndexPaths:indexPaths]; + break; + case NSKeyValueChangeSetting: + [self.collectionView reloadData]; + break; + default: + break; + } } - (void)insertCellsAtIndexPaths:(NSArray*)indexPaths { + NSParameterAssert(self.isViewLoaded); [self.collectionView insertItemsAtIndexPaths:indexPaths]; } - (void)deleteCellsAtIndexPaths:(NSArray*)indexPaths { + NSParameterAssert(self.isViewLoaded); [self.collectionView deleteItemsAtIndexPaths:indexPaths]; } - (void)reloadCellsAtIndexPaths:(NSArray*)indexPaths { + NSParameterAssert(self.isViewLoaded); [self.collectionView reloadItemsAtIndexPaths:indexPaths]; } diff --git a/Wikipedia/UI-V5/WMFRecentPagesDataSource.m b/Wikipedia/UI-V5/WMFRecentPagesDataSource.m index 99bbd05..b14d558 100644 --- a/Wikipedia/UI-V5/WMFRecentPagesDataSource.m +++ b/Wikipedia/UI-V5/WMFRecentPagesDataSource.m @@ -20,13 +20,19 @@ self = [super init]; if (self) { self.recentPages = recentPages; - [self.KVOController observe:recentPages keyPath:WMF_SAFE_KEYPATH(recentPages, entries) options:NSKeyValueObservingOptionPrior block:^(id observer, id object, NSDictionary* change) { + [self.KVOControllerNonRetaining observe:recentPages + keyPath:WMF_SAFE_KEYPATH(recentPages, entries) + options:NSKeyValueObservingOptionPrior + block:^(id observer, id object, NSDictionary* change) { NSKeyValueChange changeKind = [change[NSKeyValueChangeKindKey] unsignedIntegerValue]; - if ([change[NSKeyValueChangeNotificationIsPriorKey] boolValue]) { - [self willChange:changeKind valuesAtIndexes:change[NSKeyValueChangeIndexesKey] forKey:@"articles"]; + [observer willChange:changeKind + valuesAtIndexes:change[NSKeyValueChangeIndexesKey] + forKey:WMF_SAFE_KEYPATH(((WMFRecentPagesDataSource*)observer), articles)]; } else { - [self didChange:changeKind valuesAtIndexes:change[NSKeyValueChangeIndexesKey] forKey:@"articles"]; + [observer didChange:changeKind + valuesAtIndexes:change[NSKeyValueChangeIndexesKey] + forKey:WMF_SAFE_KEYPATH(((WMFRecentPagesDataSource*)observer), articles)]; } }]; } diff --git a/Wikipedia/UI-V5/WMFSavedPagesDataSource.m b/Wikipedia/UI-V5/WMFSavedPagesDataSource.m index 421e990..d43fbc9 100644 --- a/Wikipedia/UI-V5/WMFSavedPagesDataSource.m +++ b/Wikipedia/UI-V5/WMFSavedPagesDataSource.m @@ -20,13 +20,17 @@ self = [super init]; if (self) { self.savedPages = savedPages; - [self.KVOController observe:savedPages keyPath:WMF_SAFE_KEYPATH(savedPages, entries) options:NSKeyValueObservingOptionPrior block:^(id observer, id object, NSDictionary* change) { + [self.KVOControllerNonRetaining observe:savedPages keyPath:WMF_SAFE_KEYPATH(savedPages, entries) options:NSKeyValueObservingOptionPrior block:^(id observer, id object, NSDictionary* change) { NSKeyValueChange changeKind = [change[NSKeyValueChangeKindKey] unsignedIntegerValue]; if ([change[NSKeyValueChangeNotificationIsPriorKey] boolValue]) { - [self willChange:changeKind valuesAtIndexes:change[NSKeyValueChangeIndexesKey] forKey:@"articles"]; + [observer willChange:changeKind + valuesAtIndexes:change[NSKeyValueChangeIndexesKey] + forKey:WMF_SAFE_KEYPATH(((WMFSavedPagesDataSource*)observer), articles)]; } else { - [self didChange:changeKind valuesAtIndexes:change[NSKeyValueChangeIndexesKey] forKey:@"articles"]; + [observer didChange:changeKind + valuesAtIndexes:change[NSKeyValueChangeIndexesKey] + forKey:WMF_SAFE_KEYPATH(((WMFSavedPagesDataSource*)observer), articles)]; } }]; } diff --git a/Wikipedia/UI-V5/WMFSearchViewController.m b/Wikipedia/UI-V5/WMFSearchViewController.m index 7f8af93..b641f36 100644 --- a/Wikipedia/UI-V5/WMFSearchViewController.m +++ b/Wikipedia/UI-V5/WMFSearchViewController.m @@ -79,7 +79,7 @@ - (void)observeSavedPages { [self.KVOController observe:self.userDataStore.savedPageList keyPath:WMF_SAFE_KEYPATH(self.userDataStore.savedPageList, entries) options:0 block:^(id observer, id object, NSDictionary* change) { - [self.resultsListController refreshVisibleCells]; + [observer refreshVisibleCells]; }]; } @@ -98,7 +98,9 @@ - (void)prepareForSegue:(UIStoryboardSegue*)segue sender:(id)sender { if ([segue.destinationViewController isKindOfClass:[WMFArticleListCollectionViewController class]]) { - self.resultsListController = segue.destinationViewController; + self.resultsListController = segue.destinationViewController; + self.resultsListController.savedPages = self.userDataStore.savedPageList; + self.resultsListController.recentPages = self.userDataStore.historyList; } if ([segue.destinationViewController isKindOfClass:[RecentSearchesViewController class]]) { self.recentSearchesViewController = segue.destinationViewController; -- To view, visit https://gerrit.wikimedia.org/r/226233 To unsubscribe, visit https://gerrit.wikimedia.org/r/settings Gerrit-MessageType: newchange Gerrit-Change-Id: I404db20c89225763f683282fd790ed8baeec36f3 Gerrit-PatchSet: 1 Gerrit-Project: apps/ios/wikipedia Gerrit-Branch: 5.0 Gerrit-Owner: Bgerstle <bgers...@wikimedia.org> Gerrit-Reviewer: jenkins-bot <> _______________________________________________ MediaWiki-commits mailing list MediaWiki-commits@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/mediawiki-commits