-
Notifications
You must be signed in to change notification settings - Fork 0
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
EUID Support #98
Conversation
AdError( | ||
manager.currentIdentityStatus.value, | ||
"No Advertising Token", | ||
"UID2", |
There was a problem hiding this comment.
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
.
There was a problem hiding this comment.
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.
AdError( | ||
manager.currentIdentityStatus.value, | ||
"No Advertising Token", | ||
"UID2", |
There was a problem hiding this comment.
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.
securesignals-gma/src/main/java/com/uid2/securesignals/gma/EUIDMediationAdapter.kt
Outdated
Show resolved
Hide resolved
8f4ba81
to
29c2ee4
Compare
import com.uid2.utils.TimeUtils | ||
import kotlinx.coroutines.Dispatchers | ||
|
||
public class EUIDManager { |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1
There was a problem hiding this comment.
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.
import com.uid2.utils.TimeUtils | ||
import kotlinx.coroutines.Dispatchers | ||
|
||
public class EUIDManager { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1
securesignals-gma-dev-app/src/main/java/com/uid2/dev/BannerActivity.java
Outdated
Show resolved
Hide resolved
securesignals-gma-dev-app/src/main/java/com/uid2/dev/GMADevApplication.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, |
There was a problem hiding this comment.
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
.
There was a problem hiding this comment.
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 { |
There was a problem hiding this comment.
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.
7c10857
to
73ccb66
Compare
Implements EUID Support via a new
EUIDManager
type which vends aUID2Manager
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
EUID support in GMA and IMA adapters to follow in separate PRs.