Add ability to remove an item from image viewer #101

humblehacker · 2017-01-07T22:59:29Z

This solution works well in my testing, but there may be some aspect I've missed. I'm open to suggestions for improvement/alternate approaches.

Krisiacik · 2017-01-10T14:20:31Z

@humblehacker many thanks for the effort!. We need take a look at this..
@zfoltin ...can you take a look pls? I have some crunch time going on these days :/

zfoltin

Cheers for the PR, nice work! 🚀
A few comments here and there, please address these before we can merge it 🏎

zfoltin · 2017-01-10T18:09:21Z

Example/ViewController.swift

+    func provideGalleryItem(_ index: Int) -> GalleryItem {
+
+        let imageView = imageViews[index]
+        return items[imageView]!


Can we use guard or if let instead of !?

zfoltin · 2017-01-10T18:10:28Z

Example/ViewController.swift

@@ -20,12 +20,41 @@ class ViewController: UIViewController, GalleryItemsDataSource, GalleryDisplaced
    @IBOutlet weak var image6: UIImageView!
    @IBOutlet weak var image7: UIImageView!

-    var imageViews: [UIImageView] = []
+    var imageViews: [UIImageView]              = []


personally not a big fan of this type of indentation... but whatever :)

No problem. I've got AppCode set up to do this for me, but I can turn it off for this.

zfoltin · 2017-01-10T18:12:12Z

Example/ViewController.swift

+
+        let imageView = imageViews[index]
+        imageView.removeFromSuperview()
+        imageViews.remove(at: index)


don't we need to remove the item from items too?

Since this is just test code, and not really how one would implement a view like this, I didn't see the need to keep the two structures in sync - the dictionary will always contain a superset of the data in the array, with no real downside. If you prefer, I can come up with a cleaner and more representative example at the expense of additional code.

Correct, we want to keep the example as simple as possible. In this case, for the sake of "completeness", I'd say let's just remove the item from items too. Just to show that the consumer is responsible for managing both of these fields... maybe expanding a bit on the idea with a comment?

zfoltin · 2017-01-10T18:12:34Z

ImageViewer.xcodeproj/project.pbxproj

@@ -440,7 +444,9 @@
 				TargetAttributes = {
 					C7897CE11C1350B7006447FB = {
 						CreatedOnToolsVersion = 7.1.1;
+						DevelopmentTeam = LVJ3X3W924;


zfoltin · 2017-01-10T18:12:52Z

ImageViewer.xcodeproj/project.pbxproj

@@ -619,6 +627,8 @@
 			isa = XCBuildConfiguration;
 			buildSettings = {
 				ASSETCATALOG_COMPILER_APPICON_NAME = AppIcon;
+				"CODE_SIGN_IDENTITY[sdk=iphoneos*]" = "iPhone Developer";
+				DEVELOPMENT_TEAM = LVJ3X3W924;


zfoltin · 2017-01-10T18:12:59Z

ImageViewer.xcodeproj/project.pbxproj

@@ -632,6 +642,8 @@
 			isa = XCBuildConfiguration;
 			buildSettings = {
 				ASSETCATALOG_COMPILER_APPICON_NAME = AppIcon;
+				"CODE_SIGN_IDENTITY[sdk=iphoneos*]" = "iPhone Developer";
+				DEVELOPMENT_TEAM = LVJ3X3W924;


zfoltin · 2017-01-10T18:14:07Z

ImageViewer/Source/Extensions/UIButton.swift

+    static func deleteButton() -> UIButton {
+
+        let button = UIButton(frame: CGRect(origin: CGPoint.zero, size: CGSize(width: 80, height: 50)))
+        button.setTitle("Delete", for: UIControlState())


for: .normal no?

Yep. This was a copy/paste from one of the other existing button methods, and I hadn't noticed the UIControlState() argument.

zfoltin · 2017-01-10T18:15:01Z

ImageViewer/Source/GalleryPagingDataSource.swift

@@ -16,7 +16,7 @@ final class GalleryPagingDataSource: NSObject, UIPageViewControllerDataSource {

    fileprivate let configuration: GalleryConfiguration
    fileprivate var pagingMode = GalleryPagingMode.standard
-    fileprivate let itemCount: Int
+    fileprivate var itemCount: Int { return itemsDataSource!.itemCount() }


try to avoid ! if at all possible

When I started with Swift, I took a very strict approach with optionals, essentially never force unwrapping. I've since relaxed that view. In cases where optionals are guaranteed to be non-nil or would be a programmer error if they were, force unwrapping and the resultant crash are the way to go, as it allows finding the problem early. I found guard let with fatalError() to be unnecessary extra code.

In this case, attempting to use this class without an itemsDataSource defined would be a programmer error.

Another point is that propagating optionals can result in unnecessary complexity as the optional must be dealt with at multiple levels, so I avoid returning optionals unless it really makes sense for the interface.

That said, if you guys prefer to keep force unwrapping out, I'm fine with following your convention.

Absolutely agree with you, I try to follow a very similar approach when it comes to !
In this specific case I feel like it'd be reasonable to return 0 for itemCount in case the dataSource isn't set. How would return itemsDataSource?.itemCount() ?? 0 make you feel? :)

zfoltin · 2017-01-10T18:17:08Z

ImageViewer/Source/GalleryViewController.swift

        if let closeButton = closeButton {
+            closeButton.addTarget(self, action: #selector(GalleryViewController.closeInteractively), for: .touchUpInside)


humblehacker · 2017-01-10T19:26:39Z

Thanks so much for taking the time to review my contribution! For the sake of review, do you guys prefer additional commits with the review changes, or amending of the original commit?

zfoltin · 2017-01-10T19:28:31Z

Thanks for contributing @humblehacker!
Additional commit(s) with the changes would be preferable :) Cheers 👍

humblehacker · 2017-01-10T20:36:07Z

Pushed too soon. Please stand by ;)

humblehacker · 2017-01-10T21:15:02Z

@zfoltin Ok, second try. I think this implementation of ViewController is much simpler.

zfoltin · 2017-01-11T09:00:28Z

Cheers for the work @humblehacker great stuff! 👍
This closes #97

Add ability to remove an item from image viewer

1a41985

zfoltin requested changes Jan 10, 2017

View reviewed changes

Code review changes

3350cd0

humblehacker force-pushed the remove-item branch from e0ff242 to 3350cd0 Compare January 10, 2017 21:11

zfoltin mentioned this pull request Jan 11, 2017

Remove image from gallery #97

Closed

zfoltin approved these changes Jan 11, 2017

View reviewed changes

zfoltin merged commit d69fd2c into Krisiacik:master Jan 11, 2017

humblehacker deleted the remove-item branch January 11, 2017 20:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add ability to remove an item from image viewer #101

Add ability to remove an item from image viewer #101

humblehacker commented Jan 7, 2017 •

edited

Loading

Krisiacik commented Jan 10, 2017

zfoltin left a comment

zfoltin Jan 10, 2017

zfoltin Jan 10, 2017

humblehacker Jan 10, 2017

zfoltin Jan 10, 2017

humblehacker Jan 10, 2017

zfoltin Jan 10, 2017

zfoltin Jan 10, 2017

humblehacker Jan 10, 2017

zfoltin Jan 10, 2017

zfoltin Jan 10, 2017

zfoltin Jan 10, 2017

humblehacker Jan 10, 2017

zfoltin Jan 10, 2017

humblehacker Jan 10, 2017 •

edited

Loading

zfoltin Jan 10, 2017

zfoltin Jan 10, 2017

humblehacker commented Jan 10, 2017

zfoltin commented Jan 10, 2017

humblehacker commented Jan 10, 2017

humblehacker commented Jan 10, 2017

zfoltin commented Jan 11, 2017

		if let closeButton = closeButton {
		closeButton.addTarget(self, action: #selector(GalleryViewController.closeInteractively), for: .touchUpInside)

Add ability to remove an item from image viewer #101

Add ability to remove an item from image viewer #101

Conversation

humblehacker commented Jan 7, 2017 • edited Loading

Krisiacik commented Jan 10, 2017

zfoltin left a 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

humblehacker Jan 10, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

humblehacker commented Jan 10, 2017

zfoltin commented Jan 10, 2017

humblehacker commented Jan 10, 2017

humblehacker commented Jan 10, 2017

zfoltin commented Jan 11, 2017

humblehacker commented Jan 7, 2017 •

edited

Loading

humblehacker Jan 10, 2017 •

edited

Loading