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

Allow some mobile options to be modified from defaults #1857

Merged
merged 14 commits into from Aug 18, 2022

Conversation

mattjohnsonpint
Copy link
Contributor

@mattjohnsonpint mattjohnsonpint commented Aug 17, 2022

On Android, iOS, and MacCatalyst (and Windows if using MAUI):

  • DetectStartupTime should default to Fast (not Best), but should be allowed to set to None
  • AutoSessionTracking should be true by default, but should be allowed to set to false
  • IsGlobalModeEnabled should always be true and not allowed to be changed.

Additionally:

  • Consolidated some conditional compilation flags
    • note: __IOS__ covers both iOS and MacCatalyst, __MOBILE__ covers those plus Android
  • Deprecated SentrySdk.Init methods that took an Android Context parameter, since we can get the application context directly.

Copy link
Member

@bruno-garcia bruno-garcia left a comment

Choose a reason for hiding this comment

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

awesome stuff

samples/Sentry.Samples.Maui/MainPage.xaml.cs Show resolved Hide resolved
src/Sentry/SentryOptions.cs Outdated Show resolved Hide resolved
src/Sentry/SentryOptions.cs Outdated Show resolved Hide resolved
mattjohnsonpint and others added 4 commits August 17, 2022 15:39
Co-authored-by: Bruno Garcia <bruno@brunogarcia.com>
Co-authored-by: Bruno Garcia <bruno@brunogarcia.com>
@SimonCropp
Copy link
Contributor

i like it. but did we not need to add/change any tests related to this?

@@ -179,7 +165,7 @@ private static void InitSentryAndroidSdk(SentryOptions options)
o.AddIgnoredExceptionForType(JavaClass.ForName("android.runtime.JavaProxyThrowable"));
}));

// Set options for the managed SDK that depend on the Android SDK
// Set options for the managed SDK that depend on the Android SDK. (The user will not be able to modify these.)
Copy link
Contributor

Choose a reason for hiding this comment

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

The user will not be able to modify these

does this mean "there is no API what the user can use to modify these" or "we ignore/overwrite the options the user sets"?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It means, we set these options ourselves during init, ignoring whatever the user provided. These should be limited to what is needed to make the native and managed SDKs work together.

@mattjohnsonpint
Copy link
Contributor Author

i like it. but did we not need to add/change any tests related to this?

Not until we have ability to write tests for ios/android

@mattjohnsonpint mattjohnsonpint merged commit 76b1344 into main Aug 18, 2022
@mattjohnsonpint mattjohnsonpint deleted the default-mobile-options branch August 18, 2022 00:19
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.

None yet

3 participants