Register intent-filter to import hprof files #2122
Conversation
It will be set for both: - Import Heap Dump action - Open Heap Dump file
leakcanary-android-core/src/androidTest/java/leakcanary/LeakActivityTest.kt
Show resolved
Hide resolved
leakcanary-android-core/src/main/java/leakcanary/internal/activity/LeakActivity.kt
Outdated
Show resolved
Hide resolved
| if (intent?.action != Intent.ACTION_VIEW) return | ||
| val uri = intent.data ?: return | ||
| if (uri.lastPathSegment?.endsWith(".hprof") != true) return | ||
| resetTo(HeapDumpsScreen()) |
There was a problem hiding this comment.
This is ok but based on the video there's a little amount of time where one might think they have to click on "import hprof" on that new screen, when in fact the hprof was already being imported. So now I'm wondering if we should add a virtual row for "pending analysis" on that screen (for this new feature but also any time we import).
There was a problem hiding this comment.
That would be a nice addition indeed.
I'll try to look into that, but it seems quite more involving (modifying HeapAnalysisTable.Projection I guess).
There was a problem hiding this comment.
I'm not sure we'd want to update the DB as this is very much an "in progress" thing that's only true while the program is running, so we'd risk having out of date info after a restart.
We could probably file this as a follow up... but I'm thinking maybe an internal thing that keeps track of "are we running an analysis right now" and a way to subscribe to that.
| @@ -144,7 +156,7 @@ internal class LeakActivity : NavigatingActivity() { | |||
| input.copyTo(output, DEFAULT_BUFFER_SIZE) | |||
| } | |||
| } | |||
| HeapAnalyzerService.runAnalysis(this, target) | |||
| HeapAnalyzerService.runAnalysis(this, target, heapDumpReason = "Imported by user") | |||
There was a problem hiding this comment.
This made me realize I forgot to add a reason when calling it from leakcanary.internal.activity.LeakActivity#importHprof
Ideally we'd differentiate between the two types of import though.
There was a problem hiding this comment.
Sure, I can do that. But for me this is the same reason, but from different paths.
How would you differentiate that? Imported by user / Opened by user.
There was a problem hiding this comment.
yeah actually the words should be the same.
There was a problem hiding this comment.
Then should we force having to provide a non-null heapDumpReason when calling. And therefore move the "Unknown" fallback to the onHandleIntentInForeground body and fix at the same time the type mismatch Pair<String, String> vs Pair<String, String?> at
|
Thx, this is a great PR! |
|
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
| LeakCanary.config = LeakCanary.config.copy(onHeapAnalyzedListener = { | ||
| DefaultOnHeapAnalyzedListener.create().onHeapAnalyzed(it) | ||
| latch.countDown() | ||
| }) |
There was a problem hiding this comment.
@pyricau Do you think we should be extra careful here and reset to the default OnHeapAnalyzedListener?
We could either do this in @Before and recreate a new Config or maybe in a JUnit rule?
There was a problem hiding this comment.
I do think we should reset it, yes, though I would do that at the end of the current test, and wrap the whole thing in a run / finally to make sure we reset it even on failures.
There was a problem hiding this comment.
Not sure about the best naming though.
I tried with tryAndRestoreConfig(block: () -> Unit) 35d1d3c.
|
Thanks! |
* Register intent-filter to import hprof files Relates to square#2096 * Set heapDumpReason to "Imported by user" for imported files It will be set for both: - Import Heap Dump action - Open Heap Dump file * Test importing hprof file and navigate to HeapDumpScreen * Show Toast when URI has an unsupported file extension * Document pathPattern workaround and mention stackoverflow source * Fix flaky test by waiting for the HeapAnalysis to finish * Wrap UI test to restore original LeakCanary.config even if it fails
Here is my take on this. Let me know what you think about the implementation.
It uses an intent-filter workaround because of how Android allow us to specify
pathPattern(limited regex support and no standard MIME type).I've also forced to reset to the HeapDumps screen which seems better for what we're trying to achieve here.
I've tested it on Android's file explorer (open with), Google's Files app, and Solid Explorer.
There are 2 possible outcomes when we try to open such files:
open.hprof.files.mp4
Resolves #2096