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

EUID Support #98

Merged
merged 8 commits into from
Sep 10, 2024
Merged

EUID Support #98

merged 8 commits into from
Sep 10, 2024

Conversation

dcaunt
Copy link
Contributor

@dcaunt dcaunt commented Aug 14, 2024

Implements EUID Support via a new EUIDManager type which vends a UID2Manager configured for EUID.

There is a new AndroidManifest meta-data key uid2_environment_euid for toggling the EUID environment in the dev app.

Reference iOS PR: IABTechLab/uid2-ios-sdk#57

  • Add EUIDManager
  • Add toggles to dev app to change environment from UID2 to EUID

EUID support in GMA and IMA adapters to follow in separate PRs.

dev-app/src/main/java/com/uid2/dev/MainActivity.kt Outdated Show resolved Hide resolved
sdk/src/main/java/com/uid2/UID2Manager.kt Outdated Show resolved Hide resolved
AdError(
manager.currentIdentityStatus.value,
"No Advertising Token",
"UID2",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not clear to me yet if this should still be UID2, or EUID.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't know how we get these AdError's reported, so i'm not sure. Worth asking the team though.

sdk/src/main/java/com/uid2/EUIDManager.kt Outdated Show resolved Hide resolved
sdk/src/main/java/com/uid2/EUIDManager.kt Outdated Show resolved Hide resolved
sdk/src/main/java/com/uid2/EUIDManager.kt Outdated Show resolved Hide resolved
sdk/src/main/java/com/uid2/UID2Manager.kt Outdated Show resolved Hide resolved
sdk/src/main/java/com/uid2/UID2Manager.kt Outdated Show resolved Hide resolved
sdk/src/main/java/com/uid2/EUIDManager.kt Outdated Show resolved Hide resolved
AdError(
manager.currentIdentityStatus.value,
"No Advertising Token",
"UID2",
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't know how we get these AdError's reported, so i'm not sure. Worth asking the team though.

@dcaunt dcaunt force-pushed the dave/euid branch 2 times, most recently from 8f4ba81 to 29c2ee4 Compare August 20, 2024 16:50
@dcaunt dcaunt marked this pull request as ready for review August 20, 2024 17:05
import com.uid2.utils.TimeUtils
import kotlinx.coroutines.Dispatchers

public class EUIDManager {

Choose a reason for hiding this comment

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

The existence of this different class that's just a factory for a slightly different UID2Manager seems pretty weird. From my lack of knowledge, what is exactly changing here from the normal UIDManager.getInstance ?

We could either go the factory function route, the extension route, the initializing lambda route, etc. But to be able to give a helpful suggestion I'd need to know what is changing exactly here.

FWIW The java-ish UID2Manager shape is not exactly helpful either here XD

Choose a reason for hiding this comment

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

Another alternative is, if possible, working toward this being an object instead of a class because you could pretty much instantiate an EUIDManager class here that'd be kind of useless (and misleading).

Copy link
Collaborator

Choose a reason for hiding this comment

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

+1

Copy link
Collaborator

Choose a reason for hiding this comment

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

The challenge here is trying to keep backwards compatability with existing integrators.

Another alternative is, if possible, working toward this being an object instead of a class because you could pretty much instantiate an EUIDManager class here that'd be kind of useless (and misleading).

Dave and I have spoken about this previously, and would both love to move away from a singleton. Our current thinking is to consider this when moving to 2.0.0 of the SDK, so later - as it impacts a lot of other things (e.g. all the documentation already created for publishers).

I do agree we should consider what's easiest for publishers to consume. Since a lot will support an app that supports both UID2 and EUID, the existing interface means they will be required to do this constantly:

if (isUID2) { UID2Manager.getInstant() } else { EUID.getInstance() }

The alternative would be to just change the UID2Manager.init method to allow UID2 vs EUID configuration (and environment) and everyone access directly via UID2Manager.

sdk/src/main/java/com/uid2/EUIDManager.kt Outdated Show resolved Hide resolved
import com.uid2.utils.TimeUtils
import kotlinx.coroutines.Dispatchers

public class EUIDManager {
Copy link
Collaborator

Choose a reason for hiding this comment

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

+1

sdk/src/main/java/com/uid2/UID2Manager.kt Outdated Show resolved Hide resolved
dev-app/src/main/java/com/uid2/dev/MainActivity.kt Outdated Show resolved Hide resolved
dev-app/src/main/java/com/uid2/dev/MainActivity.kt Outdated Show resolved Hide resolved
dev-app/src/main/java/com/uid2/dev/DevApplication.kt Outdated Show resolved Hide resolved
sdk/src/main/java/com/uid2/EUIDManager.kt Outdated Show resolved Hide resolved
sdk/src/main/java/com/uid2/EUIDManager.kt Outdated Show resolved Hide resolved
dev-app/src/main/java/com/uid2/dev/DevApplication.kt Outdated Show resolved Hide resolved
dev-app/src/main/java/com/uid2/dev/DevApplication.kt Outdated Show resolved Hide resolved
@@ -51,11 +51,15 @@ sealed interface MainScreenState : ViewState {
class MainScreenViewModel(
private val api: AppUID2Client,
private val manager: UID2Manager,
isEUID: Boolean,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nit: Since this is dev code, I don't feel strongly at all, but just to call out that for the vast majority of users, the default will be UID2. Therefore it would make sense to have this inverted where true is the default, and therefore isUID2.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I reverted the manifest back to false as that was my intention. Is that what you meant? EUID would then only be used if the manifest value is set to true.

import com.uid2.utils.TimeUtils
import kotlinx.coroutines.Dispatchers

public class EUIDManager {
Copy link
Collaborator

Choose a reason for hiding this comment

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

The challenge here is trying to keep backwards compatability with existing integrators.

Another alternative is, if possible, working toward this being an object instead of a class because you could pretty much instantiate an EUIDManager class here that'd be kind of useless (and misleading).

Dave and I have spoken about this previously, and would both love to move away from a singleton. Our current thinking is to consider this when moving to 2.0.0 of the SDK, so later - as it impacts a lot of other things (e.g. all the documentation already created for publishers).

I do agree we should consider what's easiest for publishers to consume. Since a lot will support an app that supports both UID2 and EUID, the existing interface means they will be required to do this constantly:

if (isUID2) { UID2Manager.getInstant() } else { EUID.getInstance() }

The alternative would be to just change the UID2Manager.init method to allow UID2 vs EUID configuration (and environment) and everyone access directly via UID2Manager.

@dcaunt dcaunt force-pushed the dave/euid branch 2 times, most recently from 7c10857 to 73ccb66 Compare September 9, 2024 14:22
@dcaunt dcaunt merged commit c786b25 into main Sep 10, 2024
2 checks passed
@dcaunt dcaunt deleted the dave/euid branch September 10, 2024 10:18
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.

4 participants