search

Daltron / NotificationBanner

The easiest way to display highly customizable in app notification banners in iOS
MIT License
4.77k stars 660 forks source link

[GEN] Fix crash when bannerPositionFrame == nil #363

Closed niltsh closed 2 years ago

niltsh commented 2 years ago

We happened to root caused this issue when the notification was sent after the app became inactive.

We did some code to send the notification, and this part of code was triggered when app become inactive. This causes crash every time, the root cause is bannerPositionFrame is nil.

chickendiver commented 2 years ago

Any progress on this? Causing crashes for multiple users.

polis80cy commented 2 years ago

Getting close to merging...? This will really improve the experience. Thanks!

alexookah commented 2 years ago

@Daltron any chance merging this? we have lots of crashes about this

chickendiver commented 2 years ago

I implemented the changes locally and found that they actually caused the banner to not appear at all. I don't believe that this is a proper, functional fix.

If someone else could do the same and see what the effect is for them, that would help us get closer to a good solution (which might just be this PR).

abdulrehmanveemed commented 2 years ago

@chickendiver simply in BannerPositionFrame class Set this private(set) var startFrame: CGRect! private(set) var endFrame: CGRect! To this: private(set) var startFrame: CGRect.zero private(set) var endFrame: CGRect.zero

alexookah commented 2 years ago

@Daltron, @chickendiver Can you have a look on the proposed changes to fix the crashes?

chickendiver commented 2 years ago

The proposed changes from @abdulrehmanveemed are not syntactically correct Swift code, but the core idea of setting the start and end frame values to zero at initialization is captured here:

private(set) var startFrame: CGRect = CGRect.zero
private(set) var endFrame: CGRect = CGRect.zero

I have tested this change alongside the PR's changes, and can confirm that the banner displays on our iPad test devices, running iOS 15.1.

However, this test does not confirm whether the changes made actually fixed the crash. We'll monitor for crashes once these changes have been deployed.

alexookah commented 2 years ago

@chickendiver any news?

chickendiver commented 2 years ago

No new crashes reported in production, so this seems to have worked, but I would want more data from other sources before being able to definitively say that this has solved the problem.

alexookah commented 2 years ago

@chickendiver can you make a PR with the changes you tried in production so that other's can also try it ?

tomerpetel commented 2 years ago

Any workaround we can use meantime? Any progress with PR?

abdulrehman73331 commented 2 years ago

Any workaround we can use meantime? Any progress with PR?

check my comment!

alexookah commented 2 years ago

Created another PL with the proposed changes from @abdulrehman73331. Hope it gets merged soon.

sayler8182 commented 2 years ago

@Daltron could You accept this PR and create new release package ?

Daltron commented 2 years ago

Sorry for the delay everyone! Thanks to all for chiming in on this!