Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Remove experimental features (exp_skip_a11y_wait && exp_new_default_cell_layout_mode) #1383

Merged
merged 6 commits into from
Mar 11, 2019
Merged

Remove experimental features (exp_skip_a11y_wait && exp_new_default_cell_layout_mode) #1383

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
2813f6c
Remove experimental features
ernestmama Mar 7, 2019
42ef59f
prevent blocking main thread
ernestmama Mar 7, 2019
d4afe22
remove small content all together as none is the default
ernestmama Mar 7, 2019
504e7e4
Merge branch 'master' into removeExperimentalFeatures
ernestmama Mar 9, 2019
df53269
Merge branch 'master' into removeExperimentalFeatures
garrettmoon Mar 11, 2019
0e84fc0
Update ASExperimentalFeatures.h
garrettmoon Mar 11, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 0 additions & 2 deletions Schemas/configuration.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,8 +23,6 @@
"exp_skip_clear_data",
"exp_did_enter_preload_skip_asm_layout",
"exp_disable_a11y_cache",
"exp_skip_a11y_wait",
"exp_new_default_cell_layout_mode",
"exp_dispatch_apply",
"exp_image_downloader_priority",
"exp_text_drawing"
Expand Down
36 changes: 13 additions & 23 deletions Source/ASCollectionView.mm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -318,12 +318,6 @@ - (instancetype)_initWithFrame:(CGRect)frame collectionViewLayout:(UICollectionV

[self _configureCollectionViewLayout:layout];

if (ASActivateExperimentalFeature(ASExperimentalNewDefaultCellLayoutMode)) {
_cellLayoutMode = ASCellLayoutModeSyncForSmallContent;
} else {
_cellLayoutMode = ASCellLayoutModeNone;
}

return self;
}

Expand Down Expand Up @@ -1912,20 +1906,18 @@ - (BOOL)dataController:(ASDataController *)dataController shouldSynchronouslyPro
if (ASCellLayoutModeIncludes(ASCellLayoutModeAlwaysAsync)) {
return NO;
}
if (ASCellLayoutModeIncludes(ASCellLayoutModeSyncForSmallContent)) {
// Reload data is expensive, don't block main while doing so.
if (changeSet.includesReloadData) {
Copy link
Contributor

@maicki maicki Mar 7, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any reason we removed this if clause?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This code was not there when the new layout changes was introduced. This is added when we created the experiment afterward. I checked with @nguyenhuy that we did not experiment without this block of code. I will put it back.

return NO;
}
// If we have very few ASCellNodes (besides UIKit passthrough ones), match UIKit by blocking.
if (changeSet.countForAsyncLayout < 2) {
return YES;
}
CGSize contentSize = self.contentSize;
CGSize boundsSize = self.bounds.size;
if (contentSize.height <= boundsSize.height && contentSize.width <= boundsSize.width) {
return YES;
}
// Reload data is expensive, don't block main while doing so.
if (changeSet.includesReloadData) {
return NO;
}
// If we have very few ASCellNodes (besides UIKit passthrough ones), match UIKit by blocking.
if (changeSet.countForAsyncLayout < 2) {
return YES;
}
CGSize contentSize = self.contentSize;
CGSize boundsSize = self.bounds.size;
if (contentSize.height <= boundsSize.height && contentSize.width <= boundsSize.width) {
return YES;
}
return NO; // ASCellLayoutModeNone
}
Expand Down Expand Up @@ -2520,9 +2512,7 @@ - (void)setPrefetchingEnabled:(BOOL)prefetchingEnabled

- (NSArray *)accessibilityElements
{
if (!ASActivateExperimentalFeature(ASExperimentalSkipAccessibilityWait)) {
[self waitUntilAllUpdatesAreCommitted];
}
[self waitUntilAllUpdatesAreCommitted];
return [super accessibilityElements];
}

Expand Down
6 changes: 0 additions & 6 deletions Source/ASCollectionViewProtocols.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,12 +46,6 @@ typedef NS_OPTIONS(NSUInteger, ASCellLayoutMode) {
* cell height animations are desired.
*/
ASCellLayoutModeAlwaysBatchUpdateSectionReload = 1 << 9, // Default OFF

/**
* If ASCellLayoutModeSyncForSmallContent is enabled it will cause ASDataController to wait on the
* background queue if the amount of new content is small.
*/
ASCellLayoutModeSyncForSmallContent = 1 << 10,
};

NS_ASSUME_NONNULL_BEGIN
Expand Down
10 changes: 4 additions & 6 deletions Source/ASExperimentalFeatures.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,12 +27,10 @@ typedef NS_OPTIONS(NSUInteger, ASExperimentalFeatures) {
ASExperimentalFramesetterCache = 1 << 6, // exp_framesetter_cache
ASExperimentalSkipClearData = 1 << 7, // exp_skip_clear_data
ASExperimentalDidEnterPreloadSkipASMLayout = 1 << 8, // exp_did_enter_preload_skip_asm_layout
ASExperimentalDisableAccessibilityCache = 1 << 9, // exp_disable_a11y_cache
ASExperimentalSkipAccessibilityWait = 1 << 10, // exp_skip_a11y_wait
ASExperimentalNewDefaultCellLayoutMode = 1 << 11, // exp_new_default_cell_layout_mode
ASExperimentalDispatchApply = 1 << 12, // exp_dispatch_apply
ASExperimentalImageDownloaderPriority = 1 << 13, // exp_image_downloader_priority
ASExperimentalTextDrawing = 1 << 14, // exp_text_drawing
ASExperimentalDisableAccessibilityCache = 1 << 9, // exp_disable_a11y_cache
ASExperimentalDispatchApply = 1 << 10, // exp_dispatch_apply
ASExperimentalImageDownloaderPriority = 1 << 11, // exp_image_downloader_priority
ASExperimentalTextDrawing = 1 << 12, // exp_text_drawing
ASExperimentalFeatureAll = 0xFFFFFFFF
};

Expand Down
2 changes: 0 additions & 2 deletions Source/ASExperimentalFeatures.mm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,8 +22,6 @@
@"exp_skip_clear_data",
@"exp_did_enter_preload_skip_asm_layout",
@"exp_disable_a11y_cache",
@"exp_skip_a11y_wait",
@"exp_new_default_cell_layout_mode",
@"exp_dispatch_apply",
@"exp_image_downloader_priority",
@"exp_text_drawing"]));
Expand Down
30 changes: 13 additions & 17 deletions Source/ASTableView.mm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1703,20 +1703,18 @@ - (BOOL)dataControllerShouldSerializeNodeCreation:(ASDataController *)dataContro

- (BOOL)dataController:(ASDataController *)dataController shouldSynchronouslyProcessChangeSet:(_ASHierarchyChangeSet *)changeSet
{
if (ASActivateExperimentalFeature(ASExperimentalNewDefaultCellLayoutMode)) {
// Reload data is expensive, don't block main while doing so.
if (changeSet.includesReloadData) {
Copy link
Contributor

@maicki maicki Mar 7, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any reason we removed this if clause?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This code was not there when the new layout changes was introduced. This is added when we created the experiment afterward. I checked with @nguyenhuy that we did not experiment without this block of code. I will put it back.

return NO;
}
// For more details on this method, see the comment in the ASCollectionView implementation.
if (changeSet.countForAsyncLayout < 2) {
return YES;
}
CGSize contentSize = self.contentSize;
CGSize boundsSize = self.bounds.size;
if (contentSize.height <= boundsSize.height && contentSize.width <= boundsSize.width) {
return YES;
}
// Reload data is expensive, don't block main while doing so.
if (changeSet.includesReloadData) {
return NO;
}
// For more details on this method, see the comment in the ASCollectionView implementation.
if (changeSet.countForAsyncLayout < 2) {
return YES;
}
CGSize contentSize = self.contentSize;
CGSize boundsSize = self.bounds.size;
if (contentSize.height <= boundsSize.height && contentSize.width <= boundsSize.width) {
return YES;
}
return NO;
}
Expand Down Expand Up @@ -2036,9 +2034,7 @@ - (void)didMoveToSuperview

- (NSArray *)accessibilityElements
{
if (!ASActivateExperimentalFeature(ASExperimentalSkipAccessibilityWait)) {
[self waitUntilAllUpdatesAreCommitted];
}
[self waitUntilAllUpdatesAreCommitted];
return [super accessibilityElements];
}

Expand Down
6 changes: 0 additions & 6 deletions Tests/ASCollectionViewTests.mm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1050,12 +1050,6 @@ - (void)testInitialRangeBounds
shouldWaitUntilAllUpdatesAreProcessed:YES];
}

- (void)testInitialRangeBoundsCellLayoutModeSyncForSmallContent
{
[self testInitialRangeBoundsWithCellLayoutMode:ASCellLayoutModeSyncForSmallContent
shouldWaitUntilAllUpdatesAreProcessed:YES]; // Need to wait because the first initial data load is always async
}

- (void)testInitialRangeBoundsCellLayoutModeAlwaysAsync
{
[self testInitialRangeBoundsWithCellLayoutMode:ASCellLayoutModeAlwaysAsync
Expand Down
4 changes: 0 additions & 4 deletions Tests/ASConfigurationTests.mm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,8 +28,6 @@
ASExperimentalSkipClearData,
ASExperimentalDidEnterPreloadSkipASMLayout,
ASExperimentalDisableAccessibilityCache,
ASExperimentalSkipAccessibilityWait,
ASExperimentalNewDefaultCellLayoutMode,
ASExperimentalDispatchApply,
ASExperimentalImageDownloaderPriority,
ASExperimentalTextDrawing
Expand All @@ -55,8 +53,6 @@ + (NSArray *)names {
@"exp_skip_clear_data",
@"exp_did_enter_preload_skip_asm_layout",
@"exp_disable_a11y_cache",
@"exp_skip_a11y_wait",
@"exp_new_default_cell_layout_mode",
@"exp_dispatch_apply",
@"exp_image_downloader_priority",
@"exp_text_drawing"
Expand Down