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

refactor: refactor navcontroller tracking plugin teardown #70

Conversation

vgupta98
Copy link
Contributor

@vgupta98 vgupta98 commented Nov 26, 2024

Description

This PR refactors the teardown functionality for NavControllerTrackingPlugin so that the activity observers are also removed when teardown happens.

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Documentation update
  • Code refactor/optimization

Implementation Details

  • First, the updateNavContexts call in the subscriber of navContextState is wrapped in withContext(NonCancellable). It ensures that it is called even when the coroutine is cancelled during the time of shutdown. We need this when we remove all the contexts during teardown.
  • The responsibility of adding and removing activity observer is now with NavControllerTrackingPlugin. NavControllerActivityObserver exposes methods to add and remove observers which are being used in the plugin to add and remove the observers and navContextState subscriber is called.
  • So, whenever teardown is called, RemoveAllNavContextsAction is dispatched, which basically removes all the navContexts from navContextState. It then calls the subscriber which will clear currentNavContexts and activityObservers and remove the OnDestinationChangedListener and activity lifecycle observers.
  • The addObserver and removeObserver calls in NavControllerActivityObserver use runOnMainThread to ensure that adding and removing of observers happen on main thread.

Checklist

  • I have added tests that prove my fix is effective or that my feature works.
  • I have added the necessary documentation (if appropriate).
  • I have ensured that my code follows the project's code style.
  • I have checked for potential performance impacts and optimized if necessary.
  • I have checked the code for security issues.
  • I have updated the changelog (if required).

How to test?

Apply this patch and test the changes on fragment and compose sample apps.

Breaking Changes

Maintainers Checklist

  • The code has been reviewed.
  • CI tests have passed.
  • All necessary documentation has been updated.

Screenshots (if applicable)

Additional Context

Vishal Gupta added 27 commits November 15, 2024 10:17
the analyticsJob needs to be declared as a global private variable since it is used inside the secondary contructor
Copy link

linear bot commented Nov 26, 2024

SDK-2674 [Android] Make NavControllerTrackingPlugin teardown call complete

Description

Currently, the teardown of NavControllerTrackingPlugin only removes the stored navContexts. We also need to remove the attached NavControllerActivityObserver on teardown.

Tech design / Research

Acceptance Criteria

Checklist:

  • Implementation of the solution
  • Unit tests added
  • End to End cases added
    • Sample application changes
  • Meets acceptance criteria
  • Test on different OS versions
  • Developer tested feature
  • Documentation
    • Process with DOCS team initiated
    • Documentation reviewed and approved
  • PR opened
    • Code Review approved
  • Product owner approval
  • TAMs informed about changes
  • Feature merged
    • Develop
    • Main

@vgupta98 vgupta98 marked this pull request as ready for review November 26, 2024 10:32
Base automatically changed from feat/sdk-2599-add-shutdown to develop.poc November 26, 2024 16:13
Comment on lines +89 to +94
val observerToBeAdded = provideNavControllerActivityObserver(
plugin = this,
navContext = navContext
)
observerToBeAdded.addObserver()
activityObservers.add(observerToBeAdded)
Copy link
Contributor

Choose a reason for hiding this comment

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

You may consider using the scope function.

@vgupta98 vgupta98 merged commit 70222f8 into develop.poc Nov 28, 2024
2 checks passed
@vgupta98 vgupta98 deleted the refactor/sdk-2674-refactor-navcontroller-tracking-plugin-teardown branch November 28, 2024 08:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants