Code refresh + SPM support #848
Conversation
There was a problem hiding this comment.
Hi @Climbatize thank you for contributing and for the heads up. Sorry for the delay. I left just two comments on the code changes. Did you also test installing these changes through Cocoapods?
| @IBOutlet weak var userImage: UIImageView! | ||
| @IBOutlet weak var postName: UILabel! | ||
| @IBOutlet weak var postText: UILabel! | ||
| @IBOutlet var userImage: UIImageView! | ||
| @IBOutlet var postName: UILabel! | ||
| @IBOutlet var postText: UILabel! |
There was a problem hiding this comment.
Regarding making the IBOutlets strong, could you point me to documentation stating that weak is not recommended? I read the opposite in this doc.
Also connecting an Outlet from Xcode defines it as weak by default
There was a problem hiding this comment.
I admit Apple documentation is confusing. So I remade the IBOutlets from XCode just to confirm. Also, doc specifies The strong outlets are frequently specified by framework classes (for example, UIViewController’s view outlet, or NSWindowController’s window outlet), that's why I did this change. I honestly think it's more dangerous to have it as weak than strong, as I already experienced crashes related to weak outlets in production, but I can revert if you think it's better.
Screen.Recording.2023-07-26.at.9.07.59.mov
There was a problem hiding this comment.
I have seen both happen by default. For example when you build a UIViewController and want to connect its view to the controller class it defaults to strong (as that view should be strong). But for other views which are subviews of the main view it will default to weak (because subviews will be strongly referenced by their parent). This means as long as a view is in the view hierarchy it should not be deallocated and you should have no issue. However if you plan to remove the view you will have to strongly reference it if you plan to continue using it afterwards.
I think it is best to revert unless there are reproducible crashes which I would rather think could be linked to how the views are used. It might make sense to switch some of the outlets to strong if the use cases require it (rather than just switching everything)
| import FXPageControl | ||
| #endif | ||
|
|
||
| extension UIWindow { |
There was a problem hiding this comment.
I think this extension should be moved to a separate file named something like UIWindow+Pager.swift or so
ObjC/FXPageControl.h
Outdated
| @@ -1,7 +1,7 @@ | |||
| // | |||
| // FXPageControl.h | |||
| // | |||
| // Version 1.4 | |||
| // Version 1.5 | |||
There was a problem hiding this comment.
Moving this file out of the Sources folder will make it not be included in the library when installed through Cocoapods and it is also not marked as dependency. Please either revert or add FXPageControl as dependency in the podspec.
There was a problem hiding this comment.
That's a big mistake from my side! I ran a pod spec lint XLPagerTabStrip.podspec instead of pod lib lint XLPagerTabStrip.podspec. Fix is on the way
Moved UIWindow extension to an independant file, updated podspec to take objective C files refactoring in account
|
Thanks @mats-claassen for the quick and kind review! I left my comments and provided update based on your feedbacks. |
|
@Climbatize changes look good. Just the matter of the weak outlets remaining |
|
I reverted changes on weak outlets. At first sight no crashes from my side. I think we're good to merge @mats-claassen :) |
|
Thanks for the merge @mats-claassen ! Don't forget to tag the master branch to a minor version greater than 9.0.0 so the release is accessible, or to edit the the readme file to tell devs to point to master branch |
This PR is used in production since the last 9 month with no issues so far. (through this SPM dependency: https://github.com/Climbatize/XLPagerTabStrip.git > 9.1.0)
I know there is already this good PR #846 aiming to bring support to SPM, but I think this lib needed a deeper refresh to remove some deprecations.