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

Use scrolling UITextView for captions #88

Merged
merged 25 commits into from
Jan 8, 2016
Merged

Conversation

cdzombak
Copy link
Contributor

closes #45

Still todo

  • Fix shadow view position: layer's bounds after last -layoutSubviews call to the caption view have the wrong height. This applies to iOS 8 only.
  • Resolve Auto Layout warnings printed on presentation. This applies to iOS 9 only.
  • Consider pinning the text view to full width and using contentInset or textContainerInset instead of horizontal padding.

@cdzombak cdzombak self-assigned this Oct 30, 2015
@cdzombak
Copy link
Contributor Author

cdzombak commented Nov 2, 2015

See if we can allow panning the page VC left/right over a scrolling caption view.

This is going to require more though to complete in a clean way, keeping the rest of the library away from knowing about caption views (since they're replaceable).

@cdzombak cdzombak changed the title [WIP] Use scrolling UITextView for captions Use scrolling UITextView for captions Nov 2, 2015
@cdzombak cdzombak removed their assignment Nov 2, 2015
@@ -49,6 +50,10 @@ - (void)layoutSubviews {
}];

[super layoutSubviews];

if ([self.captionView respondsToSelector:@selector(setPreferredMaxLayoutWidth:)]) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be [self.captionView conformsToProtocol:@protocol(NYTPhotoCaptionViewLayoutWidthHinting)]?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure. Here are some arguments for and against changing to conformsToProtocol:

for

  • the current code carries implicit knowledge of what's in the NYTPhotoCaptionViewLayoutWidthHinting protocol, spilled across 2 lines (though a change to the protocol would still cause a compiler warning)
  • we should check exactly what we're casting to, before casting

against

  • what we really care about is whether we can set preferredMaxLayoutWidth, and with this approach certain classes (UILabel) get correct behavior for free. (the cast is practically a detail to satisfy the compiler, though a change to the protocol would still cause a compiler warning)
  • conformsToProtocol: requires a global runtime lock; respondsToSelector: does not
  • even if we check that it conforms to the protocol, the code still carries implicit knowledge about what methods are in the protocol because we still have to cast before sending the message

Copy link
Contributor

Choose a reason for hiding this comment

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

I asked because I assumed that was the point of adding the protocol in the first place. If you're just interested in the caption view responding to setPreferredMaxLayoutWidth:, what's the point of the protocol? You could just as easily cast to id and use the method, not the property, to silence any compiler warnings.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If you're just interested in the caption view responding to setPreferredMaxLayoutWidth:, what's the point of the protocol?

So that I have something to cast to on the next line: (id<NYTPhotoCaptionViewLayoutWidthHinting>)

Copy link
Contributor

Choose a reason for hiding this comment

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

I updated my comment with this line after you posted yours:

You could just as easily cast to id and use the method, not the property, to silence any compiler warnings.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's really the best answer for "why check respondsToSelector: instead of conformsToProtocol:?" Just that the former requires the runtime lock while doing 60fps UI work.

Copy link
Contributor

Choose a reason for hiding this comment

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

Whereas respondsToSelector: doesn't lock anything:

respondsToSelector: itself doesn't lock anything, but lookUpImpOrForward (which is called by lookUpImpOrNil) does get a runtime lock.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

respondsToSelector: itself doesn't lock anything, but lookUpImpOrForward (which is called by lookUpImpOrNil) does get a runtime lock.

well, I'll be! the comment on that function doesn't include a note about locking…I guess I should send a PR :)

Copy link
Contributor

Choose a reason for hiding this comment

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

That said, would something like that be considered an implementation detail if it's not mentioned in any documentation apart from the source code?

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 is all implementation detail! And I think I am sufficiently convinced not to micro-optimize this for now (though I suspect that this cache lookup will almost always succeed); see 162e0ba

@@ -11,6 +11,7 @@
#import "NYTExamplePhoto.h"

static const NSUInteger NYTViewControllerCustomEverythingPhotoIndex = 1;
static const NSUInteger NYTViewControllerLongCaptionPhotoIndex = 2;
static const NSUInteger NYTViewControllerDefaultLoadingSpinnerPhotoIndex = 3;
static const NSUInteger NYTViewControllerNoReferenceViewPhotoIndex = 4;
static const NSUInteger NYTViewControllerCustomMaxZoomScalePhotoIndex = 5;
Copy link
Contributor

Choose a reason for hiding this comment

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

just curious as to why not just make these static ints an enum.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

¯_(ツ)_/¯ they were static ints before, and slowly we've added a few more special cases.

@cdzombak
Copy link
Contributor Author

cdzombak commented Jan 4, 2016

I've resolved merge conflicts in 5d1a27d

@cdzombak
Copy link
Contributor Author

cdzombak commented Jan 7, 2016

I've resolved merge conflicts in 0cd6a1e

steve-matthews added a commit that referenced this pull request Jan 8, 2016
Use scrolling UITextView for captions
@steve-matthews steve-matthews merged commit 938968d into develop Jan 8, 2016
@cdzombak cdzombak deleted the cdz/scroll-captions branch January 8, 2016 03:48
@cdzombak
Copy link
Contributor Author

cdzombak commented Jan 8, 2016

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.

3 participants