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

Support detached Fragments by removing references to the Activity #613

Merged
merged 4 commits into from
Aug 22, 2024

Conversation

jimmithy
Copy link
Contributor

@jimmithy jimmithy commented Apr 4, 2024

🎯 Goal

Previously if we create a fragment that is not yet attached to the activity and reference the balloon, this will throw an IllegalStateException. This is because the lazy loader requires an Activity even thought the implementation only requires Context.

This change removes all references to activities, so the balloon can be successfully attached to the anchor without needing the activity context.

🛠 Implementation details

  1. Update Lazy helpers to use requireContext instead of requireActivity
  2. Remove unused getActivity method
  3. Remove unneccesary Context.isFinishing method because isAttachedToWindow should resolve issue android.view.WindowManager$BadTokenException: Unable to add window -- token null is not valid; is your activity running? #92

Code reviews

All submissions, including submissions by project members, require review. We use GitHub pull requests for this purpose. Consult GitHub Help for more information on using pull requests.

Context != Activity

This should resolve issues where the fragment is not attached to an activity. We already check that the context is not null earlier in the method
An alternative approach to skydoves#92 to avoid any reference to the activity

isAttachedToWindow will confirm that the anchor view has a valid window token.
@jimmithy jimmithy requested a review from skydoves as a code owner April 4, 2024 16:02
@skydoves
Copy link
Owner

skydoves commented Apr 16, 2024

Hey @jimmithy, checking out the isFinishing is needed for working correctly with Activity. I'm wondering if you can resolve this issue by calling the show method inside the If statements, which has isAttachedToWindow as a condition.

Switch from a chained condition to multiple conditions that early exit.

This should ensure the context doesn't have to be an activity.
@jimmithy
Copy link
Contributor Author

Hey @jimmithy, checking out the isFinishing is needed for working correctly with Activity. I'm wondering if you can resolve this issue by calling the show method inside the If statements, which has isAttachedToWindow as a condition.

Hey, I wasn't entirely sure what you were suggesting, but I've restored the isFinishing check in a way that makes the context implementation not necessary.

Copy link
Owner

@skydoves skydoves left a comment

Choose a reason for hiding this comment

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

Sorry for the very late response. Overall looks good to me. Thank you so much for your contribution 👍

@skydoves skydoves merged commit ae18002 into skydoves:main Aug 22, 2024
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.

2 participants