Refactor code to better align with Unreal's structure

[nwhite-riot]

Addresses #733 and #754.

The intent of this change is to better align the structure/layout of code with Unreal by separating out platform-specific code in a more meaningful way.

• Split SentrySubsystemDesktop into GenericPlatform, Microsoft, Windows, and Linux
  • Better encapsulation for platform-specific functionality
  • Prepares for adding Xbox support (since it'll inherit from Microsoft and not Windows)
• Use typedef overrides for multiplatform implementations (e.g. Sentry subsystem)
• Add HAL to house platform-specific includes, overrides, and macros
• Where possible, retain existing functionality
  • Unfortunately, even though a number of changes where file moves and renames, Git doesn't seem to recognize and instead shows deletes and create
• Standardized Unreal object creation and native object retrieval into templatized base (with TSentryImplWrapper<>)

[j0tt]

I still want to better understand the macro magic here, but I'll leave that for tomorrow. This is a first pass review :) I don't feel strongly about ending a file with a newline if you're not keen on the busy work, it's just a habit of mine 😅

[tustanivsky]

@nwhite-riot Thank you for all the effort you've put into this refactoring so far! Overall, things are looking good and we appreciate making the SDK more future-proof in terms of adding consoles support. This review might require a few iterations given the amount of changes however I'd like to share some of my early thoughts:

One of my major concerns is that Sentry entities (e.g. event, user, breadcrumb) cannot be created directly with NewObject in client code as they now have to be instantiated using the TSentryImplWrapper::Create static method which might not be as intuitive. This also requires a pointer to the object's native implementation to properly initialize its Unreal wrapper however the necessary interfaces are currently inaccessible outside the plugin. So the only way to create a valid Sentry object at this point is through USentryLibrary.

Previously we relied at constructors and/or initialize methods to ensure that the default native impl is set whenever possible making Unreal objects self-contained. This approach wasn't ideal but it seemed more error-proof for SDK users. Perhaps it would make sense to combine this with the object construction utilities introduced here?

If so, there doesn't seem to be any other significant changes to the plugin's public API and therefore preserving its backward compatibility with older versions would be preferred.

[tustanivsky]

One thing that worries me a little here is test coverage - it appears test coverage wasn't 100% before the refactor, and now it'll be even lower. @tustanivsky, what's the preferred approach for adding/managing tests?

After adding the CreateSharedSentry___ methods to HAL it should be possible to maintain the same level of test coverage as before, right? Our current CI pipeline is limited to running checks on Linux but we're actively working on expanding it to support more targer platforms.

[nwhite-riot]

So the only way to create a valid Sentry object at this point is through USentryLibrary

@tustanivsky, I thought this would be preferable to having users instantiate their own objects - either we have the library to do it for them, or we remove the library and allow them to construct the objects manually. Feels odd to have both?

[tustanivsky]

I thought this would be preferable to having users instantiate their own objects - either we have the library to do it for them, or we remove the library and allow them to construct the objects manually. Feels odd to have both?

USentryLibrary was originally designed as a convenience utility for users working with the plugin's API in Blueprints. At the same time, manually constructing objects with NewObject is a common approach that provides additional flexibility for creating various Sentry instances in C++. I believe keeping both options available will help developers avoid potential issues when switching to this new SDK version.

[nwhite-riot]

I believe keeping both options available will help developers avoid potential issues when switching to this new SDK version.

My last question about this is around object validity: if a user calls NewObject<...>() but then does not initialize with the native implementation, the object would be invalid and all methods would no-op (without any kind of logging). It feels to me like this is an anti-pattern that will end up causing more problems.

I will look into retaining the desired functionality (of allowing NewObject<...>()).

Thanks for the discussion!

[tustanivsky]

if a user calls NewObject<...>() but then does not initialize with the native implementation, the object would be invalid and all methods would no-op (without any kind of logging). It feels to me like this is an anti-pattern that will end up causing more problems.

For most Sentry classes that wasn't an issue since their default constructor handled the native implementation initialization (except USentryAttachment and maybe few others where that's not possible). Will it make sense to add extra logging during the NativeImpl validation?

[tustanivsky]

I've created a PR (nwhite-riot#3) that addresses some of the build issues on Apple/Android platforms.

[tustanivsky]

Thank you @nwhite-riot!