Add ability to receive UITableViewDelegate & UIScrollViewDelegate messages from UITableView #119

jaredegan · 2017-11-21T19:29:25Z

Using Static for your table view solutions previously caused some limitations for certain use cases. Since the DataSource object is both the dataSource & delegate of the UITableView instance, users of the Static library were unable to receive the UITableViewDelegate methods they might need to do things like reacting to scrolling, header displays or any further actions.

This change introduces a new weak property on DataSource: tableViewDelegate. This needs to be set during init, due to the execution timing of forwardingTarget & respondsToSelector. Using those two methods, the unimplemented UITableViewDelegate methods can get forwarded to the delegate that the user specifies, unlocking this missing functionality, mentioned in #97, #78, #72.

Two already implemented UITableViewDelegate functions will also forward the message to the tableViewDelegate property (if it implements those methods). The other remaining function's behavior is already fully exposed by Static, so it didn't need to be changed.

Other notes about this change/PR:

This is backwards compatible. Updated version based on semantic versioning.
Unit test added, fully covers new code.
Example ViewController class modified to include example of this functionality.

…ve UITableViewDelegate & UIScrollViewDelegate messages from the UITableView. Forward didSelectRow and accessoryButtonTappedForRowWith messages to tableViewDelegate property. Bump version from 2.2.0 to 2.3.0

jhaddadnia · 2017-11-21T19:52:10Z

Static.podspec

@@ -1,6 +1,6 @@
 Pod::Spec.new do |spec|
  spec.name = 'Static'
-  spec.version = '2.2.0'
+  spec.version = '2.3.0'


is this a breaking change? 🤔

nah, bro. I don't know what Venmo uses for CocoaPods, but I assumed Semantic Versioning: https://semver.org/

Given a version number MAJOR.MINOR.PATCH, increment the:

MAJOR version when you make incompatible API changes,
MINOR version when you add functionality in a backwards-compatible manner, and
PATCH version when you make backwards-compatible bug fixes.

ahhh, you right you right.

jhaddadnia · 2017-11-21T19:57:42Z

Static/DataSource.swift

@@ -275,12 +293,16 @@ extension DataSource: UITableViewDelegate {
        if let row = row(at: indexPath) {
            row.selection?()
        }
+
+        tableViewDelegate?.tableView?(tableView, didSelectRowAt: indexPath)


why is this needed if we have the forwarding target?.. wouldn't it send this there anyway?.. I may be misunderstanding forwardingTarget...

Since DataSource implements this method, it gets the message sent to it, and thus it doesn't go to the forwardingTarget.

Messages only get forwarded to the forwardingTarget if this object doesn't implement.

txaiwieser · 2017-11-22T18:19:47Z

This is great! If merged I would be able to delete a lot of hacks and even my custom Static branch

eliperkins · 2017-11-22T22:00:47Z

Oh heck yeah! This looks great, awesome usage of selector forwarding. It might be a bit of overhead to ensure that each method we implement needs to call down to tableViewDelegate as well, so we’ll have to weigh that trade off, but it doesn’t seem so bad.
I’ll have a more thorough look through this next week.

_{Sent with GitHawk}

txaiwieser · 2017-11-22T22:09:35Z

I’m using to forward scrollViewDidScroll and it seems fine. But I’m a little worried too. Any thoughts on how to measure the impact?

_{Sent with GitHawk}

jaredegan · 2017-11-28T20:12:32Z

@txaiwieser He was referring to the cognitive impact of remembering / deciding whether or not to forward the UITableViewDelegate they implement in the future. Not for performance implications.

I wouldn't worry about performance implications. This is an old API, available since iOS 2.0. If you put some logs in the forwardTarget functions you will see that they don't get called often.

eliperkins

Awesome work. 💯

Even better would be to add a blurb about this to the README (since those are kind of our docs), but I won't block the PR since we can add that after the fact anyway!

Thanks a ton for doing this, @jaredegan!

eliperkins · 2017-12-11T21:45:27Z

Would anyone else like to take a look over this? I'm good with merging it in, since it seems fairly low risk and gives us a fairly sought after feature, but I'd be great to get some more eyes on it!

dmiluski · 2017-12-11T23:02:01Z

Example/ViewController.swift

+    }
+
+    // MARK: - UITableViewDelegate example functions
+    public func tableView(_ tableView: UITableView, willDisplay cell: UITableViewCell, forRowAt indexPath: IndexPath) {


I can see this item being useful...

But didSelect is covered as part of Static closure handling, and scrollViewWillBeginDragging can be observed with a observer?

The functions here are not comprehensive regarding what methods will get forwarded, they are just meant to demonstrate this functionality. They all get forwarded, including any potential future UIScrollViewDelegate functions. The only ones that do not are the ones that Static handles completely where it doesn't make sense to forward.

I don't think all UIScrollViewDelegate methods can be handled with observers. Developers are more familiar with UIScrollViewDelegate & UITableViewDelegate methods than they are with observing those same changes. This is why Static has at least 3 tickets around requesting this functionality. If the code only forwarded the calls that can't be done with observers, that would obviously create more confusion.

txaiwieser · 2018-01-27T13:10:12Z

Hey guys, when can we have a release with this PR? So we could stop using "master" on carthage

eliperkins · 2018-01-29T16:23:05Z

@txaiwieser Ask and ye shall receive: https://github.com/venmo/Static/releases/tag/v2.3.0

Sorry we didn't do this sooner!

txaiwieser · 2018-01-29T16:26:43Z

@eliperkins Thanks!!! :D

This was referenced Nov 21, 2017

Modify delegate methods on DataSource? #78

Open

add willDisplaySelection and change Selection taking parameter #72

Closed

add possibility to react on will display header and footer #97

Closed

Add tableViewDelegate property, enabling consumers of Static to recei…

b21c5fe

…ve UITableViewDelegate & UIScrollViewDelegate messages from the UITableView. Forward didSelectRow and accessoryButtonTappedForRowWith messages to tableViewDelegate property. Bump version from 2.2.0 to 2.3.0

jaredegan force-pushed the master branch from ab24ef5 to b21c5fe Compare November 21, 2017 19:49

jhaddadnia reviewed Nov 21, 2017

View reviewed changes

eliperkins approved these changes Nov 28, 2017

View reviewed changes

dmiluski approved these changes Dec 11, 2017

View reviewed changes

dmiluski reviewed Dec 11, 2017

View reviewed changes

dmiluski merged commit d6861a5 into venmo:master Dec 11, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add ability to receive UITableViewDelegate & UIScrollViewDelegate messages from UITableView #119

Add ability to receive UITableViewDelegate & UIScrollViewDelegate messages from UITableView #119

jaredegan commented Nov 21, 2017 •

edited

Loading

jhaddadnia Nov 21, 2017

jaredegan Nov 21, 2017 •

edited

Loading

jhaddadnia Nov 21, 2017

jhaddadnia Nov 21, 2017

jaredegan Nov 21, 2017

txaiwieser commented Nov 22, 2017

eliperkins commented Nov 22, 2017

txaiwieser commented Nov 22, 2017

jaredegan commented Nov 28, 2017

eliperkins left a comment

eliperkins commented Dec 11, 2017

dmiluski Dec 11, 2017

jaredegan Dec 11, 2017

dmiluski Dec 11, 2017

txaiwieser commented Jan 27, 2018

eliperkins commented Jan 29, 2018

txaiwieser commented Jan 29, 2018

Add ability to receive UITableViewDelegate & UIScrollViewDelegate messages from UITableView #119

Add ability to receive UITableViewDelegate & UIScrollViewDelegate messages from UITableView #119

Conversation

jaredegan commented Nov 21, 2017 • edited Loading

jhaddadnia Nov 21, 2017

Choose a reason for hiding this comment

jaredegan Nov 21, 2017 • edited Loading

Choose a reason for hiding this comment

jhaddadnia Nov 21, 2017

Choose a reason for hiding this comment

jhaddadnia Nov 21, 2017

Choose a reason for hiding this comment

jaredegan Nov 21, 2017

Choose a reason for hiding this comment

txaiwieser commented Nov 22, 2017

eliperkins commented Nov 22, 2017

txaiwieser commented Nov 22, 2017

jaredegan commented Nov 28, 2017

eliperkins left a comment

Choose a reason for hiding this comment

eliperkins commented Dec 11, 2017

dmiluski Dec 11, 2017

Choose a reason for hiding this comment

jaredegan Dec 11, 2017

Choose a reason for hiding this comment

dmiluski Dec 11, 2017

Choose a reason for hiding this comment

txaiwieser commented Jan 27, 2018

eliperkins commented Jan 29, 2018

txaiwieser commented Jan 29, 2018

jaredegan commented Nov 21, 2017 •

edited

Loading

jaredegan Nov 21, 2017 •

edited

Loading