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

Proposal: Interactive reload and loading indicator #160

Open
wants to merge 13 commits into
base: develop
Choose a base branch
from

Conversation

kimar
Copy link
Contributor

@kimar kimar commented Feb 7, 2016

Preamble

I've solved three problems we've been facing when using NYTPhotoViewer, initially I planned to file three PRs but as it turns out all of those problems are connected and it's more convenient to present them all together then separately.

Rationale

We're using NYTPhotoViewer to display photos, when a hi-res version of the photo to be shown is not available, we're presenting NYTPhotoViewer from a reference view, using a thumbnail, and asynchronically download the hi-res version.

This PR solves two problems for us:

  1. Right now it's not feasible to interact with photos while downloading a hi-res version AND replace the hi-res version after download while still maintaining zoom-scale and position so that the user interaction isn't interrupted when replacing the lo-res photo with the hi-res one.
  2. Display a progress to the user to inform him about when a file has been downloaded / will be replaced with a hi-res version.
  3. Enable configurable shadow on UINavigationBar, the same way the shadow (gradient) is composed on the lower tool bar. This will result in a unified user experience, also the user will now be able to see the buttons and loading indicator on the upper part of the screen, even if a bright photo is being displayed.

Sneak peek: The solution

out-1

Problem 1 solved: Interact with lo-res and replace with hi-res without distraction

We want to provide a first class experience when viewing photos, this also requires us to make it possible for users to interact with the lo-res photo (e.g. zooming and scrolling) and preserving the position as well as zoomscale but replacing the photo with a hi-res version when available.

This is only available when providing a custom zoom scale using:

- (CGFloat)photosViewController:(NYTPhotosViewController *)photosViewController
       maximumZoomScaleForPhoto:(id<NYTPhoto>)photo

Problem 2 solved: Visualizing download progress

When downloading the hi-res version of a photo, we want to provide a non-blocking, non-interruptive way to propagate this progress to the user. We've decided to go with a 2px indicator at the top of the view controller.

Problem 3 solved: UINavigationBar buttons and progress indicator not visible on bright photo

Because the UINavigationBar doesn't come with a shadow, it's white controls aren't visible on a bright background. By setting NYTPhotosOverlayView's navigationBarGradient property, a shadow can be added to solve this problem.

Marcus Kida added 12 commits January 5, 2016 14:43
# Please enter a commit message to explain why this merge is necessary,
# especially if it merges an updated upstream into a topic branch.
#
# Lines starting with '#' will be ignored, and an empty message aborts
# the commit.
…r-Shadow

Conflicts:
	Pod/Classes/ios/NYTPhotosOverlayView.m
…Shadow

Conflicts:
	Pod/Classes/ios/NYTPhotosOverlayView.h
	Pod/Classes/ios/NYTPhotosOverlayView.m
…ar-Shadow

# Please enter a commit message to explain why this merge is necessary,
# especially if it merges an updated upstream into a topic branch.
#
# Lines starting with '#' will be ignored, and an empty message aborts
# the commit.
@kimar
Copy link
Contributor Author

kimar commented Mar 1, 2016

We might want to also expose the progress/loader using a proper datasource #163

@baswenneker
Copy link

Hi @kimar, this is exactly what I needed! I checked out your code but I can't find how and where to trigger loading the high resolution image. Can you give an example of how you use it in your example? Thanks in advance!

@kimar
Copy link
Contributor Author

kimar commented Mar 7, 2016

@baswenneker NYTPhotoViewer isn't downloading the image, this is done in our framework which then leverages NYTPhotoViewer again. What we're doing is basically an NSURLSession request for the image, in case it's not cached and will be downloaded we're propagating the progress to the the indicator in NYTPhotoViewer. We're using a custom subclass of NYTPhotoViewer and to reset the individual progress for every single using NYTPhotoViewerDelegate e.g.

- (void)photosViewController:(NYTPhotosViewController *)photosViewController
          didNavigateToPhoto:(FVImage <NYTPhoto> *)photo
                     atIndex:(NSUInteger)photoIndex {
    self.overlayView.loadingIndicatorView.progress = 0.0f;
}

The progress property is then update in our download operation.

Also bear in mind that replacing the image after downloading the hi-res version currently requires the final image to have the same aspect ratio, but for our use case this hasn't been an issue so far.

@kimar kimar force-pushed the feature/UINavigationBar-Shadow branch from 167eabd to b64916f Compare April 6, 2016 01:18
@egarlock
Copy link

egarlock commented Aug 17, 2016

I found an issue with this fix @kimar.

I am using lazy loading and calling updateImageForPhoto: to show the initial image if it's not already loaded.

When I do this it does not set the initial imageView.size here:

- (void)updateImage:(UIImage *)image imageData:(NSData *)imageData {
...
#ifdef INTERACTIVE_RELOAD
    self.imageView.frame = CGRectMake(0, 0, imageTouse.size.width, imageTouse.size.height);
#endif
...
}

I updated your code to handle this case as below:

- (void)updateImage:(UIImage *)image imageData:(NSData *)imageData {
...
#ifdef INTERACTIVE_RELOAD 
    self.imageView.frame = CGRectMake(0, 0, imageToUse.size.width, imageToUse.size.height);
#else
    if (CGSizeEqualToSize(self.imageView.frame, CGSizeZero)) {
        self.imageView.frame = CGRectMake(0, 0, imageToUse.size.width, imageToUse.size.height);
    }
#endif
...
}

Also this may further be an issue as described in #206.

@egarlock
Copy link

Hey Marcus (@kimar),

Are you using a constant for your - (CGFloat)photosViewController:(NYTPhotosViewController *)photosViewController maximumZoomScaleForPhoto:(id<NYTPhoto>)photo ?

I was experimenting with using a dynamic max scale based on size of image but just realized you may be using a constant here.

UIImageView *imageView = scalingImageView.imageView;

CGFloat imageHeight = CGRectGetWidth(imageView.frame) * CGRectGetHeight(imageView.frame) / CGRectGetWidth(imageView.frame);
CGFloat imageWidth = CGRectGetHeight(imageView.frame) * CGRectGetWidth(imageView.frame) / CGRectGetHeight(imageView.frame);
Copy link
Contributor

@cdzombak cdzombak Feb 28, 2017

Choose a reason for hiding this comment

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

What's the goal, in this calculation, of performing GetHeight * GetWidth / GetHeight? Isn't that equivalent to just GetWidth?

@awartani
Copy link

awartani commented Aug 7, 2019

Great work @kimar, any reason why this kept open until now? looks like a great addition.

@kimar
Copy link
Contributor Author

kimar commented Aug 9, 2019

@awartani it's been years since I've been in touch with NYTPhotoViewer and this PR in particular.
I'm not sure about the state and if it even aligns with the roadmap of NYTPhotoViewer, also IMO it still has quite an RFC character and probably requires significant work until it's mergeable.

Unfortunately I can't provide anymore help on this currently.

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.

None yet

5 participants