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
