Custom name/properties for auto screen reporting

[humblehacker]

What does this PR do?

Adds the ability to specify a custom name and custom properties during
automatic screen reporting.

Also fixes seg_topViewController() for UITabBarController and custom
container view controllers.

Where should the reviewer start?

UIViewController.seg_viewDidAppear()

How should this be manually tested?

• Create a UIViewController, and conform to the new SEGScreenReporting
  protocol.
• Implement seg_trackScreen to override the default screen tracking. Your implementation should call seg_screen with custom values for the screen, properties and/or options arguments.
• Implement seg_mainViewController to specify which child view
  controller of a custom container view controller should be referenced
  when determining the top view controller (seg_topViewController())

Any background context you want to provide?

None

What are the relevant tickets?

Fixes #654

Screenshots or screencasts (if UI/UX change)

No UI/UX change

Questions:

• Do the docs need an update?

  Yes

• Are there any security concerns?

  Not that I'm aware of

• Do we need to update engineering / success?

  Not sure what this means

@segmentio/gateway

[codecov-io]

Codecov Report

Merging #771 into master will decrease coverage by 1.87%.
The diff coverage is 0%.

@@            Coverage Diff             @@
##           master     #771      +/-   ##
==========================================
- Coverage   86.89%   85.02%   -1.88%     
==========================================
  Files          52       52              
  Lines        2793     2691     -102     
==========================================
- Hits         2427     2288     -139     
- Misses        366      403      +37

[humblehacker]

Is there anything in this PR that you'd like to see changed?

[f2prateek]

hey @humblehacker sorry for the delay.

I do have a question before diving too much into the implementation. If a customer has to write code to get this to work (i.e. they have to implement the new protocol), wouldn't it be just as easy to call [[SEGAnalytics sharedAnalytics] screen:...]?

[humblehacker]

Thanks for the response. Yes, they could just call screen themselves, but they'd have to either make that call manually for every view controller (losing the automatic screen tracking), or reimplement the UIViewController swizzling already performed by the Segment SDK. If they try to do both (use automatic tracking as well as manual calls to screen), they would get multiple screen events for the same view controller presentation.

[f2prateek]

So far automatic screen recording is very binary - you either use it or you don't. I would like to avoid complicating the feature set too much.

But agreed it would be nice to have a way to customize the calls somehow.

Maybe.... instead of a protocol with three properties, we could have the protocol be something like this:

@protocol SEGScreenReporting
- (void)seg_screen;
@end

Then if our library detects this, instead of calling screen directly, it calls the seg_screen method and the customer can make the actual screen call.

Another similar idea would be:

@protocol SEGScreenReporting
@optional @property (readonly, nullable) NSString * seg_screenOptOut;
@end

Then if our library detects this, it doesn't call screen at all and lets the customer record screen manually.

It's a bit hard to decide since we don't have a lot of customers who have really asked for this yet.

I would suggest we leave this open so folks can leave some feedback on this before we decide to proceed. How does that sound?

[humblehacker]

That's a reasonable compromise -- accomplishes the same goal with slightly less complexity. I prefer your first suggestion over the second. Though maybe seg_trackScreen might be a better name?

This eliminates two properties from the protocol, but the third property (seg_mainViewController) serves a different purpose of allowing custom container view controllers to participate in automatic screen tracking (related to #654). Thoughts?

I'm fine with leaving this open for a while to gather feedback.

[humblehacker]

@f2prateek I've updated the PR to simplify the protocol, as you suggested. Thoughts?

[humblehacker]

Hello again. Is there anything I can do to help move this along?

[f2prateek]

Sorry for the delay! I think this looks reasonable to me - could we add some tests?

I don't anything else is blocking this - cc @fathyb to hear your thoughts too!

[timbroder]

Is there anything left to move this along? We have a tab bar app and would like to use auto tracking :)

[itsallmememe]

Is there any way to do auto-screen tracking with container views? I can't seem to find any information about this feature, and this PR sounds like it will fix the issue.

[humblehacker]

I've rebased against master and added some tests. Let me know if there's anything else I can do.

[humblehacker]

Not sure why, but AnalyticsTests.swift hangs on my machine, and it looks like the same on CircleCI. Nothing to do with this PR though. I validated the same issue occurs against segmentio:master.

[humblehacker]

I realize it's been a while since this PR was opened. Is there any chance for its acceptance?

[bsneed]

Thanks @humblehacker !! Sorry for the extreme delay getting this in. I did the fixups needed and made sure the tests were kosher. Thanks again!!

[bsneed]

ps. #884 is the PR for this in progress release where your changes will come in.

[kowongh]

Hey thanks for this fix! It's exactly what we were looking for.

Is there a way to not have the title of the viewController override the class name? It's a bit clunky to have some view controller's screen names be overridden with title but going with the convention of the ClassName minus ViewController on others. I think if you wanted automatic screen tracking on, it should just conform to the ClassName minus
'ViewController' convention and not mix the title in there, which is why you would use the custom naming protocol.

[bsneed]

@kowongh there's not currently a way to do that. You wanna throw up a PR with the logic changes you'd like and I can spin another release once we've agreed?

[kowongh]

@bsneed Thanks Brandon. Here's my requested fix for making the naming convention more consistent for the recordScreenViews feature. #885

[simonbromberg]

Your implementation should call seg_screen with custom values for the screen, properties and/or options arguments

I don't see seg_screen has this changed?

I don't understand how to implement this protocol, an example or some documentation would go a long way.

extension MyViewController: SEGScreenReporting  {
  public func seg_trackScreen(_ screen: UIViewController, name: String) {
        // Now what?
  }
}

EDIT: someone has posted an example in #885.