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

Fix/app crashing because of wrong vc hierarchy #24

Open
wants to merge 23 commits into
base: main
Choose a base branch
from

Conversation

YahiaRagae
Copy link

@YahiaRagae YahiaRagae commented Jul 9, 2024

Fix: App Crashing Due to Wrong View Controller Hierarchy

Changes

  • Enhancements:

    • Added UIApplication+ CurrentNonAdViewController category for getting currentNonAdViewController .
    • Updated BrightcoveIMAPlayerView to use the new category.
    • Added TestHost to host unit tests requiring UIApplication.
  • Cleanup and Reorganization:

    • Removed unused adDisplayView and adResumeButton.
    • Removed fullScreenCloseBtn and fullscreen logic.
    • Removed the logic for forcing fullscreen mode for the Ad.
    • Reorganized BrightcoveIMAPlayerView for readability.
    • Removed commented-out code.
  • Updates:

    • Bumped library version to 2.3.6.
    • Updated react-native from 0.63.4 to 0.68.0.
    • Added bob to simplify building the React Native library.
    • Added patch-package to patch iOS codebase issues.
    • Updated dependencies to match the latest feature branch.
  • Exclusions:

    • Excluded BrightcoveImaPlayerTest.
    • Excluded tests from the podspec.

manojalwisnz and others added 22 commits October 18, 2023 16:53
…ctly

Nat 28585 ad timer not showing correctly
…rade to support GMA SDK version 10.10.0, also raised the miminum deployment target to ios 12.4.
…have to drop GMA SDK version to 22.4.0 as well (from 22.6.0)
- Update    `react-native` from `0.63.4` -> to `0.68.0`
- Add `bob` to simplify the process of building your React Native library for both JavaScript and native code
- Add `patch-package` to patch dome issues at the iOS code base
- remove the logic for fullscreen
- Add Logic to search for the `currentViewController`
- Fix full screen VC
…g used anymore

- remove commented out code
…the `currentViewController`

- Update `BrightcoveIMAPlayerView` to use the new `UIApplication+CurrentViewController`
- Add Unit Tests
-  Remove the case of `UISplitViewController` for now since it needs more time to test
Copy link

@jazdance jazdance left a comment

Choose a reason for hiding this comment

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

It's very thoughtful you also merged the previous PR with upgraded version of GMA SDK, into this PR already.

@jazdance jazdance self-requested a review October 2, 2024 04:00
Copy link

@jazdance jazdance left a comment

Choose a reason for hiding this comment

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

Looks like more code changes are required to cater for the scenario below:

on iphone,
if an article contains multiple videos,
right after start playing video A whose pre-roll video ad is playing...
scroll down to video B on the same article, click & start playing video B, the app will crash.

@YahiaRagae
Copy link
Author

  • The initial fix was searching for the top View Controller and attaching the AdView to it and it was running for an article with 1 Video

  • When there are 2 videos, first video will get added correctly, but for the second video it will try to be added to the top ViewController which is the first AdView , that’s why the app was crashing.

  • I have updated my fix to search for the first Non-AdView View Controller, and it’s working now.

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

Successfully merging this pull request may close these issues.

4 participants