Navigation Menu

Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Thread Safety, closure captures, debug-only FatalErrors, self-self aliasing, misc tweaks #192

Merged
merged 7 commits into from Feb 9, 2019

Conversation

RamblinWreck77
Copy link
Contributor

Decided to take a crack at fixing #153 and ended up poking around a bit more than I planned. Key points for this PR:

  • Move all FatalErrors to #DEBUG only, as it should not be done in release/prod code. Hard crashing like this will, AFAIK, break people's crash reporting/make crashes seem random.

  • let self=self aliasing is a compiler bug and will be removed in future swift versions, might as well fix now and use a guard inside weak self closures for cleaning up optional chaining.

  • Main thread safety: More enforcement of main thread usage for functions that manipulate UIKit classes. Conversion involved adding asserts, making current functions private and adding a public function that thread casts the private function to main.

  • Closure Captures: Generally closures should capture their inputs (especially if they're classes/reference types) to prevent memory/BAD_ACCESS errors when/if things get changed out from under you. ViewControllers are, in my experience, particularly nasty about this so closure captures help a ton.

Thanks again for an awesome library! It looks like all tests are passing and I'll run this fork in through our QA/production and report back over the next week if any unexpected issues occur

@RamblinWreck77
Copy link
Contributor Author

Hmmm, looks like I introduced a scrolling direction bug. One sec

@RamblinWreck77
Copy link
Contributor Author

Got it! I used auto-complete which didn't include parameters with default arguments in the main thread cast so direction was defaulting to .forward.

@msaps
Copy link
Member

msaps commented Feb 9, 2019

@RamblinWreck77 oh nice! Have you found it's managed to mitigate the internal UIPageViewController crashes? I'll get to reviewing it, thanks! 💯

@uias-bot
Copy link
Member

uias-bot commented Feb 9, 2019

1 Warning
⚠️ Looks like you changed some source files, should there have been some tests added?

SwiftLint found issues

Warnings

File Line Reason
PageboyViewController+Management.swift 13 Lines should not have trailing whitespace.
PageboyViewController+Management.swift 20 Lines should not have trailing whitespace.
PageboyViewController+Management.swift 40 Lines should not have trailing whitespace.
PageboyViewController+Management.swift 54 Lines should not have trailing whitespace.
PageboyViewController+Management.swift 106 Lines should not have trailing whitespace.
PageboyViewController+Management.swift 137 Lines should not have trailing whitespace.
PageboyViewController+Management.swift 160 Lines should not have trailing whitespace.
PageboyViewController+Management.swift 175 Lines should not have trailing whitespace.
PageboyViewController+Management.swift 191 Lines should not have trailing whitespace.
PageboyViewController+Management.swift 193 Lines should not have trailing whitespace.
PageboyViewController+Management.swift 205 Lines should not have trailing whitespace.
PageboyViewController+Management.swift 212 Lines should not have trailing whitespace.
PageboyViewController+Management.swift 234 Lines should not have trailing whitespace.
PageboyViewController+Management.swift 240 Lines should not have trailing whitespace.
PageboyViewController+Management.swift 243 Lines should not have trailing whitespace.
PageboyViewController+Management.swift 253 Lines should not have trailing whitespace.
PageboyViewController+Management.swift 261 Lines should not have trailing whitespace.
PageboyViewController+Management.swift 266 Lines should not have trailing whitespace.
PageboyViewController+Management.swift 270 Lines should not have trailing whitespace.
PageboyViewController+Management.swift 280 Lines should not have trailing whitespace.
PageboyViewController+Management.swift 284 Lines should not have trailing whitespace.
PageboyViewController+Management.swift 295 Lines should not have trailing whitespace.
PageboyViewController+Management.swift 311 Lines should not have trailing whitespace.
PageboyViewController+Management.swift 108 Function parameters should be aligned vertically if they're in multiple lines in a declaration.
PageboyViewController+Management.swift 109 Function parameters should be aligned vertically if they're in multiple lines in a declaration.
PageboyViewController+Management.swift 110 Function parameters should be aligned vertically if they're in multiple lines in a declaration.
PageboyViewController+Management.swift 111 Function parameters should be aligned vertically if they're in multiple lines in a declaration.
PageboyViewController+Management.swift 112 Function parameters should be aligned vertically if they're in multiple lines in a declaration.
PageboyViewController+Management.swift 113 Function parameters should be aligned vertically if they're in multiple lines in a declaration.
PageboyViewController+Management.swift 114 Function parameters should be aligned vertically if they're in multiple lines in a declaration.
PageboyViewController+Management.swift 125 Limit vertical whitespace to a single empty line. Currently 2.
PageboyViewController+Management.swift 41 Line should be 120 characters or less: currently 145 characters
PageboyViewController+Management.swift 146 Line should be 120 characters or less: currently 138 characters
PageboyViewController+Management.swift 148 Line should be 120 characters or less: currently 136 characters
PageboyViewController+Updating.swift 12 Lines should not have trailing whitespace.
PageboyViewController+Updating.swift 27 Lines should not have trailing whitespace.
PageboyViewController+Updating.swift 35 Lines should not have trailing whitespace.
PageboyViewController+Updating.swift 46 Lines should not have trailing whitespace.
PageboyViewController+Updating.swift 52 Lines should not have trailing whitespace.
PageboyViewController+Updating.swift 65 Lines should not have trailing whitespace.
PageboyViewController+Updating.swift 73 Lines should not have trailing whitespace.
PageboyViewController+Updating.swift 76 Lines should not have trailing whitespace.
PageboyViewController+Updating.swift 79 Lines should not have trailing whitespace.
PageboyViewController+Updating.swift 82 Lines should not have trailing whitespace.
PageboyViewController+Updating.swift 86 Lines should not have trailing whitespace.
PageboyViewController+Updating.swift 53 Line should be 120 characters or less: currently 167 characters
PageboyViewController.swift 15 Lines should not have trailing whitespace.
PageboyViewController.swift 17 Lines should not have trailing whitespace.
PageboyViewController.swift 22 Lines should not have trailing whitespace.
PageboyViewController.swift 39 Lines should not have trailing whitespace.
PageboyViewController.swift 41 Lines should not have trailing whitespace.
PageboyViewController.swift 56 Lines should not have trailing whitespace.
PageboyViewController.swift 58 Lines should not have trailing whitespace.
PageboyViewController.swift 67 Lines should not have trailing whitespace.
PageboyViewController.swift 68 Lines should not have trailing whitespace.
PageboyViewController.swift 84 Lines should not have trailing whitespace.
PageboyViewController.swift 137 Lines should not have trailing whitespace.
PageboyViewController.swift 142 Lines should not have trailing whitespace.
PageboyViewController.swift 147 Lines should not have trailing whitespace.
PageboyViewController.swift 175 Lines should not have trailing whitespace.
PageboyViewController.swift 178 Lines should not have trailing whitespace.
PageboyViewController.swift 185 Lines should not have trailing whitespace.
PageboyViewController.swift 188 Lines should not have trailing whitespace.
PageboyViewController.swift 201 Lines should not have trailing whitespace.
PageboyViewController.swift 209 Lines should not have trailing whitespace.
PageboyViewController.swift 228 Lines should not have trailing whitespace.
PageboyViewController.swift 230 Lines should not have trailing whitespace.
PageboyViewController.swift 261 Lines should not have trailing whitespace.
PageboyViewController.swift 298 Lines should not have trailing whitespace.
PageboyViewController.swift 325 Lines should not have trailing whitespace.
PageboyViewController.swift 410 Lines should not have trailing whitespace.
PageboyViewController.swift 433 Lines should not have trailing whitespace.
PageboyViewController.swift 439 Lines should not have trailing whitespace.
PageboyViewController.swift 334 Function body should span 40 lines or less excluding comments and whitespace: currently spans 45 lines
PageboyViewController.swift 68 Limit vertical whitespace to a single empty line. Currently 2.
PageboyViewController.swift 19 Line should be 120 characters or less: currently 127 characters
PageboyViewController.swift 69 Line should be 120 characters or less: currently 125 characters
PageboyViewController.swift 77 Line should be 120 characters or less: currently 130 characters
PageboyViewController.swift 78 Line should be 120 characters or less: currently 152 characters
PageboyViewController.swift 148 Line should be 120 characters or less: currently 130 characters
PageboyViewController.swift 349 Line should be 120 characters or less: currently 133 characters
PageboyViewController.swift 360 Line should be 120 characters or less: currently 129 characters
PageboyViewController+Transitioning.swift 13 Lines should not have trailing whitespace.
PageboyViewController+Transitioning.swift 16 Lines should not have trailing whitespace.
PageboyViewController+Transitioning.swift 29 Lines should not have trailing whitespace.
PageboyViewController+Transitioning.swift 34 Lines should not have trailing whitespace.
PageboyViewController+Transitioning.swift 36 Lines should not have trailing whitespace.
PageboyViewController+Transitioning.swift 51 Lines should not have trailing whitespace.
PageboyViewController+Transitioning.swift 53 Lines should not have trailing whitespace.
PageboyViewController+Transitioning.swift 68 Lines should not have trailing whitespace.
PageboyViewController+Transitioning.swift 73 Lines should not have trailing whitespace.
PageboyViewController+Transitioning.swift 75 Lines should not have trailing whitespace.
PageboyViewController+Transitioning.swift 79 Lines should not have trailing whitespace.
PageboyViewController+Transitioning.swift 135 Lines should not have trailing whitespace.
PageboyViewController+Transitioning.swift 143 Lines should not have trailing whitespace.
PageboyViewController+Transitioning.swift 168 Lines should not have trailing whitespace.
PageboyViewController+Transitioning.swift 23 Types should be nested at most 1 level deep
TransitionOperation.swift 12 Lines should not have trailing whitespace.
TransitionOperation.swift 20 Lines should not have trailing whitespace.
TransitionOperation.swift 32 Lines should not have trailing whitespace.
TransitionOperation.swift 34 Lines should not have trailing whitespace.
TransitionOperation.swift 37 Lines should not have trailing whitespace.
TransitionOperation.swift 39 Lines should not have trailing whitespace.
TransitionOperation.swift 44 Lines should not have trailing whitespace.
TransitionOperation.swift 49 Lines should not have trailing whitespace.
TransitionOperation.swift 52 Lines should not have trailing whitespace.
TransitionOperation.swift 57 Lines should not have trailing whitespace.
TransitionOperation.swift 70 Lines should not have trailing whitespace.
TransitionOperation.swift 74 Lines should not have trailing whitespace.
TransitionOperation.swift 76 Lines should not have trailing whitespace.
TransitionOperation.swift 84 Lines should not have trailing whitespace.
TransitionOperation.swift 97 Lines should not have trailing whitespace.
TransitionOperation.swift 99 Lines should not have trailing whitespace.
TransitionOperation.swift 102 Lines should not have trailing whitespace.
TransitionOperation.swift 104 Lines should not have trailing whitespace.
TransitionOperation.swift 115 Lines should not have trailing whitespace.
TransitionOperation.swift 121 Lines should not have trailing whitespace.
TransitionOperation.swift 129 Lines should not have trailing whitespace.
TransitionOperation.swift 131 Lines should not have trailing whitespace.
TransitionOperation.swift 135 Lines should not have trailing whitespace.
UIView+AutoLayout.swift 12 Lines should not have trailing whitespace.
UIView+AutoLayout.swift 24 Lines should not have trailing whitespace.
UIView+AutoLayout.swift 34 Lines should not have trailing whitespace.
UIView+AutoLayout.swift 47 Lines should not have trailing whitespace.
UIView+AutoLayout.swift 57 Lines should not have trailing whitespace.
UIView+AutoLayout.swift 70 Lines should not have trailing whitespace.
UIView+AutoLayout.swift 80 Lines should not have trailing whitespace.
UIView+AutoLayout.swift 82 Lines should not have trailing whitespace.
UIView+AutoLayout.swift 87 Lines should not have trailing whitespace.
UIView+AutoLayout.swift 97 Lines should not have trailing whitespace.
UIView+AutoLayout.swift 24 Limit vertical whitespace to a single empty line. Currently 3.
UIView+AutoLayout.swift 89 Line should be 120 characters or less: currently 127 characters
UIView+Localization.swift 12 Lines should not have trailing whitespace.
UIView+Localization.swift 25 Lines should not have trailing whitespace.

Generated by 🚫 Danger

Copy link
Member

@msaps msaps left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me 👍 I'll sort the linter errors 😄

@msaps msaps merged commit 51f1ee0 into uias:master Feb 9, 2019
@RamblinWreck77
Copy link
Contributor Author

RamblinWreck77 commented Feb 10, 2019

@msaps That bug was/is particularly rare in our project, but so far it hasn't happened again in prod with these changes so knock on wood!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants