Expose a data-source-oriented API for PhotosViewController #226

cdzombak · 2017-01-27T19:47:40Z

The current NYTPhotoViewer API accepts a collection of photos and creates an internal, mutable data source. Clients may call one method to update a photo currently in the data source. This comes with some problems and complexity, most notably: It's not possible to add or remove photos, and adding imperative add/remove/reorder photo API would further increase complexity of PhotosViewController, which probably shouldn't be responsible for managing a collection of data. (see #139 #140 #141).

Future improvements could include separating data source requests for caption and photo data, should we decide that would be helpful.

This should also make #162 easier (or maybe it's now possible?).

I've also fixed the problematic behavior where delegate methods were never called for the first photo.

closes #163
closes #214

This makes using the photo viewer with a single photo _almost_ as simple as it was before.

This eliminates the bug where some delegate methods weren’t called for the first photo

craighowarth · 2017-01-30T16:15:32Z

NYTPhotoViewer/NYTPhotoViewerArrayDataSource.m

@@ -1,20 +1,14 @@
 //
-//  NYTPhotosDataSource.m
+//  NYTPhotoViewerArrayDataSource.m
 //  NYTPhotoViewer
 //
 //  Created by Brian Capps on 2/11/15.
 //


Should there be a copyright statement here. Not sure what the protocol is for open source.

The license (which was reviewed by the legal team a while back, I think) includes a copyright so I'll add one here.

Added in db4ccb5

craighowarth · 2017-01-30T16:15:50Z

NYTPhotoViewer/NYTPhotoViewerArrayDataSource.h

+//  NYTPhotoViewer
+//
+//  Created by Brian Capps on 2/11/15.
+//


Added in db4ccb5

craighowarth · 2017-01-31T14:04:05Z

Example/NYTViewController.m


    [self presentViewController:photosViewController animated:YES completion:nil];

-    [self updateImagesOnPhotosViewController:photosViewController afterDelayWithPhotos:self.photos];
+    [self updateImagesOnPhotosViewController:photosViewController afterDelayWithDataSource:self.dataSource];
+//    [self switchDataSourceOnPhotosViewController:photosViewController afterDelayWithDataSource:self.dataSource];


Perhaps provide a little explanation here. Is your intention that only one of these two lines be uncommented or should we simply uncomment line 41 to see the data source switch after a delay. It works both ways, but it would be nice to know your intention. Or maybe use an if statement to make it more clear:

BOOL switchDataSource = false [self updateImagesOnPhotosViewController:photosViewController afterDelayWithDataSource:self.dataSource]; if (switchDataSource) { [self switchDataSourceOnPhotosViewController:photosViewController afterDelayWithDataSource:self.dataSource]; }

I'd like to demonstrate both the possibility of updating a single photo, and the possibility of completely changing out the data source.

The complete-data-source-change is surprising, though, if you're not expecting it. I wanted that to be optional.

I like the BOOL suggestion and will implement it shortly.

craighowarth · 2017-01-31T14:13:46Z

Example/NYTViewController.m

            if (!photo.image && !photo.imageData) {
                photo.image = [UIImage imageNamed:@"NYTimesBuilding"];
-                [photosViewController updateImageForPhoto:photo];
+                photo.attributedCaptionSummary = [[NSAttributedString alloc] initWithString:@"Photo which previously had a loading spinner (reopen the photo viewer and scroll to here to see it)" attributes:@{NSForegroundColorAttributeName: [UIColor whiteColor], NSFontAttributeName: [UIFont preferredFontForTextStyle:UIFontTextStyleBody]}];


Perhaps change the caption to... "Photo which previously had a loading spinner (to see the spinner, reopen the photo viewer and scroll to this photo)"

craighowarth · 2017-01-31T14:17:39Z

Example/NYTViewController.m

@@ -140,24 +107,24 @@ - (UIView *)photosViewController:(NYTPhotosViewController *)photosViewController
 }

 - (CGFloat)photosViewController:(NYTPhotosViewController *)photosViewController maximumZoomScaleForPhoto:(id <NYTPhoto>)photo {
-    if ([photo isEqual:self.photos[NYTViewControllerPhotoIndexCustomMaxZoomScale]]) {
+    if ([photo isEqual:self.dataSource.photos[NYTViewControllerPhotoIndexCustomMaxZoomScale]]) {
        return 10.0f;


Perhaps change the custom scale to 0.5f because 10.0f is hard to tell that it is different when you zoom because the original image is so large to begin with. Using 0.5f it is clear that the scaling is different.

craighowarth · 2017-01-31T14:26:52Z

NYTPhotoViewer/NYTPhotosViewController.h

 */
- (instancetype)initWithPhotos:(NSArray <id <NYTPhoto>> * _Nullable)photos initialPhoto:(id <NYTPhoto> _Nullable)initialPhoto delegate:(nullable id <NYTPhotosViewControllerDelegate>)delegate NS_DESIGNATED_INITIALIZER;
+- (instancetype)initWithDataSource:(id <NYTPhotoViewerDataSource>)dataSource initialPhoto:(id <NYTPhoto> _Nullable)initialPhoto delegate:(nullable id <NYTPhotosViewControllerDelegate>)delegate NS_DESIGNATED_INITIALIZER;


Aren't _Nullable and nullable equivalent? Is there a reason not to be consistent? I guess this was already here but it might be nice to make the usage consistent and settle on one style.

Ah, yes, they are. Good catch. I think the initial code was written before nullable was added, and only _Nullable was available. Then I just blindly copied it here when adding this declaration.

craighowarth · 2017-01-31T14:33:42Z

The swift example does not compile because of "Use Legacy Swift Language Version (SWIFT_VERSION) is required to be configured correctly for targets which use Swift." Not sure if this is covered by another issue to bring it update to date.

craighowarth · 2017-01-31T14:34:45Z

@cdzombak Looks good! Just a few fairly minor comments. Let me know when you've addressed these (or not) and I'll give it a final review.

cdzombak · 2017-01-31T21:42:12Z

The swift example does not compile because of "Use Legacy Swift Language Version (SWIFT_VERSION) is required to be configured correctly for targets which use Swift." Not sure if this is covered by another issue to bring it update to date.

#223 would update the project to Swift 3 (though after that, the Swift project will need to be updated with the API changes in this PR).

cdzombak · 2017-01-31T21:44:31Z

Let me know when you've addressed these (or not) and I'll give it a final review.

@craighowarth I believe I've addressed each of your comments.

craighowarth · 2017-01-31T22:09:18Z

👍

Chris Dzombak added 30 commits December 9, 2016 14:53

clarify when NYTPhotoCaptionView is used

1acd496

rename concrete data source to NYTPhotoViewerArrayDataSource

66784db

[minor] add clarifying comment to umbrella header

f5e3dc8

Rename NYTPhotoViewerDataSource and clarify it vs. delegate

4c180d9

[minor] Improve comments in NYTPhoto.h

d1394c6

Update library code to work with new NYTPhotoViewerDataSource

e615a09

Update Example app to work with new NYTPhotoViewerDataSource

2652474

Clean up ArrayDataSource interface

577aa89

Add factory method for ArrayDataSource

ae015e2

Handle null input properly in ArrayDataSource

620540d

NYTPhotosViewController takes a data source, not an array

207a448

Allow informing PhotosVC that an individual photo has changed

3e0dba9

Better document updatePhotoAtIndex: behavior

de002c1

refactor: use a property that’s already been implemented

ed8f246

Add another example photo

b0a9525

Better document updatePhoto: methods

762483b

Allow setting NYTPhotosVC data source

587c82b

Add reload-all-photos method to NYTPhotosVC

4750578

Add an Example codepath which demos replacing the data source

d3d2323

Improve NYTPhotosVC animation logic

290ba8c

Add a single-photo data source class

8f6011a

This makes using the photo viewer with a single photo _almost_ as simple as it was before.

docs: tweak ArrayDataSource initializer documentation

3dcdbf0

Modernize PhotosVC code style

f8ccb16

Make Xcode (8.2) recommended updates to NYTPhotoViewer’s Xcode project

62d54be

Avoid using NYTPhotosVC’s delegate until after -init… returns

21c3852

This eliminates the bug where some delegate methods weren’t called for the first photo

Add convenience NYTPhotosVC initializer accepting solely a data source

bb38825

Fix incorrect NYTPhotosVC convenience initializer

2f45ab9

style: correct spacing in Example ViewController

b5e3c07

fix: use proper equality checks

f71d8f4

docs: clarify how NYTPhoto equality is determined

4f81acb

docs: add this feature to CHANGELOG

cf9cfbc

cdzombak added this to the 2.0.0 milestone Jan 27, 2017

craighowarth reviewed Jan 30, 2017

View reviewed changes

Chris Dzombak added 2 commits January 30, 2017 11:31

Update LICENSE to specify (c) through 2017

a57f6d2

docs: add missing copyright in headers

db4ccb5

craighowarth reviewed Jan 31, 2017

View reviewed changes

Chris Dzombak added 4 commits January 31, 2017 16:43

example: Use a more obvious demo for custom maximum zoom scale

bbacf0c

example: Improve data-source-swap demo

7dc1e0d

example: Improve caption for photo with a loading spinner

d7aa268

Use more modern nullable instead of _Nullable

c67c605

cdzombak merged commit f399642 into develop Jan 31, 2017

cdzombak deleted the feature/datasource branch January 31, 2017 22:18

cdzombak mentioned this pull request Feb 14, 2017

updatePhotoAtIndex should work based on index, not identity #230

Closed

cdzombak mentioned this pull request Mar 8, 2017

Setting Data Source #233

Closed

cdzombak mentioned this pull request Apr 13, 2017

Update README to reflect new data source API #238

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Expose a data-source-oriented API for PhotosViewController #226

Expose a data-source-oriented API for PhotosViewController #226

cdzombak commented Jan 27, 2017

craighowarth Jan 30, 2017

cdzombak Jan 30, 2017

cdzombak Jan 30, 2017

craighowarth Jan 30, 2017

cdzombak Jan 30, 2017

craighowarth Jan 31, 2017 •

edited

Loading

cdzombak Jan 31, 2017

craighowarth Jan 31, 2017

craighowarth Jan 31, 2017

craighowarth Jan 31, 2017

cdzombak Jan 31, 2017

craighowarth commented Jan 31, 2017

craighowarth commented Jan 31, 2017

cdzombak commented Jan 31, 2017

cdzombak commented Jan 31, 2017

craighowarth commented Jan 31, 2017

Expose a data-source-oriented API for PhotosViewController #226

Expose a data-source-oriented API for PhotosViewController #226

Conversation

cdzombak commented Jan 27, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

craighowarth Jan 31, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

craighowarth commented Jan 31, 2017

craighowarth commented Jan 31, 2017

cdzombak commented Jan 31, 2017

cdzombak commented Jan 31, 2017

craighowarth commented Jan 31, 2017

craighowarth Jan 31, 2017 •

edited

Loading