Automatic Screen Tracking with consistent naming conventions

[kowongh]

What does this PR do?
This simply reverses the logic for naming conventions regarding automatic screen tracking in an attempt to provide more consistent naming conventions. The current approach is sacrificing consistency for readability which muddies up data. With the addition of the custom naming protocol, we don't need title anymore.

Assuming that all view controllers have a somewhat unique name (other than ViewController which is specified in the logic), we can assume that the automatic screen tracking will have a consistent naming convention throughout all screens (rather than having a mix of both ClassName minus "ViewController" and title), since all view controllers will not have a title i.e NavigationItem.

Furthermore, if two view controllers have the same title, then they will show up as having the same name value. Using class name is a better way to uniquely identify a screen.

With the addition of the custom naming protocol, title as a first choice naming convention is a bit redundant because you can conform to the protocol if you need a custom or more readable name such as the title of the view controller.

Where should the reviewer start?
UIViewController.seg_viewDidAppear()

How should this be manually tested?
Add recordScreenViews to your config, then look at the names of the events reported.

Any background context you want to provide?
With the automatic screen tracking feature we want a consistent naming convention without overriding or conforming to protocols.

What are the relevant tickets?
Related to #771

Screenshots or screencasts (if UI/UX change)

Questions:

• Does the docs need an update?
  Yes

• Are there any security concerns?
  No

• Do we need to update engineering/success?
  N/A

[codecov-io]

Codecov Report

Merging #885 into releases/3.8.1 will decrease coverage by 0.02%.
The diff coverage is 0.00%.

@@                Coverage Diff                 @@
##           releases/3.8.1     #885      +/-   ##
==================================================
- Coverage           87.57%   87.55%   -0.03%     
==================================================
  Files                  55       55              
  Lines                3220     3221       +1     
==================================================
  Hits                 2820     2820              
- Misses                400      401       +1     

[camwest]

Hey folks,

FYI we just upgraded to 3.9 and our analytics broke. See attached screenshot.

I wish this change was made optional!!!

[camwest]

FYI we also used to get nice messages for all the UIAlertControllers in our app. Now we only get a single generic event.

[bsneed]

@camwest the change is optional. pin your pod or Carthage version to a specific release. This comment is also void of actual details. The chart doesn't shed light on anything at all other than you're having issues. Happy to work through them, but please file an issue with some details.

[simonbromberg]

Hi @bsneed, I work with @camwest —

• We have already updated to the latest version of Segment to get the newer features in 4.1.2. Downgrading at this point is not a viable option. This change could have been introduced without it being a major breaking change, e.g. something that you could opt into.
• This change makes a flawed assumption that everyone uses custom view controllers for every view — this is not the case in our project as we have many instances of custom UIView instances that are loaded into a generic ViewController (which because of this change is also rendered completely blank because it parses out that part of the name)
• This change breaks tracking on UIAlertController as it i) does not contain "ViewController" and ii) it is a commonly reused view controller that one would want to track but it is almost never subclassed.
• These breaking changes are easily missed as it doesn't create a compiler error but rather silently breaks analytics after you've released. Not everyone is going to catch the one line in the release notes or realize its implications. Honestly it seems a bit reckless for a product like Segment to merge something in like this without considering the implications for its customers.

I have also commented on #771 that I don't understand how to implement the custom naming protocol. There's no example nor is this mentioned in the documentation anywhere Also submitted a support request via segment.com.

[bsneed]

Hi @simonbromberg,

Looking at the logic here, if your controller name is UIAlertController I don't see how that would fail to go through.

- (void)seg_viewDidAppear:(BOOL)animated
{
    UIViewController *top = [[self class] seg_rootViewControllerFromView:self.view];
    if (!top) {
        SEGLog(@"Could not infer screen.");
        return;
    }

    NSString *name = [[[top class] description] stringByReplacingOccurrencesOfString:@"ViewController" withString:@""];
    
    if (!name || name.length == 0) {
        // if no class description found, try view controller's title.
        name = [top title];
        // Class name could be just "ViewController".
        if (name.length == 0) {
            SEGLog(@"Could not infer screen name.");
            name = @"Unknown";
        }
    }

    if ([top conformsToProtocol:@protocol(SEGScreenReporting)] && [top respondsToSelector:@selector(seg_trackScreen:name:)]) {
        __auto_type screenReporting = (UIViewController<SEGScreenReporting>*)top;
        [screenReporting seg_trackScreen:top name:name];
        return;
    }

    [[SEGAnalytics sharedAnalytics] screen:name properties:nil options:nil];

    [self seg_viewDidAppear:animated];
}

You'll have a value for name at any point. If it's UIAlertController, that's what the value of name is going to be. The "ViewController" bit is getting stripped off if present, otherwise you're getting the actual class name. If you want to do something more custom, conform to SEGScreenReporting. You can also call screen(...) on your own if needed.

This appears to be extremely straight forward to me, so I'm not sure why you're throwing around words like reckless and what not. I'm trying to assist, there's no need to be disparaging.

As I mentioned to @camwest, please file an issue if I'm missing something.

[simonbromberg]

I think a certain level of quality is expected given the cost of Segment, I don't think that how this PR was merged in is aligned with that expectation. It seems pretty reckless to me; it wasn't intended as a personal attack on anyone in particular. I certainly appreciate the help, but I wish this hadn't been necessary in the first place. I don't think it's unreasonable to expect a project like this to introduce changes like this gracefully or if it's absolutely necessary to break things then it should introduce compiler errors so it's obvious there's something that needs to change. I also did not appreciate the suggestion that the solution here is to peg my project to an outdated version of the Segment library.

The UIAlertController issue is just a side issue here. And I didn't say it failed to go through, the problem is that the event is no longer being uniquely tracked based on the title of the alert.

The primary issue is we have lots of instances of a class called ViewController which load in a variety of UIView subclasses. Because of this change, all those events are being lumped together with no title.

I am trying to conform to SEGScreenReporting but it is not obvious how it works and there is no mention of it in the documentation nor are there examples explaining how to implement it in the documentation nor in that PR. Also, the damage is already done as we have a live version of an app where many of the analytics events are now useless. This is not something that could have easily been anticipated.

[bsneed]

@simonbromberg we'll have to agree to disagree.

Here's the code you want:

@interface MyAlertController: UIAlertController<SEGScreenReporting>
@end
@implementation MyAlertController
- (void)seg_trackScreen:(UIViewController*)screen name:(NSString*)name
{
    // construct a new name however you want.
    // use MyAlertController in place of UIAlertController.

    [[SEGAnalytics sharedAnalytics] screen:[NSString stringWithFormat:"MyAlertController-%@", name] properties:nil options:nil];
}
@end

[bsneed]

my bad, fixed. I'm sure you could see where it was going.