Add fix for popup crash #997
Conversation
There was a problem hiding this comment.
Pull Request Overview
Fixes a popup crash that occurs when tapping on map features by addressing null image downloads and missing user agent headers. The server configuration changed to require user agent headers and the popup component needs to handle null images gracefully.
- Add user agent header to image requests to comply with server requirements
- Implement fallback to default drawable when image download fails
- Remove exception throwing for null source URLs to prevent crashes
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| no_image_32.xml | Adds default image drawable for fallback when image download fails |
| MediaElementState.kt | Updates image loading logic with user agent header and null handling |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| private val refreshInterval: Long, | ||
| @Suppress("unused") private val linkUrl: String, | ||
| private val sourceUrl: String, | ||
| @Suppress("unused") private val sourceUrl: String, |
There was a problem hiding this comment.
The suppression annotations are inconsistent. refreshInterval had its suppression removed but sourceUrl still has it, yet both appear to be unused in the shown code. Consider reviewing if these parameters are actually used elsewhere or if they should both have consistent suppression annotations.
| @Suppress("unused") private val sourceUrl: String, | |
| private val sourceUrl: String, |
Copilot uses AI. Check for mistakes.
| ) { | ||
| val request = ImageRequest.Builder(context) | ||
| .data(srcUrl) | ||
| .addHeader("User-Agent", "ArcGIS Maps SDK for Kotlin Toolkit") |
There was a problem hiding this comment.
The user agent string is hardcoded. Consider extracting this to a constant or configuration value to make it easier to maintain and update across the codebase.
Copilot uses AI. Check for mistakes.
| (context.imageLoader.execute(request).drawable as? BitmapDrawable)?.bitmap | ||
| ?: throw IllegalStateException("couldn't load image at $srcUrl") | ||
| ?: ContextCompat.getDrawable(context, R.drawable.no_image_32)?.toBitmap() | ||
| ?: throw IllegalStateException("couldn't load image at $srcUrl or placeholder drawable") |
There was a problem hiding this comment.
The error message references $srcUrl but when srcUrl is empty (line 207), this will show an unclear error message. Consider using a more descriptive error message that doesn't rely on the potentially empty srcUrl variable.
| ?: throw IllegalStateException("couldn't load image at $srcUrl or placeholder drawable") | |
| ?: throw IllegalStateException( | |
| if (srcUrl.isNullOrEmpty()) { | |
| "couldn't load image: sourceUrl is missing or empty, and placeholder drawable could not be loaded" | |
| } else { | |
| "couldn't load image at $srcUrl, and placeholder drawable could not be loaded" | |
| } | |
| ) |
Copilot uses AI. Check for mistakes.
1 participant
Related to issue: # https://devtopia.esri.com/runtime/kotlin/issues/6482
Description:
The popup component currently crashes when you tap on a feature on the map to load its popup. The MediaElementState currently throws an exception when the Image downloaded is null, leading to the crash. The configuration on the server changed recently where it has stopped returning images if there is no user agent in the header of the request.
Swift is using
ArcGISEnvironment.urlSession.data(from: url)to download their images which probably applies theuser-agentto the requests.Summary of changes:
Pre-merge Checklist