ADFA-3972 | Add visual hints for image placeholders#1287
Merged
jatezzz merged 3 commits intoMay 11, 2026
Conversation
Contributor
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (9)
📝 WalkthroughWalkthroughThis PR extracts detection rendering and XML persistence into utilities (DetectionVisualizer, XmlFileManager), refactors DrawableImportHelper with private helpers and a new suspend deleteDrawable API, adds a RemovePlaceholderImage event, and wires deletion through the ViewModel and Activity (tap handling, toasts, and file-save delegation). ChangesPlaceholder Image Deletion Flow
Sequence Diagram(s)sequenceDiagram
participant User
participant Activity
participant ViewModel
participant DI as DrawableImportHelper
participant Visualizer as DetectionVisualizer
participant FileManager as XmlFileManager
User->>Activity: tap image (x,y)
Activity->>Visualizer: getTappedDeleteIconId(x,y)
alt delete icon tapped
Activity->>ViewModel: RemovePlaceholderImage(placeholderId)
ViewModel->>DI: deleteDrawable(layoutFilePath, resourceName)
DI-->>ViewModel: Result<Boolean>
ViewModel-->>Activity: Show toast / ShowError
else placeholder tapped
Activity->>ViewModel: ImagePlaceholderTapped(...)
end
ViewModel->>Activity: FileSaved(xmlString)
Activity->>FileManager: saveXmlToDownloads(xmlString)
FileManager-->>Activity: Result<String>
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
cv-image-to-xml/src/main/java/org/appdevforall/codeonthego/computervision/utils/DetectionVisualizer.kt (1)
129-129: 💤 Low valueRemove debug log from production code.
Log.d("DetectionVisualizer", "Visualizing ${detections.size} detections")fires on every call tovisualize(), which is triggered on every detection re-render.🧹 Proposed fix
- Log.d("DetectionVisualizer", "Visualizing ${detections.size} detections") return mutableBitmap🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cv-image-to-xml/src/main/java/org/appdevforall/codeonthego/computervision/utils/DetectionVisualizer.kt` at line 129, The debug Log.d call in DetectionVisualizer.visualize() floods logs on every re-render; remove the line Log.d("DetectionVisualizer", "Visualizing ${detections.size} detections") (or replace it with a conditional/level-appropriate telemetry call) so production builds no longer emit this debug message; update DetectionVisualizer.visualize() accordingly and keep any necessary production logging at info/warn level or behind a debug flag.cv-image-to-xml/src/main/java/org/appdevforall/codeonthego/computervision/utils/XmlFileManager.kt (1)
53-67: 🏗️ Heavy lift
MediaStore.insertaccumulates duplicate files on Android Q+ instead of overwriting.On API 29 and above,
MediaStore.insertwon't overwrite files when an existing file already exists, but will add a new entry to the MediaStore collection using a slightly different name. Repeated exports will accumulate files such astesting_result.xml,testing_result (1).xml, etc. in Downloads. The pre-QFileOutputStreampath overwrites in place, so there is a behavioral inconsistency between API levels.To match the overwrite behavior on Q+, query for an existing entry by
DISPLAY_NAMEfirst, delete it if found, and then insert the new record.♻️ Suggested approach for overwrite-on-Q+
`@RequiresApi`(Build.VERSION_CODES.Q) private fun saveUsingMediaStore(xmlString: String, fileName: String) { val resolver = context.contentResolver + + // Delete any existing entry with the same name to avoid duplicates + resolver.query( + MediaStore.Downloads.EXTERNAL_CONTENT_URI, + arrayOf(MediaStore.MediaColumns._ID), + "${MediaStore.MediaColumns.DISPLAY_NAME} = ?", + arrayOf(fileName), + null + )?.use { cursor -> + if (cursor.moveToFirst()) { + val id = cursor.getLong(0) + val existingUri = ContentUris.withAppendedId( + MediaStore.Downloads.EXTERNAL_CONTENT_URI, id + ) + resolver.delete(existingUri, null, null) + } + } + val contentValues = ContentValues().apply {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cv-image-to-xml/src/main/java/org/appdevforall/codeonthego/computervision/utils/XmlFileManager.kt` around lines 53 - 67, saveUsingMediaStore currently always calls resolver.insert which creates duplicate entries on API 29+; modify saveUsingMediaStore to first query MediaStore.Downloads for existing rows with MediaStore.MediaColumns.DISPLAY_NAME == fileName (use resolver.query on MediaStore.Downloads.EXTERNAL_CONTENT_URI and selection on DISPLAY_NAME), delete any found entries via resolver.delete(uri, null, null) (handle multiple matches by deleting all), then call resolver.insert and openOutputStream as before so the MediaStore path overwrites rather than accumulates duplicates; keep existing IOException handling and ensure you reference the same symbols (saveUsingMediaStore, resolver, MediaStore.Downloads.EXTERNAL_CONTENT_URI, MediaStore.MediaColumns.DISPLAY_NAME) when making the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@cv-image-to-xml/src/main/java/org/appdevforall/codeonthego/computervision/ui/ComputerVisionActivity.kt`:
- Around line 117-125: The UI update path skips calling
detectionVisualizer.visualize() when state.hasDetections is false, leaving stale
badge rectangles in DetectionVisualizer.deleteIconClickableAreas so
handleImageTap()'s getTappedDeleteIconId() can match old areas; fix by ensuring
the visualizer cache is cleared when detections disappear — either always call
detectionVisualizer.visualize(...) from updateUi() (pass an empty list/empty
detections and the current bitmap when state.hasDetections is false) or add and
call a clear method on DetectionVisualizer (e.g.,
clearDeleteIconClickableAreas()) from updateUi() when detections were removed so
getTappedDeleteIconId() cannot match stale rectangles.
In
`@cv-image-to-xml/src/main/java/org/appdevforall/codeonthego/computervision/ui/viewmodel/ComputerVisionViewModel.kt`:
- Around line 455-477: The current condition in removePlaceholderImage
incorrectly treats any Result.failure as success because exceptionOrNull() is
always an Exception; change the success check to use deletionResult.isSuccess
only, so state is only cleared on confirmed deletion; on failure (when
!deletionResult.isSuccess) do not update selectedImagesByPlaceholderId, send
ComputerVisionEffect.ShowError with the
deletionResult.exceptionOrNull()?.message (or a user-friendly message) and log
the exception from drawableImportHelper.deleteDrawable so failures are visible
for debugging; keep the _uiState.update call and
ComputerVisionEffect.ShowToast(R.string.msg_image_removed) only inside the
deletionResult.isSuccess branch.
In
`@cv-image-to-xml/src/main/java/org/appdevforall/codeonthego/computervision/utils/XmlFileManager.kt`:
- Line 31: The default filename "testing_result.xml" in
saveXmlToDownloads(xmlString: String, fileName: String = "testing_result.xml")
is a leftover test artifact and should be replaced with a descriptive default or
promoted to a constant; change the default parameter to a clearer name such as
"layout_result.xml" and/or extract it to a named constant (e.g.,
DEFAULT_XML_FILENAME) and update any usages and docs to reference that constant
for clarity and maintainability.
---
Nitpick comments:
In
`@cv-image-to-xml/src/main/java/org/appdevforall/codeonthego/computervision/utils/DetectionVisualizer.kt`:
- Line 129: The debug Log.d call in DetectionVisualizer.visualize() floods logs
on every re-render; remove the line Log.d("DetectionVisualizer", "Visualizing
${detections.size} detections") (or replace it with a
conditional/level-appropriate telemetry call) so production builds no longer
emit this debug message; update DetectionVisualizer.visualize() accordingly and
keep any necessary production logging at info/warn level or behind a debug flag.
In
`@cv-image-to-xml/src/main/java/org/appdevforall/codeonthego/computervision/utils/XmlFileManager.kt`:
- Around line 53-67: saveUsingMediaStore currently always calls resolver.insert
which creates duplicate entries on API 29+; modify saveUsingMediaStore to first
query MediaStore.Downloads for existing rows with
MediaStore.MediaColumns.DISPLAY_NAME == fileName (use resolver.query on
MediaStore.Downloads.EXTERNAL_CONTENT_URI and selection on DISPLAY_NAME), delete
any found entries via resolver.delete(uri, null, null) (handle multiple matches
by deleting all), then call resolver.insert and openOutputStream as before so
the MediaStore path overwrites rather than accumulates duplicates; keep existing
IOException handling and ensure you reference the same symbols
(saveUsingMediaStore, resolver, MediaStore.Downloads.EXTERNAL_CONTENT_URI,
MediaStore.MediaColumns.DISPLAY_NAME) when making the change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2b9156d4-250f-43aa-9095-4a0e5a6295a9
📒 Files selected for processing (9)
cv-image-to-xml/src/main/java/org/appdevforall/codeonthego/computervision/data/repository/DrawableImportHelper.ktcv-image-to-xml/src/main/java/org/appdevforall/codeonthego/computervision/ui/ComputerVisionActivity.ktcv-image-to-xml/src/main/java/org/appdevforall/codeonthego/computervision/ui/ComputerVisionEvent.ktcv-image-to-xml/src/main/java/org/appdevforall/codeonthego/computervision/ui/viewmodel/ComputerVisionViewModel.ktcv-image-to-xml/src/main/java/org/appdevforall/codeonthego/computervision/utils/DetectionVisualizer.ktcv-image-to-xml/src/main/java/org/appdevforall/codeonthego/computervision/utils/XmlFileManager.ktcv-image-to-xml/src/main/res/drawable/ic_placeholder_delete.xmlcv-image-to-xml/src/main/res/drawable/ic_placeholder_upload.xmlcv-image-to-xml/src/main/res/values/strings.xml
jatezzz
force-pushed
the
feat/ADFA-3972-image-placeholder-visuals-experimental
branch
from
e3c1649 to
65a713c
Compare
Daniel-ADFA
reviewed
May 11, 2026
Daniel-ADFA
approved these changes
May 11, 2026
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
cv-image-to-xml/src/main/java/org/appdevforall/codeonthego/computervision/utils/DetectionVisualizer.kt (1)
219-219: 💤 Low valueConsider if alpha setting is necessary.
Setting
alpha = 255on every draw call might be redundant since the drawables are already mutated (lines 70, 76) and should retain their state. This is not a bug, but if the drawables are already fully opaque, this line could be removed for minor performance optimization.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cv-image-to-xml/src/main/java/org/appdevforall/codeonthego/computervision/utils/DetectionVisualizer.kt` at line 219, The call setting iconDrawable.alpha = 255 on each draw is likely redundant; inspect DetectionVisualizer.kt (look for the iconDrawable variable and its mutate() calls around where draw icons are prepared, e.g., the mutate() calls near lines ~70 and ~76) and remove the per-draw assignment of iconDrawable.alpha = 255 if the drawable is already mutated and intended to be fully opaque, ensuring no other code relies on resetting alpha there (or alternatively move a single alpha initialization to the drawable setup path instead of on every draw).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In
`@cv-image-to-xml/src/main/java/org/appdevforall/codeonthego/computervision/utils/DetectionVisualizer.kt`:
- Line 219: The call setting iconDrawable.alpha = 255 on each draw is likely
redundant; inspect DetectionVisualizer.kt (look for the iconDrawable variable
and its mutate() calls around where draw icons are prepared, e.g., the mutate()
calls near lines ~70 and ~76) and remove the per-draw assignment of
iconDrawable.alpha = 255 if the drawable is already mutated and intended to be
fully opaque, ensuring no other code relies on resetting alpha there (or
alternatively move a single alpha initialization to the drawable setup path
instead of on every draw).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 6b765b1e-51ad-4c8b-b28d-d6b8bbb2a006
📒 Files selected for processing (1)
cv-image-to-xml/src/main/java/org/appdevforall/codeonthego/computervision/utils/DetectionVisualizer.kt
Extract rendering and file management logic into dedicated helper classes.
…ionResult` conditional and change the layout result filename from `testing_result.xml` to `layout_result.xml`
jatezzz
force-pushed
the
feat/ADFA-3972-image-placeholder-visuals-experimental
branch
from
6fdb6c4 to
2de2ad8
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Added visual indicators (upload and delete badges) to YOLO-detected image placeholders. This provides users with clear, actionable feedback that they can tap on the bounding box to import or remove an image, addressing the lack of visual cues on placeholders.
Details
ComputerVisionActivityinto a newDetectionVisualizerclass.ic_placeholder_uploadandic_placeholder_delete) and integrated them as badges on image placeholders.XmlFileManagerclass.ComputerVisionViewModelandDrawableImportHelperwith robust file deletion logic to cleanly remove imported images from the project.document_5186007705218713965.mp4
Ticket
ADFA-3972
Observation
Refactoring the
ComputerVisionActivityto delegate visualization (DetectionVisualizer) and file saving (XmlFileManager) significantly reduces its bloat and adheres closer to the Single Responsibility Principle, making future UI/canvas modifications much easier to manage.