Fix divider counter issue and deadlock when initializing the view programatically.

[ppamorim]

Hi, with this PR I will fix the mismatch divider counter caused when you set up the view manually. To solve this I replaced the use of self.items.count with self.segments.count, I didn't see any regression, please let me know if you see anything wrong.

Most part of the code addition is from the ProgramaticallyViewController.m file. The changes done in the TOSegmentedControl.m are minimal, please have a look.

List of what has been done:

• Added an option Programatically in the Third item, when you select it, it opens a UIViewController that has been created programmatically and does some invalid setup, with this setup I could reproduce all the problems and sort them, especially the deadlocks that I found.

• Modified the function insertSegment(_,at:) to return if the segment could be or not insert into the component. For me, this change was needed because if it sends an index bigger than the items.count, the function insert to crash the application.

• Added the functions count() and isEmpty(). They are useful for the function above.

• Added one more example where it's basically an empty TOSegmentedControl. In the current code, if you find this situation and click on the component, your application will crash. I simply added a check to assert if the component is not empty. If it is, nothing happens.

• Added a function that disabled the click if the user pressed and leaves the area pressed. With the current version, if you do the action described below:

The "Second" view will get stuck in the pressed mode. With this PR I fixed it.

• Added the function setSelectedSegmentIndex:int:animated that makes the TOSegmentedControl animate the transition.

Please let me know if you find any problem. :)

[TimOliver]

Hi @ppamorim! Thanks for the PR!

Oh wow. That's a LOT of code changes. I feel like we could solve the issues with a bit less code.

1. It probably doesn't make a lot of sense to add ProgramaticallyViewController to the sample app, because it's fixing a developer detail and not really demonstrating anything (The point of the sample app is to demonstrate the control to people). A lot of people trying the sample app might be confused at what the point of it is. It might be better to make that a separate sample app that can be used to test. :)
2. I'd rather not deviate from the same API that Apple used. Having a look at the UISegmentedControl documentation, if an index is too high, then the segment is just attached to the very end. This might make more sense than managing complexity by checking the BOOL values.
3. These don't really apply if we can avoid making the insertion methods return BOOL.
4. Again, it might be better to put crash examples in a separate app, and not the sample app.
5. Tapping down and leaving the area is intended functionality. That's how the real UISegmentedControl works. :)
6. Okay cool. That sounds like a legitimate bug fix. Thanks for that. :)

I think for the time being, can you please prepare a separate sample app that shows me what the exact issues you're having are? I'm pretty confident we can fix all of this with far less code, and not needing to change the sample app. :)

Thanks!

[ppamorim]

Hi @TimOliver,

1 - The ProgramaticallyViewController exists to show the user how to use the library purely by code, today the only example was through Storyboard and it didn't catch crash situations where the pure code did. You can test this by removing my fixes and running that view controller. I am sorry for the massiveness but it's mostly the fault of the NSLayoutConstraints verbosity, the class for the example is minimal. I am working to simplify it. 2 - Ok I will change that. But I don't get your argument since the library includes add and insert functions and the Apple's one has only insert, this is a deviation in my opinion. 3 - I think it's a lacking feature in the Apple SDK, why it could not be an addition to the library? I don't see the harm. 4 - I totally disagree, any possible crash that happened with a specific configuration should inserted in a demo (to show to our users that the feature can be used and its application will not crash) or UI test. 5 - I don't think you get the issue, please have a look at this:

(Note that both First and Second are under pressed state)

This happens when you add the component in a UIViewController with the modalPresentationStyle set to UIModalPresentationPageSheet and move away from the component. Please remove this line below in my example to reproduce the problem:

viewController.modalPresentationStyle = UIModalPresentationFullScreen;

(I have done that for you, just pull my changes. :)

6 - Thanks :)

EDIT: I simplified the NSLayoutConstraints constraints configuration, now the ProgramaticallyViewController is below 200 lines.

[TimOliver]

Thanks @ppamorim. While it's fine to disagree, once this PR is merged, it becomes my responsibility forever to manage and maintain the code moving forward, so I need to be happy with it before I can accept it. 😉

It might have been better to discuss this in an issue beforehand. 😅

In any case, what I said before stands. Please do not modify the sample app, nor the public headers for this bug fix. 🙏

If you absolutely need boolean logic for your app, it might be better to fork this project so you can tailor it specifically for your needs. :)

[ppamorim]

Could you please give me a feedback for points 2 to 5?

[TimOliver]

Okay. But I've run out of time tonight, so this will have to be the last message from me.

2. add is just a convenience on insert, but changing void to a BOOL is a fundamental shift on how the methods behave. In this case, even though it may be a bug on my behalf that they do now, these functions should never ever fail. So there should be no need to expose a BOOL function (Just, add default behaviour if the developer adds an invalid index).
3. There's already numberOfSections publicly available that can be used to test both, so these ones are redundant. In contrast self.segmentedControl.count doesn't make sense on its own, and self.segmentedControl.isEmpty is somewhat vague. isEmpty might be acceptable for use as an internal convenience property though.
4. It's true that there should be a way to capture crashes so they can be tested in future, but not at the expense of polluting the sample app. It's not even standard iOS UX to have a view controller present from selecting a segment control. Usually I'd prefer to only have one instance of the control, and then have the ability to test it by having code for each of its properties, but commented out by default. In any case, we don't need another view controller for now. :)
5. Ah, I tried it now. I see what you mean. Because it's contained in a scroll view, when the user attempts to scroll, the segmented control gets stuck. Okay that's a legitimate bug. Well picked. It should be relatively easy to fix by just adding a UIControlEventTouchCancel handler to the UIControl logic, but that should be in a separate issue.

I hope that all makes sense. Regardless, thanks for your hard work! :)

[ppamorim]

@TimOliver Thank you for the answer. I will implement the changes described below:

2 - I will revert this change back. I still think add can be confusing and redundant when we fix the insert function, maybe we should be very strict with the UISegmentedControl specification. Please let me know if I am wrong to think that add is redundant, I am probably missing something? 3 - Understood, I am removing this. 4 - I agree with you, I can refactor this and remove it from the view itself. But I still think that would be nice to have at least one example of usage of this component programmatically, not everyone use storyboard and this can be confused in how we recommend to use it outside of this scope. We can even add this in the README to let users know that the component has been tested and how we recommend to initialize the view programmatically. Most of the library I know don't implement this type of initialization and also most of them have problems when doing it. The same happens with views that the developer focus too much in the programmatic initialization and forgets to implement the storyboard one.

Pedro, :)

[ppamorim]

@TimOliver I did some essential changes in this PR as requested, could you please review it? I also removed the DEVELOPMENT_TEAM by accident, but should it be restored? What's the point to have the ID in the repository since it cannot be used anyway?

[TimOliver]

Cool. Thanks for refining it! It looks good enough for me! I'll pull it into a separate branch and give it some final touch-ups for code consistency.

Instead of the auxiliary view controller, I'll make sure to update the README with those instructions, and make some unit tests to ensure there's never any regressions.

Thanks a lot for your hard work on this @ppamorim! :)