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

iOS 17 state restoration crash fix #767

Merged
merged 5 commits into from
Sep 20, 2023
Merged

Conversation

michalrentka
Copy link
Contributor

The app crashed on iOS 17 during state restoration due to changes in #762. It crashed because

let tagFilterController = (self.viewControllers.first as? MasterContainerViewController)?.bottomController as? TagFilterViewController
was called before was called and so the bottomController tried to add itself into bottomContainer, which didn't exist yet.

I fixed it by creating bottomController and not adding it as subview immediately, but I moved adding it as subview to viewDidLoad function together with other UI components.

I also noticed something I didn't notice during code review of #762 and that is that the distinction between MasterCoordinator and MasterTopCoordinator was not needed anymore, because the MasterContainerViewController took that role and connects "top" and "bottom" controllers together. The MasterCoordinator was almost empty anyway at this point, so I refactored a little and removed it.

Last fix is in

showBottomSheet(!splitViewController.isCollapsed)
. Currently on state restoration, when you closed the PDF, the bottom tag picker would just pop up without animation, because it appered in viewDidAppear which is called too late. I moved it to viewIsAppearing which seems to have better effect.

Copy link
Contributor

@mvasilak mvasilak left a comment

Choose a reason for hiding this comment

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

viewIsAppearing works great and it's great that it is backwards compatible. One small note, it is called every time the view appears, and although in this case the actions would be noops after the first call, in some cases we may still need to guard with a Bool property, to truly run specific appearance logic only once.

Zotero/Scenes/Master/MasterContainerViewController.swift Outdated Show resolved Hide resolved
@mvasilak mvasilak self-requested a review September 18, 2023 14:56
Copy link
Contributor

@mvasilak mvasilak left a comment

Choose a reason for hiding this comment

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

In MasterContainerViewController since the setup of the bottom container happens only for iPad form, which is the best optimization, now in iPhone form bottomYConstraint & bottomContainerBottomConstraint may be nil, so they should become optional as well.

@michalrentka
Copy link
Contributor Author

In MasterContainerViewController since the setup of the bottom container happens only for iPad form, which is the best optimization, now in iPhone form bottomYConstraint & bottomContainerBottomConstraint may be nil, so they should become optional as well.

Fixed in 6878aab, also there was another issue with keyboard apperance. When tag filter was in custom position and keyboard appeared, it just covered the filter if it was just too low. Fixed in 88f888f

@michalrentka michalrentka merged commit d878e2a into zotero:master Sep 20, 2023
1 check failed
@michalrentka michalrentka deleted the crashfix branch September 20, 2023 09:15
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.

2 participants