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 connectivity android #568

Open
wants to merge 4 commits into
base: project/PH1
Choose a base branch
from
Open

Conversation

sleushunou
Copy link
Contributor

Description

Issues Resolved

  • fixes #

API Changes

None

Platforms Affected

  • Core (all platforms)
  • iOS
  • Android

Behavioral/Visual Changes

None

Before/After Screenshots

Not applicable

PR Checklist

  • I have read the CONTRIBUTING document
  • My code follows the code styles
  • Targets the correct branch
  • Tests are passing (or failures are unrelated)

@github-actions github-actions bot added c/common Related to Common component. c/connectivity Related to Connectivity component. c/push-notifications Related to Push Notifications component. p/Android Related to Android platform. labels Nov 19, 2024
/// <param name="disposing"><c>true</c> to dispose managed state.</param>
/// <seealso cref="Dispose"/>
/// <seealso cref="T:System.IDisposable"/>
protected virtual void Dispose(bool disposing)
Copy link
Member

Choose a reason for hiding this comment

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

Please add additional public method:

public void Dispose()
{
    Dispose(true);

    GC.SuppressFinalize(this);
}

to be able to remove observer from outside

{
Connectivity.ConnectivityChanged -= CurrentConnectivityChanged;
Connectivity.ConnectivityChanged += CurrentConnectivityChanged;
CurrentConnectivityChanged(this, new ConnectivityChangedEventArgs(Connectivity.NetworkAccess, Connectivity.ConnectionProfiles));
Copy link
Member

Choose a reason for hiding this comment

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

Is it really necessary to manually trigger an event only on Start at this place?

/// </param>
public DroidConnectivityService(IConnectivity connectivity) : base(connectivity)
{
_startAction = new WeakAction(OnAppStart);
Copy link
Member

Choose a reason for hiding this comment

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

Do you need Lifecycle.Event.ON_CREATE or ON_RESUME also?

@@ -18,4 +18,8 @@
<ProjectReference Include="..\Softeq.XToolkit.Common\Softeq.XToolkit.Common.csproj" />
</ItemGroup>

<ItemGroup>
<PackageReference Include="Xamarin.AndroidX.Lifecycle.Process" Version="2.6.2.1" />
Copy link
Member

@wcoder wcoder Nov 20, 2024

Choose a reason for hiding this comment

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

The main reason for Common projects is 'no 3rd-party dependencies'

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c/common Related to Common component. c/connectivity Related to Connectivity component. c/push-notifications Related to Push Notifications component. p/Android Related to Android platform.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants