Simplify custom notification trigger example and move to activity #4255
Conversation
lfkellogg
force-pushed
the
lk/custom-notification-activity
branch
from
071f82e to
cb38a28
Compare
Coverage Report 1Affected Products
Test Logs |
lfkellogg
force-pushed
the
lk/custom-notification-activity
branch
from
cb38a28 to
7e3e9cf
Compare
lfkellogg
force-pushed
the
lk/custom-notification-activity
branch
from
7e3e9cf to
252336e
Compare
Size Report 1Affected ProductsNo changes between base commit (9d3bed7) and merge commit (ecb32c1).Test Logs |
...c/main/java/com/googletest/firebase/appdistribution/testapp/ShakeDetectionFeedbackTrigger.kt
Show resolved
Hide resolved
| return | ||
| } | ||
| fun showNotification(activity: Activity) { | ||
| synchronized(this) { |
There was a problem hiding this comment.
See below, removing
| Log.d(TAG, "setting current activity") | ||
| activityToScreenshot = activity | ||
| fun cancelNotification() { | ||
| synchronized(this) { |
There was a problem hiding this comment.
See below, removing
| if (activity == null) { | ||
| Log.e(TAG, "Can't take screenshot because activity is unknown") | ||
| return | ||
| synchronized(CustomNotificationFeedbackTrigger) { |
There was a problem hiding this comment.
Why? Also, I think this synchronizes on the class - not the instance like the other two synchronize blocks.
Maybe this is an attempt to somehow protect modifications to activityToScreenshot, but I think read/writes to reference variables are atomic.
The assignment to the local variable activity on line 109 makes sure that the value doesn't change between the null check on line 110 and line 114. Inlining CustomNotificationFeedbackTrigger.activityToScreenshot without synchronization would indeed be a problem.
So I think we should add a comment to the assignment on line 109 talking about this issue.
There was a problem hiding this comment.
Maybe this is an attempt to somehow protect modifications to activityToScreenshot, but I think read/writes to reference variables are atomic.
I wasn't trying to protect the actual reference and assignment, but rather the surrounding logic that depends on the reference being set to a particular activity, or to null. For example what if we copied CustomNotificationFeedbackTrigger.activityToScreenshot into a non-nullable reference, and then the activity is destroyed before we try to capture the screenshot. However looking again that particular failure case isn't a concern because onCreate and onDestroy will both be called on the main thread. Or maybe I was concerned about someone calling showNotification multiple times in rapid succession from a different thread, but even that wouldn't be a problem AFAICT... After the weekend I can't remember. Comments would have been helpful :) Either way I'll remove these synchronized blocks.
I think this synchronizes on the class - not the instance like the other two synchronize blocks
They all synchronize on the CustomNotificationFeedbackTrigger object. Above we are in the context of the object, hence synchronized(this), and here we are not, so we synchronize on the object reference, synchronized(CustomNotificationFeedbackTrigger). I see how this could be confusing so maybe in cases like this I should just use the latter everywhere.
Uh oh!
There was an error while loading. Please reload this page.