search

bitmovin / bitmovin-player-android-analytics-conviva

Integration of the Bitmovin Android Player SDK with the Conviva Analytics
MIT License
1 stars 4 forks source link

Features/update conviva to v4.x #38

Closed sinewave440hz closed 2 years ago

sinewave440hz commented 2 years ago

The changes in summary for the users of this sdk are:

  1. The ConvivaAnalytics class is now called ConvivaAnalyticsIntegration

  2. The third constructor ConvivaAnalytics(Player player, String customerKey, Context context, ConvivaConfig config, Client client) is now removed because the Client class is deprecated

    Other than that, the interface remains the same for now.

dweinber commented 2 years ago

Please make sure to add a CHANGLOG.md entry, and I'd assume we need to update the build.gradle as well? https://github.com/bitmovin/bitmovin-player-android-analytics-conviva/blob/2e8e5b39a5aab839fbca791addd80e0f0e51b30c/conviva/build.gradle#L40

sinewave440hz commented 2 years ago

Please make sure to add a CHANGLOG.md entry, and I'd assume we need to update the build.gradle as well?

https://github.com/bitmovin/bitmovin-player-android-analytics-conviva/blob/2e8e5b39a5aab839fbca791addd80e0f0e51b30c/conviva/build.gradle#L40

Yes, that would be good. The legacy SDKs were removed in 4.0.18. I'll test out the latest - v4.0.20.

sinewave440hz commented 2 years ago

Please make sure to add a CHANGLOG.md entry, and I'd assume we need to update the build.gradle as well? https://github.com/bitmovin/bitmovin-player-android-analytics-conviva/blob/2e8e5b39a5aab839fbca791addd80e0f0e51b30c/conviva/build.gradle#L40

Yes, that would be good. The legacy SDKs were removed in 4.0.18. I'll test out the latest - v4.0.20.

This does, however, present us with a problem. The bitmovin integration's interface exposes objects from the legacy SDKs - the Client object. It's a simple enough change, but will change the interface for existing customers right away. This was always going to happen of course, but I rather expected there to be a transition phase. Should we dive in right away with that change?

(The alternative being to stay lower than 4.0.18 for that transition phase...)

sinewave440hz commented 2 years ago

Please make sure to add a CHANGLOG.md entry, and I'd assume we need to update the build.gradle as well? https://github.com/bitmovin/bitmovin-player-android-analytics-conviva/blob/2e8e5b39a5aab839fbca791addd80e0f0e51b30c/conviva/build.gradle#L40

Yes, that would be good. The legacy SDKs were removed in 4.0.18. I'll test out the latest - v4.0.20.

This does, however, present us with a problem. The bitmovin integration's interface exposes objects from the legacy SDKs - the Client object. It's a simple enough change, but will change the interface for existing customers right away. This was always going to happen of course, but I rather expected there to be a transition phase. Should we dive in right away with that change?

(The alternative being to stay lower than 4.0.18 for that transition phase...)

A bit more about this - upgrading to 4.0.17.197 (immediately prior to the removal of the old SDK stuff) results in the Client class being marked as deprecated. So if there are any customers that are using the full interface including the Client parameter, perhaps that would be the best user experience for them - that their current interface works but is marked deprecated, and the fix is simply to remove the Client parameter.

dweinber commented 2 years ago

Let's upgrade anyway. As you said, we'll need to do this at some point anyway. We'll manage Comms to clients about this change

sinewave440hz commented 2 years ago

Let's upgrade anyway. As you said, we'll need to do this at some point anyway. We'll manage Comms to clients about this change

Cool, done that. In that case, perhaps we should go ahead and make a change that I was also holding off on and that we discussed earlier - ie. changing the name of the Bitmovin ConvivaAnalytics class to ConvivaAnalyticsIntegration, so that we can use the unqualified name ConvivaAnalytics for Conviva without clashes? :)

dweinber commented 2 years ago

Agreed, let's do that. Let's break multiple things at once instead of breaking one thing in every release 😉

sinewave440hz commented 2 years ago

All of the above discussions are now resolved so I'm requesting review on this once again :)

luckygoyal-bitmovin commented 2 years ago
luckygoyal-bitmovin commented 2 years ago

@sinewave440hz , thanks for addressing review comments. The change look good to me now. IMO, the only pending work in updating the test cases to work again. Do you intend to work on test cases now or later?

sinewave440hz commented 2 years ago

@sinewave440hz , thanks for addressing review comments. The change look good to me now. IMO, the only pending work in updating the test cases to work again. Do you intend to work on test cases now or later?

I had started to look at these actually. However, I did notice that they don't seem to be passing now in master either. This may mean that it will take longer than my original half day estimate, I'm afraid, as I don't appear to have working code to refer to. Unless perhaps you can see what is wrong, or indeed confirm that you are seeing the same failures.