Move Notifications to get triggered from NotificationEventListener #2395
Conversation
|
|
sahil2441
force-pushed
the
sahil2441/Issue-2394
branch
from
6388c9c to
8efb01b
Compare
leakcanary-android-core/src/main/java/leakcanary/EventListener.kt
Outdated
Show resolved
Hide resolved
|
Hi, Can above solution allow to suppress or delay creation of notification channel after enabling permission ? Thanks, |
@ashish-kshirsagar Can you file a new issue that references this PR, with a screenshot and ideally a repro scenario? |
leakcanary-android-core/src/main/java/leakcanary/NotificationEventListener.kt
Outdated
Show resolved
Hide resolved
leakcanary-android-core/src/main/java/leakcanary/internal/InternalLeakCanary.kt
Outdated
Show resolved
Hide resolved
leakcanary-android-core/src/main/java/leakcanary/internal/HeapDumpTrigger.kt
Outdated
Show resolved
Hide resolved
sahil2441
changed the title
|
Strange that the unit tests which are failing in CI jobs are passing locally with: @pyricau Looks like it's due to difference in heap size expected; Does the heap size here vary with the machine it's generated on? |
|
@pyricau PTAL |
|
@pyricau ptal |
| @@ -154,7 +141,6 @@ internal class HeapDumpTrigger( | |||
| return | |||
| } | |||
|
|
|||
| dismissRetainedCountNotification() | |||
There was a problem hiding this comment.
Looks like this has been replaced by new behavior in the event listener class, as I'm seeing no direct call to dismissRetainedCountNotification()
| @@ -193,10 +176,9 @@ internal class HeapDumpTrigger( | |||
| lastDisplayedRetainedObjectCount = 0 | |||
| lastHeapDumpUptimeMillis = SystemClock.uptimeMillis() | |||
| objectWatcher.clearObjectsWatchedBefore(heapDumpUptimeMillis) | |||
| currentEventUniqueId = UUID.randomUUID().toString() | |||
There was a problem hiding this comment.
currentEventUniqueId was previously changing on every actual dump. Looks like that's not the case anymore? So all dump heap events get the same id? That doesn't seem right?
| assertThat(retained after FINDING_DOMINATORS).isEqualTo(7.02 MB +-5 % margin) | ||
| assertThat(retained after INSPECTING_OBJECTS).isEqualTo(7.02 MB +-5 % margin) | ||
| assertThat(retained after COMPUTING_NATIVE_RETAINED_SIZE).isEqualTo(7.02 MB +-5 % margin) | ||
| assertThat(retained after COMPUTING_RETAINED_SIZE).isEqualTo(5.77 MB +-5 % margin) |
There was a problem hiding this comment.
Definitely an unexpected change, yes it changes per env. I'd try rebasing & seeing if CI passes with and without this.
| ) | ||
| } | ||
|
|
||
| is Event.ShowRetainedCountNotification -> { |
There was a problem hiding this comment.
Ideally the names of the events wouldn't have "Notification" in them, and instead describe "what just happened". Then the notification listener can turn those events into the proper notifications.
|
Thanks so much for your work Sahil! I'm sorry it took so long for me to review. After thinking through this some more, I'm realizing our notification code is tightly coupled to the rest of the logic and hard to move around without introducing bugs , which is why I hadn't moved those notifications in the first place. I'm not super comfortable with the risk involved here, so I went back to the original ticket and thought about your concrete need, and then landed a PR with less changes, though that leaves the current code in its not so great shape: #2440 I hope you won't mind me closing this PR. |
|
@pyricau ty! no worries. |
Fixes: #2394
Change
This PR moves the notifications triggering to the
NotificationEventListener.Test Plan
./gradlew build --stacktrace./gradlew leakcanary-android-sample:installDebugand ensure Notifications work as intended: