removed asynchonous code in setup method

[n13]

I had a crash with bad access in the setup method, where it's running in the background thread.

Unfortunately I didn't save the exception.

It was on the simulator and only happened once out of maybe 30 starts but I really don't want this to happen in my app on a customer's device. Especially because it was so rare. Those bugs are hard / impossible to debug.

IMO it's a mistake to do anything asynchronously that has to do with views. Views must be modified in the main thread. They are single threaded. I recall this being mentioned in Apple's documentation somewhere too.

Submitting this as change request - integrate it if you agree, or not if you don't ;)

I am grateful you wrote this code, so this is giving back.

[filipealva]

Hello @n13

I'm even more grateful you've submitted this change request.

I agree with you and Apple docs, this is something that I wasn't confident about, I made it as a workaround because when the PickerView was called in a modally presented view controller it was kind of "flashing", appearing to have a performance problem, but only during the transition. I thought that it could be happening because there was so much things happening at the same time within the initialization of PickerView, then I made a clean up in the initialization process and also made this mistake of putting these calls in background. The flashes problem during the transition has been solved, but it maybe the cause of these crashes.

I made some tests to verify if it is working properly just putting the code on the main thread. Unfortunately it causes a problem on setup and the rows height are not being calculated right.

I do accept your change request, but we will need to work a little bit more to bring this code to the main thread without any impact on PickerView behavior.

Below an example of the problem (look at the line that indicates a selection, it is wrong placed because of the wrong calculated row height):

[n13]

Hi!

Happy you like it! I think it's definitely the right way to go. Surprised it impacts calculations - I didn't really change the code but I guess there may have been a dependency where some things depended on getting called a little later.

In fact now I am writing this I'm pretty sure that's what it is. Initialization sequence bugs can be a pain but I am very happy to fix that

I will see if I can reproduce the issue in the example app and if I can will provide a fix for that

Sorry I just fired off the changes without thinking too much (or testing), didn't know if you would agree ;)

It works for my case because I'm not showing the selector line in my application. So I never saw this.

cheers,

Nik

On Aug 27, 2016, at 1:02 AM, Filipe Alvarenga notifications@github.com wrote:

Hello @n13 https://github.com/n13 I'm even more grateful you've submitted this change request.

I agree with you and Apple docs, this is something that I wasn't confident about, I made it as a workaround because when the PickerView was called in a modally presented view controller it was kind of "flashing", appearing to have a performance problem, but only during the transition. I thought that it could be happening because there was so much things happening at the same time within the initialization of PickerView, then I made a clean up in the initialization process and also made this mistake of putting these calls in background. The flashes problem during the transition has been solved, but it maybe the causes of these crashes.

I made some tests to verify if it is working properly just putting the code on the main thread. Unfortunately it causes a problem on setup and the rows height are not being calculated right.

I do accept your change request, but we will need to work a little bit more to bring this code to the main thread without any impact on PickerView behavior.

Below an example of the problem (look at the line that indicates a selection, it is wrong placed because of the wrong calculated row height):

https://camo.githubusercontent.com/15735948e7215e4bf476f5b4a7239cd9ecc5349e/687474703a2f2f672e7265636f726469742e636f2f4943504a6c574d4a75522e676966 — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/filipealva/PickerView/pull/15#issuecomment-242807345, or mute the thread https://github.com/notifications/unsubscribe-auth/AAD_hDHs5yfjkj_Qc5aIxRVRaDDoB4icks5qjyoegaJpZM4JtCpQ.

[n13]

Ok I fixed that bug - actually the table cell position was off, not the row height. E.g. row height was correct but cell position was incorrect.

I believe it has to do with some contraints modifying code in selectedNearbyToMiddleRow

However, the simplest and risk-free fix was to return the delayed execution of the last two setup items to the code. That's OK - it's running on the main queue and won't cause any crashes.

I'd like to remove this though

As performance is concerned, do you think it would be better to do all of the static setup inside the view's init() method? Or does it not matter? Might not I guess as it's all on the main queue. Just curious because there is a ton of code getting run on the first layoutSubviews.

As a general comment, I admire you can deal with constraints in this way, but if you like a method that is 100 times easier both for coding and reading, look at SnapKit. I have moved away from Interface Builder and using only SnapKit to build all views in code now. SnapKit makes constraints setup super easy. It's just a wrapper around Apple's API. But unlike Apple's API, it's actually beautiful and easy to use. ;)

[n13]

I also noticed there's two "reloadData" calls to table view on setup. Needs only 1. I also didn't change this though I want to keep this commit risk free and not start optimizing the code.

[n13]

On the fix - I guess the dependency is that selectedNearbyToMiddleRow somehow needs to be called after some other setup has occurred. I tried with superview but that wasn't it; the delay of running it "later" makes it work. Maybe the view has to be showing on screen? Something like that. Just speculation.

[filipealva]

@n13 Hey,

Sorry about the delay. I'm very ashamed about it, but I had to travel to some places and solving a lot of problems that were going on within the company I'm running.

Thank you for your comments, I've loved to know your point of view.

1 - I guess the dependency is that selectedNearbyToMiddleRow somehow needs to be called after some other setup has occurred. I tried with superview but that wasn't it; the delay of running it "later" makes it work. Maybe the view has to be showing on screen? Something like that. Just speculation.

A: Yes! Your guess is correct. In fact it needs to be called after the superview is visually "ready". I say it because the view doesn't need to be appearing on the screen, it just needs to have finished the layout of subviews, i.e. the call need to be associated with the execution of viewDidLayoutSubviews of the view controller that owns it. Somehow, someway delaying its call makes it happen after the execution of viewDidLayoutSubviews and everything works properly.

2 - I've tested the project here and it looks like the bug was fixed, but I'm wondering if the code lines below are the better ones to delay the execution of this code.

dispatch_async(dispatch_get_main_queue(),{
    // Some UI Adjustments we need to do after setting UITableView data source & delegate.
    self.configureFirstSelection()
    self.adjustSelectionOverlayHeightConstraint()
})

I mean, are you sure it always delays the execution? It seems that you are just grabbing the code which is already running on the main thread and reinforcing that it should run on the main thread using the dispatch_async. I don't know exactly how it ends delaying the execution ¯(ツ)/¯

(I will wait your answer on it to merge the pull request)

3 - As performance is concerned, do you think it would be better to do all of the static setup inside the view's init() method? Or does it not matter? Might not I guess as it's all on the main queue. Just curious because there is a ton of code getting run on the first layoutSubviews.

A: It do think it would be better to do all of the static setup inside the view's init() method, but I have a lot of initialization problems caused by methods being called on the wrong sequence (as the problem with selectedNearbyToMiddleRow) then I ended organizing the code this way.

I'm sure it could improve a lot. I just released the project that way because I needed to reach deadlines, so I decided to release this minimum stable version of PickerView and then, with help of awesome people like you, start to optimize the project (which I have to say: are happening too slow and thats my fault).

Basically the summarized answer for this one is: You are right, doing that way is probably better, but we need to refactor it with care to make sure everything will be working properly after that.

4 - I also noticed there's two "reloadData" calls to table view on setup. Needs only 1. I also didn't change this though I want to keep this commit risk free and not start optimizing the code.

A: The answer for 3 is valid for this one. I will create issues pointing these enhancements.

5 - As a general comment, I admire you can deal with constraints in this way, but if you like a method that is 100 times easier both for coding and reading, look at SnapKit. I have moved away from Interface Builder and using only SnapKit to build all views in code now. SnapKit makes constraints setup super easy. It's just a wrapper around Apple's API. But unlike Apple's API, it's actually beautiful and easy to use. ;)

A: Thank your for the admiration, dealing with Apple's Autolayout API is really painful, hehe. And thank you soooo muchh for letting me to know about SnapKit. I just started using it in another project and it is way better 👍

Thats it! I'm going to sleep right now, It is 2:20 A.M in Brazil :P I will wait until tomorrow for you to answer what I've pointed in 2 to merge it and launch a version with this update for swift 2 besides launching a version supporting swift 3.

If you can't answer I will merge it anyway, but I would love to hear your thoughts about delaying the calls using dispatch_async within the main queue.

[filipealva]

@n13 Hey I just mentioned you in my last medium post