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.

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

refresh caption and overlay info whenever the current photo updates #214

Closed
wants to merge 2 commits into from
Closed

refresh caption and overlay info whenever the current photo updates #214

wants to merge 2 commits into from

Conversation

zadr
Copy link

@zadr zadr commented Sep 13, 2016

this allows for building more interactive captions by updating the view model and then having photosViewController(_:captionViewFor:) called again.

@cdzombak
Copy link
Contributor

cdzombak commented Dec 9, 2016

Thanks for the PR, @zadr! I have two questions/issues with this PR as it stands:

  • How is this refresh triggered? As far as I can tell it happens when -updateImageForPhoto: is called, which isn't a particularly clear API (the docs for that method say, "Update the image displayed for the given photo object").

  • When displaying multiple photos, this will cause the overlay view to flicker or reset for all photos, not just the one that changed. See for example this gif from the Example app, where the scrolled caption view is reset when a different photo is updated:

reset-all-overlays

I am currently trying to make the dataflow for this library clearer (by, among other things, exposing the data source; see #163) which I hope will provide groundwork to add flexibility like this.

@zadr
Copy link
Author

zadr commented Dec 24, 2016

@cdzombak Hm. As far as your first point goes, I think it's relatively clear to say that if you call updateImageForPhoto:, it updates everything as required for the view to show the latest information.

If I'm understanding you correctly, the problem you're worried about boils down to updateImageForPhoto: being a blanket refreshing of all data (something changed, a la reloadData), rather than being able to diff changes between immutable photo models (this one thing in the backing data changed, a la reloadItemsAtIndexPaths:)?

This kinda thinking about view models really lends itself better to Swift and immutable structs than it does to Objective-C and mutable objects.

Anyway, moving to a data source-y basis does make it easy to ask for individual components on a per-photo basis, rather than having it all lumped together and having to diff a model object (or not being able to diff at all if the same underlying object is mutated), and that definitely seems like a decent direction to move in to me.

As far as the second point goes, oops. thats a bug. Will update the PR; thanks for catching it!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants