Adds custom typing indicator support

[dzenbot]

Inspired from @sveinhal's implementation in #202.

It now uses KVO internally, observing the visible propery's changes to properly update the view constraints constants and animately present the typing indicator view.
By simply conforming to SLKTypingIndicatorProtocol required methods, it should be very easy to add any sort of custom typing indicator view.

I have added a custom typing indicator view to the sample project:

PS: Thanks @sveinhal for taking the initiative to do this! I felt the need to refactor your implementation for a more friendly approach. Observing the hidden property wasn't enough, since it would force the view to hide when dismissing the typing indicator. I have also taken the time to refactor and improve other related to logic to the default typing indicator view.
This update should be backwards compatible, but might still need some testing before merging.

[sveinhal]

This will probably crash if you try to do something with the returned value, and it isn't really a kind of SLKTypingIndicatorView. You may want to return nil in this case?

[dzenbot]

Good point! Will fix that. Thanks for all your feedback!

[dzenbot]

@sveinhal I've made some fixes, considering all your feedback. Mind having a look once more?

[sveinhal]

Why specify this? The typing indicator class is already required to be a kind of UIView and this class already conforms to this protocol. Also, SLKTextViewController uses - [UIView systemLayoutSizeFittingSize:] internally, so it is not calling this method directly. It is not necessary for correct behavior. In my specific example, I'm also using auto-layout inside the custom typing indicator view, and it works perfectly without supplying an implementation of intrinsicContentSize, since all of it's subviews either do implement it, or have explicit size constraints (and explicit margins to the superview).

I have a view that has two subviews. Both of these have explicit height and width constraints, as well as explicit margins to the superview, and inter spacing between them. Therefore, systemLayoutSizeFittingSize: is able to deterministically deduce a size, without any of the views implementing intrinsicContentSize

[sveinhal]

Anyways: Having this requirement here does nothing (except maybe set expectations to the developer). Leaving an implementation out keeps the compiler silent, since the UIView implements it.

[dzenbot]

You're right! I totally forgot we were using systemLayoutSizeFittingSize: now, which is way better for dynamic sizes. I'll remove that required method from the protocol.

[dzenbot]

You're right! I totally forgot we were using systemLayoutSizeFittingSize: now, which is way better for dynamic sizes. I'll remove that required method from the protocol.

[sveinhal]

I'm satisfied with this, but I think the quirks of implementing a custom protocol-conforming class in Swift justifies a note somewhere in the README.md file or something. It's not really intuitive, but also there are not simple solutions that either won't break Obj-C conventions (by having the setter and getter share name), or break automatic KVO (by not declaring it a property, but instead specify the setter and getters as methods in the protocol).

Anyways, you get my 👍 for merging this!

[sveinhal]

Contra: I discovered a bug: If the scroll view is scrolled up when setting visible = false does not cause the height constraint to be set to 0. Why?

[sveinhal]

I don't understand this requirement:

    // Don't show if the content offset is not at top (when inverted) or at bottom (when not inverted)
    if ((self.isInverted && ![self.scrollViewProxy slk_isAtTop]) || (!self.isInverted && ![self.scrollViewProxy slk_isAtBottom])) {
        return NO;
    }

Maybe that should be configurable. Also, it behaves different when showing/hiding.

[dzenbot]

About -canShowTypingIndicator: this logic was actually a request for Slack's app. The goal was not to show the typing indicator when the scrollview's offset wasn't at the bottom, to avoid making the view jump constantly (specially in very busy channels). This might not be generic enough for others, so I can perhaps remove it from SLKTVC's base code. I wonder if people are depending of this behavior tho.

[sveinhal]

I guess I can override the method in my subclass of SLKTVC ;-)

[dzenbot]

Yes, definitely. That's what I did in https://github.com/slackhq/SlackTextViewController/blob/5c4939325d04462aaf172c1badd0b7b60ef1e316/Examples/Messenger-Shared/MessageViewController.m#L354-360

[sveinhal]

But when leaving it on, it still had a bug. The typing indicator would display, but not hide when scrolled up. (I didn't try to see if it would hide if I scrolled up while it was visible, but that's another issue)

[sveinhal]

Anyways: If you clean up the protocol, and preferably add a note about Swift in the read me, I think this is production quality :-) Great work!

[dzenbot]

Sounds good! What do you mean by "clean up the protocol"?
Bw, the typing indicator will not hide automatically. The default typing indicator does, because it has a timer internally. You will need to implement all that yourself.

[sveinhal]

Sounds good! What do you mean by "clean up the protocol"?

Remove the intrinsicContentSize requirement

Bw, the typing indicator will not hide automatically. The default typing indicator does, because it has a timer internally. You will need to implement all that yourself.

Sure, but calling visible = false, would make the observer fire, and it would set the layout constraint to 0. But if this happens while scrolled up, it does nothing, because canShowTypingIndicator returns NO. However, when setting visible = true under the same circumstances, canShowTypingIndicator returns YES, which makes it display, but never hide.

It may be some quirks in the frame calculations again. I'm not displaying my VC full screen ¯_(ツ)_/¯

I've also confirmed that this isn't a regression, so it is unrelated to this pull request.

[dzenbot]

Remove the intrinsicContentSize requirement

That's done in 52c201e

Sure, but calling visible = false, would make the observer fire, and it would set the layout constraint to 0. But if this happens while scrolled up, it does nothing, because canShowTypingIndicator returns NO.

Are you overriding -canShowTypingIndicator like this? Without calling super?

- (BOOL)canShowTypingIndicator
{
    return YES;
}

[sveinhal]

Are you overriding -canShowTypingIndicator like this? Without calling super?

Now I am (or rather — the Swift equivalent). So that solved my problem. Now it's always displayed or hidden with a nice animation whenever I set the visible property of my class.

However, before I added this, and just let the SLKTVC handle it, it would prevent the view from being hidden, but not prevent it from being displayed.

However, this is also the current behavior for my particular view hierarchy in master as well, so I suggest we close this discussion, and perhaps open another issue for it, if needed :-)

[dzenbot]

👍

[sveinhal]

Over at appear.in we usually give each other a cake when something is ready to be merged.

🍰

[dzenbot]

Thanks for all your input @sveinhal ! 🙌

[sveinhal]

Thank you for creating a great open source component, and also a great product which we use every day both on iOS and the desktop. And: Which has great integration with the stuff I'm working on every day: https://appear.in (shameless plug)

Seems like people like us:

[hmaidasani]

Do you have an example of this in Swift?