Add new callback interfaces split from DeployGateCallback #95

satsukies · 2024-08-20T00:41:59Z

We will plan to implement a new callback method for the Capture feature.
Expanding the current callback interface is difficult because we cannot avoid causing breaking changes for many users.
We will maybe have the same problem in the future when planning for new features with callbacks.

So, we separate the DeployGateCallback interface into multiple interfaces.
When new interfaces are added in the future, it will be possible to provide them without affecting users who do not need them.

We take care in making changes to avoid breaking changes.

….Builder

…dkConfigration in each module

satsukies · 2024-08-20T00:48:51Z

sample/src/main/java/com/deploygate/sample/App.java

+            .setEnabledOnNonDebuggableBuild(true)
+            .build();
+
+        DeployGate.install(this, configuration);


use install(Context, DeployGateSdkConfiguration) because install(Application, DeployGateCallback, boolean) is deprecated.

satsukies · 2024-08-20T00:57:29Z

sdk/src/main/java/com/deploygate/sdk/DeployGateCallback.java

 */
-public interface DeployGateCallback {
+@Deprecated
+public interface DeployGateCallback extends DeployGateInitializeCallback, DeployGateStatusChangeCallback, DeployGateUpdateAvailableCallback {


Separate to DeployGateInitializeCallback, DeployGateStatusChangeCallback, DeployGateUpdateAvailableCallback.

Now, we marked it deprecated.
We can still use this interface, but we plan to delete it in the next major update.

satsukies · 2024-08-20T01:11:21Z

sdk/src/main/java/com/deploygate/sdk/DeployGate.java

    private final HashSet<DeployGateCallback> mCallbacks;
+    private final HashSet<DeployGateInitializeCallback> mInitializeCallbacks;
+    private final HashSet<DeployGateStatusChangeCallback> mStatusChangeCallbacks;
+    private final HashSet<DeployGateUpdateAvailableCallback> mUpdateAvailableCallbacks;


I chose the implementation to keep mCallbacks to minimize the effect on existing code.
It will be deleted with DeployGateCallback.

No, it means minimizing changes to the current code but not minimizing the effect.

The current changes mean we are going to keep maintaining two callback mechanisms until we can completely delete the deprecated entities, even tough the new mechanism is compatible with the legacy one.

The current changes allows at most two callbacks like DeployGateInitializeCallback at the initialization time. It's not a planned feature.

Please remove mCallbacks property and dispatch the callback registration to each property. It will be completely the same behaviour as the current behaviour i.e. it can minimize the effect.

I focused on minimizing code changes.
I understood it is important to minimize behavior changes same behavior as the current behavior in this case.

I'll fix it.

satsukies · 2024-08-20T01:14:44Z

sdk/src/main/java/com/deploygate/sdk/DeployGate.java

+                    // call onInitialized on each callbacks
                    for (DeployGateCallback callback : mCallbacks) {
                        callback.onInitialized(true);
+                    }
+                    for (DeployGateInitializeCallback callback : mInitializeCallbacks) {
+                        callback.onInitialized(true);
+                    }
+
+                    // after call onInitialized, then call onStatusChanged
+                    for (DeployGateCallback callback : mCallbacks) {
+                        callback.onStatusChanged(isManaged, isAuthorized, loginUsername, isStopped);
+                    }
+                    for (DeployGateStatusChangeCallback callback : mStatusChangeCallbacks) {
                        callback.onStatusChanged(isManaged, isAuthorized, loginUsername, isStopped);
                    }


Keep order of call callback methods.
We call onInitializad() first, after that call onStatusCahnged().

satsukies · 2024-08-20T01:15:01Z

sdk/src/main/java/com/deploygate/sdk/DeployGate.java

+                // call onInitialized on each callbacks
                for (DeployGateCallback callback : mCallbacks) {
                    callback.onInitialized(false);
+                }
+                for (DeployGateInitializeCallback callback : mInitializeCallbacks) {
+                    callback.onInitialized(false);
+                }
+
+                // after call onInitialized, then call onStatusChanged
+                for (DeployGateCallback callback : mCallbacks) {
+                    callback.onStatusChanged(false, false, null, false);
+                }
+                for (DeployGateStatusChangeCallback callback : mStatusChangeCallbacks) {
                    callback.onStatusChanged(false, false, null, false);
                }


satsukies · 2024-08-20T01:18:04Z

sdk/src/test/java/com/deploygate/sdk/DeployGateSdkConfigurationInterfaceTest.java

Added new testcases for DeployGateSdkConfigration.
It checks all public methods are defined each modules(sdk, sdk-mock).

satsukies · 2024-08-20T01:31:40Z

sdk/src/main/java/com/deploygate/sdk/DeployGateCallback.java

+     * @deprecated since 4.9.0. Use {@link DeployGateInitializeCallback#onInitialized(boolean)} instead.
     */
+    @Deprecated
+    @Override
    public void onInitialized(boolean isServiceAvailable);


We can also delete this method and delegate it to each extended class.
In this case, we cannot mark deprecated to methods in X.

jmatsu

Decomposing the current Callback class sounds great.

Please fix unwanted breaking changes when deprecating methods.

sdkMock/src/main/java/com/deploygate/sdk/DeployGateCallback.java

jmatsu · 2024-08-20T01:44:36Z

sdk/src/main/java/com/deploygate/sdk/DeployGateStatusChangeCallback.java

@@ -0,0 +1,23 @@
+package com.deploygate.sdk;
+
+public interface DeployGateStatusChangeCallback {


Please add javadocs to new classes.

jmatsu · 2024-08-20T01:45:26Z

sdk/src/main/java/com/deploygate/sdk/DeployGateSdkConfiguration.java

@@ -62,6 +73,9 @@ public static final class Builder {
        private boolean isCaptureEnabled = true;

        private DeployGateCallback callback = null;


Please remove here.

jmatsu · 2024-08-20T01:46:40Z

sdk/src/main/java/com/deploygate/sdk/DeployGateSdkConfiguration.java

         */
+        @Deprecated
        public Builder setCallback(DeployGateCallback callback) {
            this.callback = callback;


To keep the current behaviour, at most one callback for each should be allowed.

Suggested change

this.callback = callback;

this.initializeCallback = callback;

this.statusChangeCallback = callback;

this.updateAvailableCallback = callback;

jmatsu · 2024-08-20T01:47:03Z

sdk/src/main/java/com/deploygate/sdk/DeployGateSdkConfiguration.java

@@ -12,8 +12,10 @@ public final class DeployGateSdkConfiguration {

    final boolean isCrashReportingEnabled;

-
    final DeployGateCallback callback;


Please remove here.

jmatsu · 2024-08-20T01:47:44Z

sdk/src/main/java/com/deploygate/sdk/DeployGate.java

     */
+    @Deprecated


Great. Please deprecate these methods in sdk-mock as well.

I couldn't add a comment to registerCallbackInternal method so let me write it here.

Please fix registerCallbackInternal to register a DeployGateCallback to each property.

jmatsu · 2024-08-20T01:51:54Z

sdk/src/main/java/com/deploygate/sdk/DeployGate.java

+     */
+    public static void registerInitializeCallback(
+            DeployGateInitializeCallback callback,
+            boolean refreshImmediately


We don't need to provide methods with boolean refreshImmediately anymore. Alternatively, users should call refresh manually. It will also be able to avoid leading users to many refresh requests hell.

jmatsu · 2024-08-20T01:57:38Z

sdk/src/main/java/com/deploygate/sdk/DeployGate.java

    private final HashSet<DeployGateCallback> mCallbacks;
+    private final HashSet<DeployGateInitializeCallback> mInitializeCallbacks;
+    private final HashSet<DeployGateStatusChangeCallback> mStatusChangeCallbacks;
+    private final HashSet<DeployGateUpdateAvailableCallback> mUpdateAvailableCallbacks;


No, it means minimizing changes to the current code but not minimizing the effect.

The current changes mean we are going to keep maintaining two callback mechanisms until we can completely delete the deprecated entities, even tough the new mechanism is compatible with the legacy one.

The current changes allows at most two callbacks like DeployGateInitializeCallback at the initialization time. It's not a planned feature.

Please remove mCallbacks property and dispatch the callback registration to each property. It will be completely the same behaviour as the current behaviour i.e. it can minimize the effect.

satsukies · 2024-08-20T04:03:40Z

sample/src/main/java/com/deploygate/sample/App.java

+        // with setAuthor method when creating DeployGate SdkConfiguration. like:
        //
-        // DeployGate.install(this, "YOURUSERNAME");
+        // builder.setAuthor("YOURUSERNAME")


describe to use setAuthor(String) method defined in DeployGateSdkConfuration.Builder class because DeployGate.install(Application, String) is deprecated.

satsukies · 2024-08-20T04:08:08Z

@jmatsu
thank you for the review.
I fixed it according to your comments. Please re-check my code.

jmatsu

Why do you wrap a DeployGateCallback by anonymous classes?

jmatsu · 2024-08-20T04:10:56Z

sample/src/main/java/com/deploygate/sample/App.java

-            @Override
-            public void onInitialized(boolean isServiceAvailable) {
+        DeployGateSdkConfiguration configuration = new DeployGateSdkConfiguration.Builder()
+            // Please note that these callback is called if you have removed the content provider.


iff is not a typo by the way. It's an abbreviation of if and only if.

Oh.. reverted this change.

jmatsu · 2024-08-20T04:16:07Z

sdk/src/main/java/com/deploygate/sdk/DeployGate.java

+        mInitializeCallbacks.add(new DeployGateInitializeCallback() {
+            @Override
+            public void onInitialized(boolean isServiceAvailable) {
+                listener.onInitialized(isServiceAvailable);
+            }
+        });
+        mStatusChangeCallbacks.add(new DeployGateStatusChangeCallback() {
+            @Override
+            public void onStatusChanged(boolean isManaged, boolean isAuthorized, String loginUsername, boolean isStopped) {
+                listener.onStatusChanged(isManaged, isAuthorized, loginUsername, isStopped);
+            }
+        });
+        mUpdateAvailableCallbacks.add(new DeployGateUpdateAvailableCallback() {
+            @Override
+            public void onUpdateAvailable(int revision, String versionName, int versionCode) {
+                listener.onUpdateAvailable(revision, versionName, versionCode);
+            }
+        });


This should be compilable. What happened?

Suggested change

mInitializeCallbacks.add(new DeployGateInitializeCallback() {

@Override

public void onInitialized(boolean isServiceAvailable) {

listener.onInitialized(isServiceAvailable);

}

});

mStatusChangeCallbacks.add(new DeployGateStatusChangeCallback() {

@Override

public void onStatusChanged(boolean isManaged, boolean isAuthorized, String loginUsername, boolean isStopped) {

listener.onStatusChanged(isManaged, isAuthorized, loginUsername, isStopped);

}

});

mUpdateAvailableCallbacks.add(new DeployGateUpdateAvailableCallback() {

@Override

public void onUpdateAvailable(int revision, String versionName, int versionCode) {

listener.onUpdateAvailable(revision, versionName, versionCode);

}

});

mInitializeCallbacks.add(listener);

mStatusChangeCallbacks.add(listener);

mUpdateAvailableCallbacks.add(listener);

The current code breaks unregister methods because their identities have been changed.

Sorry.
I added new testcases and checked keeping the behavior between before and after.

jmatsu · 2024-08-20T04:21:05Z

sdk/src/main/java/com/deploygate/sdk/DeployGate.java

        if (sInstance == null) {
            return;
        }
        if (listener == null) {
            return;
        }

-        sInstance.mCallbacks.remove(listener);
+        sInstance.mInitializeCallbacks.remove(new DeployGateInitializeCallback() {


These codes are completely wrong. Please re-check what this code expects.

By the way, the test job passed so the test codes of unregister methods are also broken or not tested. Could you please fix them as well?

No testcase for register/unregister methods exists.
Added it and fixed my code causing unexpected behavior.

jmatsu · 2024-08-20T04:22:02Z

sdk/src/main/java/com/deploygate/sdk/DeployGateCallback.java

 /**
 * A callback interface to receive DeployGate events. Implement this and pass to
 * {@link DeployGate#registerCallback(DeployGateCallback, boolean)} to listen.
 *
 * @author tnj
+ * @deprecated since 4.9.0. Use each extended callback interfaces instead.


Suggested change

* @deprecated since 4.9.0. Use each extended callback interfaces instead.

* @deprecated since 4.9.0. Use fine-grained callback interfaces for each purpose.

jmatsu · 2024-08-20T04:23:44Z

sdk/src/main/java/com/deploygate/sdk/DeployGateInitializeCallback.java

+ * A callback interface to receive DeployGate service initialization events.Implement this and pass to
+ * {@link DeployGate#registerInitializeCallback(DeployGateInitializeCallback)} to listen.


It would be hard to maintain comments if you embedded the detailed usages. see annotation is enough.

Suggested change

* A callback interface to receive DeployGate service initialization events.Implement this and pass to

* {@link DeployGate#registerInitializeCallback(DeployGateInitializeCallback)} to listen.

* A callback interface to receive DeployGate service initialization events.

jmatsu · 2024-08-20T04:24:20Z

sdk/src/main/java/com/deploygate/sdk/DeployGateSdkConfiguration.java

-            this.callback = callback;
+        @Deprecated
+        public Builder setCallback(final DeployGateCallback callback) {
+            this.setInitializeCallback(new DeployGateInitializeCallback() {


…ethod

satsukies · 2024-08-20T10:41:44Z

@jmatsu
Sorry, the behavior was broken as mentioned in the comments from you.

I added some testcases for check behavior of DeployGate#register, DeployGate#unregister, and DeployGateSdkConfiguration.Builder class.

1d45993...190ef4c

Please re-check this PR. Thanks.

satsukies · 2024-08-20T10:47:04Z

sdk/src/main/java/com/deploygate/sdk/DeployGate.java

+    static HashSet<DeployGateInitializeCallback> getInitializeCallbacks() {
+        return sInstance != null ? sInstance.mInitializeCallbacks : null;
+    }
+
+    static HashSet<DeployGateStatusChangeCallback> getStatusChangeCallbacks() {
+        return sInstance != null ? sInstance.mStatusChangeCallbacks : null;
+    }
+
+    static HashSet<DeployGateUpdateAvailableCallback> getUpdateAvailableCallbacks() {
+        return sInstance != null ? sInstance.mUpdateAvailableCallbacks : null;
+    }


Some test cases use these package-private methods.

jmatsu

Almost great. Only a few fix is required.

jmatsu · 2024-08-20T10:45:05Z

sdk/src/main/java/com/deploygate/sdk/DeployGate.java

+    static HashSet<DeployGateInitializeCallback> getInitializeCallbacks() {
+        return sInstance != null ? sInstance.mInitializeCallbacks : null;
+    }
+
+    static HashSet<DeployGateStatusChangeCallback> getStatusChangeCallbacks() {
+        return sInstance != null ? sInstance.mStatusChangeCallbacks : null;
+    }
+
+    static HashSet<DeployGateUpdateAvailableCallback> getUpdateAvailableCallbacks() {
+        return sInstance != null ? sInstance.mUpdateAvailableCallbacks : null;
+    }


Suggested change

static HashSet<DeployGateInitializeCallback> getInitializeCallbacks() {

return sInstance != null ? sInstance.mInitializeCallbacks : null;

}

static HashSet<DeployGateStatusChangeCallback> getStatusChangeCallbacks() {

return sInstance != null ? sInstance.mStatusChangeCallbacks : null;

}

static HashSet<DeployGateUpdateAvailableCallback> getUpdateAvailableCallbacks() {

return sInstance != null ? sInstance.mUpdateAvailableCallbacks : null;

}

/**

* Not a public API. This is only for testing.

*/

static HashSet<DeployGateInitializeCallback> getInitializeCallbacks() {

return sInstance != null ? sInstance.mInitializeCallbacks : null;

}

/**

* Not a public API. This is only for testing.

*/

static HashSet<DeployGateStatusChangeCallback> getStatusChangeCallbacks() {

return sInstance != null ? sInstance.mStatusChangeCallbacks : null;

}

/**

* Not a public API. This is only for testing.

*/

static HashSet<DeployGateUpdateAvailableCallback> getUpdateAvailableCallbacks() {

return sInstance != null ? sInstance.mUpdateAvailableCallbacks : null;

}

sdkMock/src/main/java/com/deploygate/sdk/DeployGateSdkConfiguration.java

jmatsu · 2024-08-20T10:48:17Z

sdk/src/test/java/com/deploygate/sdk/DeployGateTest.java

+import org.junit.runner.RunWith;
+
+@RunWith(AndroidJUnit4.class)
+public class DeployGateTest {


…Mock

jmatsu

Well done!

satsukies · 2024-08-20T11:09:31Z

Thanks!

satsukies added 8 commits August 19, 2024 16:59

chore: separate each method in DeloyGateCallback to interface class

b5ac092

feat: Add method for set each callbacks to DeployGateSdkConfiguration…

c75248c

….Builder

chore: add new callback interfaces to sdkMock module

36e88dc

feat: add register/unregister method for new callback interfaces

3d46edb

chore: refactoring sample codes

5d6efe5

chore: add methods to DeployGateSdkConfiguration at sdkMock module

9774b93

test: add testcase of new interfaces

f7d85ba

test: add new testcase about public methods definition on DeployGateS…

69bd4fc

…dkConfigration in each module

satsukies self-assigned this Aug 20, 2024

satsukies commented Aug 20, 2024

View reviewed changes

satsukies requested a review from jmatsu August 20, 2024 01:18

satsukies commented Aug 20, 2024

View reviewed changes

jmatsu suggested changes Aug 20, 2024

View reviewed changes

satsukies added 6 commits August 20, 2024 11:39

fix: remove class body

7a6f81d

fix: mark deprecated in sdkMock module

6bee4f7

fix: delete member of deprecated interface

bcd6b86

fix: remove 'refreshImmediately' argument

41bdd9c

chore: update sample code

c361d01

chore: add javadoc

3954e1c

satsukies commented Aug 20, 2024

View reviewed changes

fix: forgot semi-colon

4062288

satsukies requested a review from jmatsu August 20, 2024 04:08

jmatsu suggested changes Aug 20, 2024

View reviewed changes

test: add testcase of register/unregister callbacks

1d45993

satsukies added 4 commits August 20, 2024 18:36

fix: use deprecated callback directly when register/unregister

ee6bd30

chore: separate unregister logic to 'unregisterXXXInternal' private m…

0ff07a4

…ethod

fix: improve comments

083c8f9

fix: treat DeployGateCallback as fine-grained callbacks

190ef4c

satsukies requested a review from jmatsu August 20, 2024 10:41

satsukies commented Aug 20, 2024

View reviewed changes

jmatsu suggested changes Aug 20, 2024

View reviewed changes

satsukies added 3 commits August 20, 2024 19:57

chore: add comment to package-private static methods

6af347c

chore: add no-op comment

12883ac

fix: deprecated DeployGateSdkConfiguration.Builder#setCallback in sdk…

7d5d76b

…Mock

satsukies requested a review from jmatsu August 20, 2024 11:08

jmatsu approved these changes Aug 20, 2024

View reviewed changes

satsukies merged commit b4aa1bc into master Aug 20, 2024
6 checks passed

satsukies deleted the feat/separate-callback-interface branch August 20, 2024 11:09

satsukies mentioned this pull request Aug 23, 2024

Add new callback interface to receive the Capture creation event #96

Merged

		@@ -0,0 +1,23 @@
		package com.deploygate.sdk;

		public interface DeployGateStatusChangeCallback {

		@@ -62,6 +73,9 @@ public static final class Builder {
		private boolean isCaptureEnabled = true;

		private DeployGateCallback callback = null;

-            this.callback = callback;
+            this.initializeCallback = callback;
+            this.statusChangeCallback = callback;
+            this.updateAvailableCallback = callback;

		@@ -12,8 +12,10 @@ public final class DeployGateSdkConfiguration {

		final boolean isCrashReportingEnabled;


		final DeployGateCallback callback;

	* @deprecated since 4.9.0. Use each extended callback interfaces instead.
	* @deprecated since 4.9.0. Use fine-grained callback interfaces for each purpose.

		* A callback interface to receive DeployGate service initialization events.Implement this and pass to
		* {@link DeployGate#registerInitializeCallback(DeployGateInitializeCallback)} to listen.

Add new callback interfaces split from DeployGateCallback #95

Add new callback interfaces split from DeployGateCallback #95

Conversation

satsukies commented Aug 20, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

satsukies Aug 20, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jmatsu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

satsukies commented Aug 20, 2024

jmatsu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

satsukies commented Aug 20, 2024

satsukies Aug 20, 2024 • edited Loading

Choose a reason for hiding this comment

jmatsu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jmatsu left a comment

Choose a reason for hiding this comment

satsukies commented Aug 20, 2024

satsukies commented Aug 20, 2024 •

edited

Loading

satsukies Aug 20, 2024 •

edited

Loading

satsukies Aug 20, 2024 •

edited

Loading