Fix various crashes #3533
Fix various crashes #3533
Conversation
…k, so not need to add an extra one. Also the block can return a non-null TaskHandle.
bmarty
added
Record-Screenshots
github-actions
bot
removed
the
Record-Screenshots
|
📱 Scan the QR code below to install the build (arm64 only) for this PR. |
| try { | ||
| managedLauncher.launch(defaultRequest) | ||
| } catch (activityNotFoundException: ActivityNotFoundException) { | ||
| Timber.w(activityNotFoundException, "No activity found") |
There was a problem hiding this comment.
Should we use toast int these 2 cases too besides just logging the issue?
There was a problem hiding this comment.
The issue happen when the device has no Camera application, so I would not bother too much.
| PdfRenderer(parcelFileDescriptor) | ||
| }.fold( | ||
| onSuccess = { pdfRenderer -> | ||
| this@PdfRendererManager.pdfRenderer = pdfRenderer |
There was a problem hiding this comment.
Maybe directly do:
runCatching {
pdfRenderer = PdfRenderer(parcelFileDescriptor)
}So we get rid of the ugly this@pdfRendererManager.pdfRenderer = ... line.
There was a problem hiding this comment.
ugly yes, but I need the runCatching to return the value.
Maybe using a also block like
runCatching {
PdfRenderer(parcelFileDescriptor).also {
pdfRenderer = it
}
}.fold(
onSuccess = { pdfRenderer ->
// Preload just 3 pages so we can render faster
val firstPages = pdfRenderer.loadPages(from = 0, to = 3)
mutablePdfPages.value = AsyncData.Success(firstPages.toImmutableList())
val nextPages = pdfRenderer.loadPages(from = 3, to = pdfRenderer.pageCount)
mutablePdfPages.value = AsyncData.Success((firstPages + nextPages).toImmutableList())
},
onFailure = {
mutablePdfPages.value = AsyncData.Failure(it)
}
)is less ugly?
There was a problem hiding this comment.
I have found another solution 55704f0
There was a problem hiding this comment.
I think the change is ok and maybe I'm overlooking something but with the change I proposed wouldn't the pdfRenderer property only be assigned if the initiailzation using the parcelFileDescriptor works too?
There was a problem hiding this comment.
Yes, but with the change you proposed, the result of the assignation is Unit, so I can't get the PdfRenderer instance in the onSuccess parameter block.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #3533 +/- ##
===========================================
- Coverage 82.68% 82.63% -0.06%
===========================================
Files 1732 1732
Lines 40904 40940 +36
Branches 4973 4980 +7
===========================================
+ Hits 33823 33829 +6
- Misses 5316 5343 +27
- Partials 1765 1768 +3 ☔ View full report in Codecov by Sentry. |
| @@ -105,8 +105,12 @@ class RustMatrixRoom( | |||
|
|
|||
| override val roomInfoFlow: Flow<MatrixRoomInfo> = mxCallbackFlow { | |||
| launch { | |||
There was a problem hiding this comment.
Yes, will try. I did not want to change the logic.
| .getOrNull() | ||
| ?.let(matrixRoomInfoMapper::map) | ||
| ?.let { initial -> | ||
| channel.trySend(initial) |
There was a problem hiding this comment.
use channel.send instead of trySend?
There was a problem hiding this comment.
I did not want to change the existing code here. Maybe in another PR?
|
bmarty
added
the
Run-Maestro
github-actions
bot
removed
the
Run-Maestro
Content
Fix various crashes reported by Sentry.
Also cleanup the code at a few location.
To be reviewed commit per commit.
Motivation and context
More stable application.
Screenshots / GIFs
Tests
Not always easy to repro the crashes.
For the PDF crash, changing the extension of a Gif file (for instance), then send it to a room and try to open it made the application crash. It's no fixed and display an inlined error.
Tested devices
Checklist