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

Adapter for Maio SDK v2.0.0 #529

Merged
merged 5 commits into from
Jan 6, 2025

Conversation

7pairs
Copy link
Contributor

@7pairs 7pairs commented Aug 29, 2024

Update Maio adapter to call Maio SDK v2.0.0.

@LTPhantom
Copy link
Collaborator

Hello @7pairs thanks for the pull request. There was an update to bring unit tests to github and I was wondering if you could update your changes to get those tests running. Thanks in advance!

Copy link
Collaborator

@LTPhantom LTPhantom left a comment

Choose a reason for hiding this comment

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

Gentle bump for last comment, also making it as a request change.
To continue with the review, can this PR be sync to the latest version? Thanks!

@7pairs
Copy link
Contributor Author

7pairs commented Oct 23, 2024

@LTPhantom
Thank you for your feedback.
We are currently working on updating our SDK.
Once the updated SDK is released, I will update the pull request to address the points you have raised and reference the corrected version of the SDK.
We kindly ask for a bit more time to complete this process.

@7pairs
Copy link
Contributor Author

7pairs commented Nov 6, 2024

@LTPhantom
After reviewing the conflict, I noticed that the tests added in September are based on our SDK v1 structure, making them incompatible with v2.

Unlike v1, v2 does not include classes like MaioAds or MaioAdsInstance to manage the entire system, nor does it require an initialization process. Consequently, most methods in MaioAdsManager are now obsolete and have been removed in this pull request, along with MaioAdsManagerListener.

In v2, to display an ad, developers only need to load an ad with static methods provided by Rewarded or Interstitial, and then display the ad using show(), as shown below:

Rewarded r = Rewarded.load("zoneID");
r.show();

Could you advise on the approach we should take to reconcile the implementation differences between v1 and v2?

Going forward, we plan on discontinuing V1 and making all subsequent versions of our admob SDK adhere to V2 only (which is not backwards compatible with V1). Apart from fixing the tests, are there any other potential issues we need to fix to make the adapter only for V2 going forward?

@LTPhantom
Copy link
Collaborator

LTPhantom commented Nov 21, 2024

@7pairs
Thanks for the update. I think it should be ok to remove all v1 implementation if we are also making a major release of the adapter. The only stuff we would love to have are new tests for the new methods used by v2, it is completely ok to remove unnecessary tests. I will review the changes and comment accordingly. Thanks again.

@7pairs
Copy link
Contributor Author

7pairs commented Nov 22, 2024

@LTPhantom
Thank you for your response.
We will remove tests for v1 and add replacement tests.
Please give us some time.

@7pairs
Copy link
Contributor Author

7pairs commented Dec 20, 2024

@LTPhantom
To the extent possible, we have adapted the tests to our SDK v2.
As I mentioned before, we had to remove MaioAdsManagerListener in v2, so we could not write tests for some events.

@7pairs 7pairs requested a review from LTPhantom December 22, 2024 23:02
Copy link
Collaborator

@LTPhantom LTPhantom left a comment

Choose a reason for hiding this comment

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

Thanks for cleaning up the tests. General code is looking good, we can look forward to submit after the last comments are addressed.

@@ -10,7 +10,7 @@ ext {
// String property to store the proper name of the mediation network adapter.
adapterName = "maio"
// String property to store version name.
stringVersion = "1.1.16.3"
stringVersion = "2.0.0.0"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you also update the versionCode below to 2000000

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 have fixed it, as you suggested.
In addition, the SDK version has been updated to 2.0.3.

}
}
this.maioRewarded = Rewarded.loadAd(new MaioRequest(zoneID,
mediationRewardedAdConfiguration.isTestRequest(), ""), context, new IRewardedLoadCallback() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Since the third parameter of the MaioRequest constructor is an empty string, can you add the name of the parameter in the constructor call to give more clarity? Something like ClassName(/*paramName=/* "")

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 have fixed it, as you suggested.

@@ -35,18 +35,24 @@
import java.lang.annotation.RetentionPolicy;
import java.util.HashSet;
import java.util.List;
import jp.maio.sdk.android.FailNotificationReason;
import jp.maio.sdk.android.MaioAds;

Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: Remove empty line

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 have fixed it, as you suggested.

@@ -21,23 +21,28 @@
import android.util.Log;
import androidx.annotation.NonNull;
import androidx.annotation.Nullable;
import com.google.ads.mediation.maio.MaioAdsManagerListener;

Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: Remove empty line. Same on line 30.

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 have fixed it, as you suggested.

}
});
this.maioInterstitial = jp.maio.sdk.android.v2.interstitial.Interstitial.loadAd(
new MaioRequest(zoneID, mediationAdRequest.isTesting(), ""), context, new IInterstitialLoadCallback() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same as in the MaioMediationAdapter comment, can you add the name of the parameter?

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 have fixed it, as you suggested.

Comment on lines 1 to 14
// Copyright 2019 Google LLC
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Please keep the License header.

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 have fixed it, as you suggested.

@7pairs 7pairs requested a review from LTPhantom December 27, 2024 08:57
@LTPhantom
Copy link
Collaborator

Unit tests seem to be failing, it seems related to MaioMediationAdapter.java, line 97. Are you able to see the details?

@7pairs
Copy link
Contributor Author

7pairs commented Dec 29, 2024

@LTPhantom
Sorry, the ErrorCode implementation had changed since v2.0.3.
The adapter was fixed accordingly.

@copybara-service copybara-service bot merged commit f6df35f into googleads:main Jan 6, 2025
2 of 3 checks passed
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.

2 participants