feat: support multi resolution#37
Conversation
📝 WalkthroughWalkthroughAdds dimension and text-size scaling tied to VideoAspectRatio, exposes top-level font-size properties, adds PopUpTextView with per-word animation, replaces lyrics text wiring to use the new view and fonts, copies project name to clipboard on share, adjusts effect/view frame handling, and refactors MotionVideoPlayer to use an overlay, time display, and time-based playback loop. ChangesCore Dimension & Text Scaling Utilities
Pop-up Text Rendering
Motion Video Player UI & Playback
Motion Effects & MotionView Lifecycle
Clipboard & Share Flow, Imports Cleanup
Sequence Diagram(s)sequenceDiagram
participant User as User
participant Player as MotionVideoPlayer
participant Timer as TimerLoop
participant Composer as MotionComposerView
participant Seek as SeekBar
participant Overlay as OverlayImageView
User->>Player: Tap Play
Player->>Timer: start(elapsedBase = now)
Timer-->>Player: tick(elapsed)
Player->>Composer: compute/render expectedFrame(elapsed)
Player->>Seek: updateProgress(expectedFrame)
Player->>Overlay: updateBitmap(expectedFrame)
User->>Seek: seek(pos)
Seek->>Player: onProgressChanged(pos, fromUser=true)
Player->>Composer: renderFrame(pos)
Player->>Overlay: refreshBitmap(pos)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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 docstrings
🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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. Review rate limit: 0/1 reviews remaining, refill in 60 minutes.Comment |
![amazon-q-developer[bot]](https://avatars.githubusercontent.com/in/1220912?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary
This PR successfully implements multi-resolution support for video production by introducing a scalable dimension and text size provider system. The changes enable videos to adapt to different aspect ratios (480p, 720p, etc.) by dynamically scaling dimensions and font sizes.
Key Changes
- New Utilities: Added
MotionDimensionProviderandMotionTextSizeProviderfor automatic dimension and text size scaling - Multi-Resolution Support: Updated video producers to use
VideoAspectRatio.Ratio9x16_720and dynamic font sizing withfontSizeH3 - Code Cleanup: Removed unused imports and replaced hardcoded values with scaled alternatives
Critical Issue (1)
- Crash Risk: Empty lyrics list validation is missing in both
getLyricsVideoProducerandgetMultiLyricsVideoProducerfunctions. Calling.first()or.last()on an empty list will throw NoSuchElementException
Recommendation
Address the empty list validation issue before merging to prevent potential runtime crashes when processing projects with empty or missing lyrics metadata.
You can now have the agent implement changes and create commits directly on your pull request's source branch. Simply comment with /q followed by your request in natural language to ask the agent to make changes.
| val motionConfig = | ||
| MotionConfig( | ||
| aspectRatio = VideoAspectRatio.Ratio9x16_480, | ||
| aspectRatio = VideoAspectRatio.Ratio9x16_720, |
There was a problem hiding this comment.
🛑 Crash Risk: Both getLyricsVideoProducer and getMultiLyricsVideoProducer functions call .first() and .last() on the lyrics list without validation. If the metadata contains an empty lyrics array, this will throw NoSuchElementException. Add empty list checks before using these functions.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a dynamic scaling system for dimensions and text sizes based on video aspect ratio, including new providers and utility extensions. It also adds a clipboard copy feature during project sharing and updates the video player with an overlay preview. Feedback highlights a critical performance issue in MotionVideoPlayer due to frequent bitmap rendering in the progress listener, incorrect unit handling for scaled font sizes which should use pixels instead of SP, and a UX concern regarding the automatic overwriting of the system clipboard.
| overlayImageView.setImageBitmap( | ||
| motionVideoProducer | ||
| .motionComposerView | ||
| .getViewBitmap(), | ||
| ) |
There was a problem hiding this comment.
Calling getViewBitmap() inside onProgressChanged is a critical performance issue. This method is triggered for every frame during playback (24+ FPS), and getViewBitmap() performs a full view-to-bitmap render (including allocations and drawing). This will cause severe UI stuttering and high memory pressure. Since motionComposerView is already in the view hierarchy, it should render itself. If an overlay is required for scaling, consider updating it only during manual seeking (if (fromUser)).
| textView = | ||
| AppCompatTextView(applicationContext).apply { | ||
| textSize = 18f | ||
| textSize = fontSizeH3 |
There was a problem hiding this comment.
The textSize property in Android defaults to SP (Scale-independent Pixels). Since fontSizeH3 is calculated in pixels relative to the video resolution, setting it directly will cause the text to be scaled incorrectly by the device's display density. Use setTextSize(TypedValue.COMPLEX_UNIT_PX, ...) to ensure the text size is consistent with the video frame.
| textSize = fontSizeH3 | |
| setTextSize(android.util.TypedValue.COMPLEX_UNIT_PX, fontSizeH3) |
| tvSongName.textSize = fontSizeH5 | ||
| tvLyricsLine1.textSize = fontSizeH3 | ||
| tvLyricsLine2.textSize = fontSizeH3 |
There was a problem hiding this comment.
These text sizes are calculated in pixels relative to the video dimensions. Setting them via the textSize property treats them as SP, which will result in incorrect rendering on devices with different display densities. They should be applied using TypedValue.COMPLEX_UNIT_PX.
| tvSongName.textSize = fontSizeH5 | |
| tvLyricsLine1.textSize = fontSizeH3 | |
| tvLyricsLine2.textSize = fontSizeH3 | |
| tvSongName.setTextSize(android.util.TypedValue.COMPLEX_UNIT_PX, fontSizeH5) | |
| tvLyricsLine1.setTextSize(android.util.TypedValue.COMPLEX_UNIT_PX, fontSizeH3) | |
| tvLyricsLine2.setTextSize(android.util.TypedValue.COMPLEX_UNIT_PX, fontSizeH3) |
| * Share Project | ||
| */ | ||
| private fun shareProjectFile(motionProject: MotionProject) { | ||
| copyProjectNameToClipboard(motionProject) |
There was a problem hiding this comment.
Automatically overwriting the user's clipboard when they click 'Share' is unexpected behavior and can lead to data loss if the user had something important in their clipboard. It is generally better to provide an explicit 'Copy' button or include the name in the share intent's text extra.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
modules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/activity/SearchActivity.kt (1)
134-153:⚠️ Potential issue | 🟡 MinorUnexpected side effect: sharing silently overwrites the user's clipboard.
Triggering
copyProjectNameToClipboardfrom insideshareProjectFilecouples two unrelated UX actions. When the user taps "share" they will see the system share-chooser plus a "Project name copied to clipboard" toast, and their existing clipboard contents are silently replaced. That's surprising behavior and is generally considered an anti-pattern — clipboard writes should be the result of an explicit user gesture (e.g. a long-press or a dedicated "Copy name" menu item).If the goal is to make the project name easy to paste into the receiving app, prefer adding it to the share intent itself instead of mutating the clipboard:
🛠️ Proposed change
private fun shareProjectFile(motionProject: MotionProject) { - copyProjectNameToClipboard(motionProject) - val videoFile = createProjectFile(motionProject) val videoFileUri: Uri = FileProvider.getUriForFile( this, "${this.packageName}.fileprovider", videoFile, ) val intent = Intent(Intent.ACTION_SEND) intent.setDataAndType( videoFileUri, URLConnection.guessContentTypeFromName(videoFile.name), ) intent.addFlags(Intent.FLAG_GRANT_READ_URI_PERMISSION or Intent.FLAG_ACTIVITY_NEW_TASK) intent.putExtra(Intent.EXTRA_STREAM, videoFileUri) + intent.putExtra(Intent.EXTRA_SUBJECT, motionProject.name) + intent.putExtra(Intent.EXTRA_TEXT, motionProject.name) startActivity(Intent.createChooser(intent, "Share Video")) }If the clipboard copy is intentional product behavior, please confirm — and consider gating it behind an explicit UI affordance rather than the share path.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@modules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/activity/SearchActivity.kt` around lines 134 - 153, Remove the unexpected clipboard side-effect from shareProjectFile by deleting the call to copyProjectNameToClipboard(motionProject) and instead include the project name in the share intent (e.g., via Intent.EXTRA_TEXT) so the receiving app can access it; update shareProjectFile to add the project's name or description from the motionProject into the intent (use Intent.EXTRA_TEXT and keep Intent.EXTRA_STREAM for the file), and leave copyProjectNameToClipboard defined for explicit copy actions only (do not invoke it from shareProjectFile).
🧹 Nitpick comments (1)
modules/core/src/main/java/com/tejpratapsingh/motionlib/core/MotionDimensionProvider.kt (1)
23-48: Misleadingdpnaming — values are pixel-base, not density-independent.
getScaledValue,VideoAspectRatio.scale, and the top-levelscaledDp(...)perform a purely multiplicative pixel-to-pixel scaling againstreferenceSize = 1080f(which is itself a pixel count — the smaller dimension of Full-HD). The return docstring even says "The scaled value in pixels."Naming the input
dpValueand the helperscaledDp(...)strongly implies these accept Android density-independent pixels, which they do not. A future caller passing an actual DP value (e.g., fromdimenResource) would get silently incorrect output. Consider renaming to make the unit unambiguous.♻️ Suggested rename
- fun getScaledValue( - aspectRatio: VideoAspectRatio, - dpValue: Float, - referenceSize: Float = 1080f, - ): Float { - val currentSize = min(aspectRatio.width, aspectRatio.height).toFloat() - val scale = currentSize / referenceSize - return dpValue * scale - } + fun getScaledValue( + aspectRatio: VideoAspectRatio, + basePx: Float, + referencePx: Float = 1080f, + ): Float { + val currentSize = min(aspectRatio.width, aspectRatio.height).toFloat() + val scale = currentSize / referencePx + return basePx * scale + } } /** * Extension to scale a value based on the [VideoAspectRatio]. */ fun VideoAspectRatio.scale( - dpValue: Float, - reference: Float = 1080f, -): Float = MotionDimensionProvider.getScaledValue(this, dpValue, reference) + basePx: Float, + referencePx: Float = 1080f, +): Float = MotionDimensionProvider.getScaledValue(this, basePx, referencePx) /** - * Convenience function to get a scaled value using the current [MotionConfig]'s aspect ratio. + * Convenience function to scale a base pixel value using the current [MotionConfig]'s aspect ratio. */ -fun scaledDp( - value: Float, - reference: Float = 1080f, -): Float = provideCurrentConfig().aspectRatio.scale(value, reference) +fun scaledPx( + basePx: Float, + referencePx: Float = 1080f, +): Float = provideCurrentConfig().aspectRatio.scale(basePx, referencePx)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@modules/core/src/main/java/com/tejpratapsingh/motionlib/core/MotionDimensionProvider.kt` around lines 23 - 48, The functions use pixel-based scaling but use "dp" in names/params which is misleading; rename parameters and functions to reflect pixels (e.g., change dpValue → pxValue in MotionDimensionProvider.getScaledValue, VideoAspectRatio.scale, and the top-level scaledDp → scaledPx, and rename referenceSize → referencePx/reference), update their KDocs to say "pixels" instead of "dp", and update all call sites to the new names so callers won't confuse density-independent pixels with raw pixels.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@modules/core/src/main/java/com/tejpratapsingh/motionlib/core/MotionTextSizeProvider.kt`:
- Around line 62-77: getFontSize currently returns a flat 32f when
getTextSizes(aspectRatio) is null, collapsing the typographic hierarchy for
VideoAspectRatio.Custom; change the fallback to be variant-aware by computing
per-variant sizes instead of a single constant. Update getFontSize to call
getTextSizes(aspectRatio) ?: computeVariantFallback(variant, aspectRatio) (or
inline the logic) where computeVariantFallback uses
VideoAspectRatio.Custom.width/height to scale the same way the named ratios do
(or returns distinct defaults per MotionTextVariant like h1,h2,...,p), ensuring
each MotionTextVariant (H1..P) gets its own appropriate size rather than always
32f; reference getFontSize, getTextSizes, VideoAspectRatio.Custom and
MotionTextVariant when making the change.
In
`@modules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/motion/MultiLyricsVideoProducer.kt`:
- Line 84: The assigned fontSizeH3 is in pixels (computed by
MotionTextSizeProvider.getFontSize / MotionDimensionProvider.getScaledValue) but
textSize was set via AppCompatTextView.textSize which treats the value as SP;
change the assignment to call the TextView setTextSize overload that accepts a
unit and value (use COMPLEX_UNIT_PX) so the pixel value is applied verbatim
(i.e., replace the direct textSize = fontSizeH3 with a call that sets
TypedValue.COMPLEX_UNIT_PX and fontSizeH3). Also consider routing raw padding
constants through the same aspect-ratio scale(...) to keep all layout values
resolution-aware.
In
`@modules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/view/LyricsContainer.kt`:
- Around line 64-66: The TextView sizes are being set with pixel values
(fontSizeH5/fontSizeH3 from MotionTextSizeProvider) but TextView.textSize
interprets values as SP; update the three assignments (tvSongName,
tvLyricsLine1, tvLyricsLine2) to set sizes in pixels using
TextView.setTextSize(COMPLEX_UNIT_PX, value) or call a new helper (e.g.,
TextView.setMotionTextSize(variant) in motionlib.core) that applies
COMPLEX_UNIT_PX, so the bitmap rendered by toBitmap() uses the intended pixel
sizes tied to VideoAspectRatio rather than device SP scaling.
In
`@modules/motion-store/src/main/java/com/tejpratapsingh/motionstore/extensions/ContextExtensions.kt`:
- Around line 17-22: In
copyProjectNameToClipboard(Context.copyProjectNameToClipboard), stop showing the
Toast on Android 13+ by checking Build.VERSION.SDK_INT >=
Build.VERSION_CODES.TIRAMISU (suppress the Toast when true) and move both
hardcoded strings into resources (e.g., R.string.project_name_label for the
ClipData label and R.string.project_copied_to_clipboard for the confirmation
text), then use getString(...) for the label and toast text; ensure Build is
imported and only call Toast.makeText(...) when the API check indicates pre-33.
In
`@modules/motionlib/src/main/java/com/tejpratapsingh/motionlib/ui/MotionVideoPlayer.kt`:
- Around line 96-99: The ImageView for the preview uses
ImageView.ScaleType.FIT_XY which will non-uniformly stretch bitmaps when the
produced bitmap aspect ratio differs from the on-screen preview; update the
overlayImageView setup (the ImageView created in MotionVideoPlayer.kt) to use a
scale type that preserves aspect ratio (e.g., ImageView.ScaleType.FIT_CENTER or
CENTER_CROP) or adjust previewLayout to respect the configured VideoAspectRatio
so previews match exported video; locate the overlayImageView initialization and
replace FIT_XY with the chosen scale type (or add code to constrain
previewLayout to VideoAspectRatio) to avoid distortion.
- Around line 57-61: onProgressChanged currently always updates
overlayImageView.setImageBitmap which captures the previous frame during
playback because startPlayback sets seekBar.progress before advancing state with
motionVideoProducer.motionComposerView.forFrame; fix by ensuring the frame is
advanced before capturing the bitmap: either (A) in startPlayback call
motionVideoProducer.motionComposerView.forFrame(currentProgress) before
assigning seekBar.progress, or (B) move the overlayImageView.setImageBitmap
refresh out of onProgressChanged into startPlayback and invoke it immediately
after motionVideoProducer.motionComposerView.forFrame(currentProgress);
additionally, if you keep the onProgressChanged refresh, gate it with the
fromUser flag so it only updates when the user moves the seek bar.
- Around line 57-61: The onProgressChanged handler in MotionVideoPlayer.kt
currently calls
overlayImageView.setImageBitmap(motionVideoProducer.motionComposerView.getViewBitmap())
on every frame which allocates a new Bitmap each time and causes UI jank/OOM;
change this to (1) avoid per-frame bitmap creation by only updating the overlay
during scrubs (handle updates in onStopTrackingTouch or debounce
onProgressChanged) and (2) reuse or recycle bitmaps: add a
drawIntoBitmap(Bitmap) API or reuse a single backing Bitmap from
MotionComposerView instead of getViewBitmap(), and before calling
setImageBitmap() explicitly recycle the previous bitmap (retrieve the current
drawable's bitmap, ensure it isn’t same instance, then recycle) to prevent
leaks; update references: MotionVideoPlayer.onProgressChanged,
MotionVideoPlayer.seekBar progress loop,
MotionComposerView.getViewBitmap/drawIntoBitmap, and
overlayImageView.setImageBitmap.
---
Outside diff comments:
In
`@modules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/activity/SearchActivity.kt`:
- Around line 134-153: Remove the unexpected clipboard side-effect from
shareProjectFile by deleting the call to
copyProjectNameToClipboard(motionProject) and instead include the project name
in the share intent (e.g., via Intent.EXTRA_TEXT) so the receiving app can
access it; update shareProjectFile to add the project's name or description from
the motionProject into the intent (use Intent.EXTRA_TEXT and keep
Intent.EXTRA_STREAM for the file), and leave copyProjectNameToClipboard defined
for explicit copy actions only (do not invoke it from shareProjectFile).
---
Nitpick comments:
In
`@modules/core/src/main/java/com/tejpratapsingh/motionlib/core/MotionDimensionProvider.kt`:
- Around line 23-48: The functions use pixel-based scaling but use "dp" in
names/params which is misleading; rename parameters and functions to reflect
pixels (e.g., change dpValue → pxValue in
MotionDimensionProvider.getScaledValue, VideoAspectRatio.scale, and the
top-level scaledDp → scaledPx, and rename referenceSize →
referencePx/reference), update their KDocs to say "pixels" instead of "dp", and
update all call sites to the new names so callers won't confuse
density-independent pixels with raw pixels.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 24a64e12-cb3a-4330-aeaa-684ea65a3809
📒 Files selected for processing (10)
modules/core/src/main/java/com/tejpratapsingh/motionlib/core/MotionDimensionProvider.ktmodules/core/src/main/java/com/tejpratapsingh/motionlib/core/MotionTextSizeProvider.ktmodules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/activity/SearchActivity.ktmodules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/compose/ProjectDetailsCompose.ktmodules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/motion/LyricsVideoProducer.ktmodules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/motion/MultiLyricsVideoProducer.ktmodules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/view/LyricsContainer.ktmodules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/view/LyricsTextView.ktmodules/motion-store/src/main/java/com/tejpratapsingh/motionstore/extensions/ContextExtensions.ktmodules/motionlib/src/main/java/com/tejpratapsingh/motionlib/ui/MotionVideoPlayer.kt
💤 Files with no reviewable changes (3)
- modules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/compose/ProjectDetailsCompose.kt
- modules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/view/LyricsTextView.kt
- modules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/motion/LyricsVideoProducer.kt
| internal fun getFontSize( | ||
| aspectRatio: VideoAspectRatio, | ||
| variant: MotionTextVariant, | ||
| ): Float { | ||
| val sizes = getTextSizes(aspectRatio) ?: return 32f | ||
|
|
||
| return when (variant) { | ||
| MotionTextVariant.H1 -> sizes.h1 | ||
| MotionTextVariant.H2 -> sizes.h2 | ||
| MotionTextVariant.H3 -> sizes.h3 | ||
| MotionTextVariant.H4 -> sizes.h4 | ||
| MotionTextVariant.H5 -> sizes.h5 | ||
| MotionTextVariant.H6 -> sizes.h6 | ||
| MotionTextVariant.P -> sizes.p | ||
| } | ||
| } |
There was a problem hiding this comment.
Fallback for Custom collapses the typographic hierarchy.
When the configured aspect ratio is VideoAspectRatio.Custom, every variant (H1 … P) returns the same 32f. Callers requesting fontSizeH1 for a Custom resolution will silently render at the same size as fontSizeP, which is almost certainly not the intended behavior.
VideoAspectRatio.Custom already exposes width/height, so one option is to scale Custom the same way as the named ratios; if you specifically want a non-scaled fallback, at least key it off the variant so the hierarchy is preserved.
🐛 Suggested fix — variant-aware fallback
internal fun getFontSize(
aspectRatio: VideoAspectRatio,
variant: MotionTextVariant,
): Float {
- val sizes = getTextSizes(aspectRatio) ?: return 32f
-
- return when (variant) {
- MotionTextVariant.H1 -> sizes.h1
- MotionTextVariant.H2 -> sizes.h2
- MotionTextVariant.H3 -> sizes.h3
- MotionTextVariant.H4 -> sizes.h4
- MotionTextVariant.H5 -> sizes.h5
- MotionTextVariant.H6 -> sizes.h6
- MotionTextVariant.P -> sizes.p
+ val sizes = getTextSizes(aspectRatio)
+ return when (variant) {
+ MotionTextVariant.H1 -> sizes?.h1 ?: 96f
+ MotionTextVariant.H2 -> sizes?.h2 ?: 72f
+ MotionTextVariant.H3 -> sizes?.h3 ?: 48f
+ MotionTextVariant.H4 -> sizes?.h4 ?: 36f
+ MotionTextVariant.H5 -> sizes?.h5 ?: 24f
+ MotionTextVariant.H6 -> sizes?.h6 ?: 20f
+ MotionTextVariant.P -> sizes?.p ?: 32f
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| internal fun getFontSize( | |
| aspectRatio: VideoAspectRatio, | |
| variant: MotionTextVariant, | |
| ): Float { | |
| val sizes = getTextSizes(aspectRatio) ?: return 32f | |
| return when (variant) { | |
| MotionTextVariant.H1 -> sizes.h1 | |
| MotionTextVariant.H2 -> sizes.h2 | |
| MotionTextVariant.H3 -> sizes.h3 | |
| MotionTextVariant.H4 -> sizes.h4 | |
| MotionTextVariant.H5 -> sizes.h5 | |
| MotionTextVariant.H6 -> sizes.h6 | |
| MotionTextVariant.P -> sizes.p | |
| } | |
| } | |
| internal fun getFontSize( | |
| aspectRatio: VideoAspectRatio, | |
| variant: MotionTextVariant, | |
| ): Float { | |
| val sizes = getTextSizes(aspectRatio) | |
| return when (variant) { | |
| MotionTextVariant.H1 -> sizes?.h1 ?: 96f | |
| MotionTextVariant.H2 -> sizes?.h2 ?: 72f | |
| MotionTextVariant.H3 -> sizes?.h3 ?: 48f | |
| MotionTextVariant.H4 -> sizes?.h4 ?: 36f | |
| MotionTextVariant.H5 -> sizes?.h5 ?: 24f | |
| MotionTextVariant.H6 -> sizes?.h6 ?: 20f | |
| MotionTextVariant.P -> sizes?.p ?: 32f | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@modules/core/src/main/java/com/tejpratapsingh/motionlib/core/MotionTextSizeProvider.kt`
around lines 62 - 77, getFontSize currently returns a flat 32f when
getTextSizes(aspectRatio) is null, collapsing the typographic hierarchy for
VideoAspectRatio.Custom; change the fallback to be variant-aware by computing
per-variant sizes instead of a single constant. Update getFontSize to call
getTextSizes(aspectRatio) ?: computeVariantFallback(variant, aspectRatio) (or
inline the logic) where computeVariantFallback uses
VideoAspectRatio.Custom.width/height to scale the same way the named ratios do
(or returns distinct defaults per MotionTextVariant like h1,h2,...,p), ensuring
each MotionTextVariant (H1..P) gets its own appropriate size rather than always
32f; reference getFontSize, getTextSizes, VideoAspectRatio.Custom and
MotionTextVariant when making the change.
| textView = | ||
| AppCompatTextView(applicationContext).apply { | ||
| textSize = 18f | ||
| textSize = fontSizeH3 |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
What unit does Android TextView.setTextSize(Float) use by default, and how does it differ from setTextSize(TypedValue.COMPLEX_UNIT_PX, Float)?
💡 Result:
Android TextView.setTextSize(float size) uses scaled pixels (SP, TypedValue.COMPLEX_UNIT_SP) by default. The float value is interpreted as SP units, which are scaled by display density and user font size preferences. TextView.setTextSize(TypedValue.COMPLEX_UNIT_PX, float size) explicitly uses pixels (PX). The float value is treated as exact pixels, without scaling for density or user font preferences. The key difference: SP scales with user accessibility font size settings and density (sp = px * scaledDensity), while PX does not scale with user font size (only density-adjusted dp does, but PX is raw pixels). Using setTextSize(float) on a value from getTextSize (which returns PX) causes double-scaling if not using PX explicitly.
Citations:
- 1: https://stackoverflow.com/questions/3687065/textview-settextsize-behaves-abnormally-how-to-set-text-size-of-textview-dynam
- 2: https://stackoverflow.com/questions/6998938/textview-setting-the-text-size-programmatically-doesnt-seem-to-work
- 3: https://stackoverflow.com/questions/5032355/android-textview-settextsize-incorrectly-increases-text-size
- 4: http://docs.52im.net/extend/docs/api/android-50/reference/android/widget/TextView.html
🏁 Script executed:
find . -name "MultiLyricsVideoProducer.kt" -type fRepository: tejpratap46/AndroidVideoMotion
Length of output: 190
🏁 Script executed:
find . -name "MotionTextSizeProvider.kt" -type fRepository: tejpratap46/AndroidVideoMotion
Length of output: 163
🏁 Script executed:
find . -name "MotionDimensionProvider.kt" -type fRepository: tejpratap46/AndroidVideoMotion
Length of output: 164
🏁 Script executed:
cat -n ./modules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/motion/MultiLyricsVideoProducer.kt | sed -n '75,95p'Repository: tejpratap46/AndroidVideoMotion
Length of output: 889
🏁 Script executed:
cat -n ./modules/core/src/main/java/com/tejpratapsingh/motionlib/core/MotionTextSizeProvider.ktRepository: tejpratap46/AndroidVideoMotion
Length of output: 4097
🏁 Script executed:
cat -n ./modules/core/src/main/java/com/tejpratapsingh/motionlib/core/MotionDimensionProvider.ktRepository: tejpratap46/AndroidVideoMotion
Length of output: 2045
Unit mismatch: fontSizeH3 is in pixels, but textSize = … is interpreted as SP.
fontSizeH3 is computed by MotionTextSizeProvider.getFontSize() which uses aspectRatio.scale(48f). The scale() function returns a value explicitly documented as "in pixels" (see MotionDimensionProvider.getScaledValue). However, AppCompatTextView.textSize = X invokes setTextSize(Float) which defaults to TypedValue.COMPLEX_UNIT_SP, so Android multiplies by the device's density × fontScale at draw time.
For video rendering this is a real correctness problem: the text size in the produced frames will vary with the host device's display density and the user's accessibility font-scale setting, defeating the whole point of the new resolution-aware sizing. Apply the value explicitly as pixels.
Suggested fix
- textSize = fontSizeH3
+ setTextSize(android.util.TypedValue.COMPLEX_UNIT_PX, fontSizeH3)Note: padding values (16, 16, 16, 16) are also raw pixels and don't scale with aspectRatio — once the px/sp issue is fixed, consider routing them through scale(...) for consistency with the new resolution-aware sizing.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@modules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/motion/MultiLyricsVideoProducer.kt`
at line 84, The assigned fontSizeH3 is in pixels (computed by
MotionTextSizeProvider.getFontSize / MotionDimensionProvider.getScaledValue) but
textSize was set via AppCompatTextView.textSize which treats the value as SP;
change the assignment to call the TextView setTextSize overload that accepts a
unit and value (use COMPLEX_UNIT_PX) so the pixel value is applied verbatim
(i.e., replace the direct textSize = fontSizeH3 with a call that sets
TypedValue.COMPLEX_UNIT_PX and fontSizeH3). Also consider routing raw padding
constants through the same aspect-ratio scale(...) to keep all layout values
resolution-aware.
| tvSongName.textSize = fontSizeH5 | ||
| tvLyricsLine1.textSize = fontSizeH3 | ||
| tvLyricsLine2.textSize = fontSizeH3 |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -name "LyricsContainer.kt" -type fRepository: tejpratap46/AndroidVideoMotion
Length of output: 179
🏁 Script executed:
wc -l ./modules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/view/LyricsContainer.ktRepository: tejpratap46/AndroidVideoMotion
Length of output: 183
🏁 Script executed:
cat -n ./modules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/view/LyricsContainer.ktRepository: tejpratap46/AndroidVideoMotion
Length of output: 5950
🏁 Script executed:
find . -name "*MotionTextSizeProvider*" -type fRepository: tejpratap46/AndroidVideoMotion
Length of output: 163
🏁 Script executed:
cat -n ./modules/core/src/main/java/com/tejpratapsingh/motionlib/core/MotionTextSizeProvider.ktRepository: tejpratap46/AndroidVideoMotion
Length of output: 4097
🏁 Script executed:
find . -name "MultiLyricsVideoProducer*" -type fRepository: tejpratap46/AndroidVideoMotion
Length of output: 190
🏁 Script executed:
wc -l ./modules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/motion/MultiLyricsVideoProducer.ktRepository: tejpratap46/AndroidVideoMotion
Length of output: 193
🏁 Script executed:
cat -n ./modules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/motion/MultiLyricsVideoProducer.ktRepository: tejpratap46/AndroidVideoMotion
Length of output: 4081
🏁 Script executed:
rg "\.textSize\s*=" --type kt -A 1 -B 1Repository: tejpratap46/AndroidVideoMotion
Length of output: 101
🏁 Script executed:
rg "\.textSize\s*=" -A 1 -B 1 --type-list | grep -i kotlinRepository: tejpratap46/AndroidVideoMotion
Length of output: 464
🏁 Script executed:
rg "\.textSize\s*=" -A 1 -B 1Repository: tejpratap46/AndroidVideoMotion
Length of output: 1484
🏁 Script executed:
rg "setTextSize|COMPLEX_UNIT" -A 1 -B 1Repository: tejpratap46/AndroidVideoMotion
Length of output: 442
Same SP/PX unit mismatch — fontSizeH3/fontSizeH5 are pixel values.
fontSizeH5 and fontSizeH3 from MotionTextSizeProvider are scaled pixel values based on the video aspect ratio, but TextView.textSize = X uses COMPLEX_UNIT_SP by default and applies density × fontScale at render time. Since this view is rendered to a bitmap via toBitmap() (line 135) for video output, the resulting frame will contain text whose size depends on the host device's display density and the user's font-scale preference — not on the configured VideoAspectRatio.
Suggested fix
- tvSongName.textSize = fontSizeH5
- tvLyricsLine1.textSize = fontSizeH3
- tvLyricsLine2.textSize = fontSizeH3
+ tvSongName.setTextSize(android.util.TypedValue.COMPLEX_UNIT_PX, fontSizeH5)
+ tvLyricsLine1.setTextSize(android.util.TypedValue.COMPLEX_UNIT_PX, fontSizeH3)
+ tvLyricsLine2.setTextSize(android.util.TypedValue.COMPLEX_UNIT_PX, fontSizeH3)Recommend either (a) standardizing all motion library TextView consumers on COMPLEX_UNIT_PX at the call sites, or (b) wrapping the size in a small helper (e.g., TextView.setMotionTextSize(variant)) inside motionlib.core so this can't be misapplied as SP elsewhere.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@modules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/view/LyricsContainer.kt`
around lines 64 - 66, The TextView sizes are being set with pixel values
(fontSizeH5/fontSizeH3 from MotionTextSizeProvider) but TextView.textSize
interprets values as SP; update the three assignments (tvSongName,
tvLyricsLine1, tvLyricsLine2) to set sizes in pixels using
TextView.setTextSize(COMPLEX_UNIT_PX, value) or call a new helper (e.g.,
TextView.setMotionTextSize(variant) in motionlib.core) that applies
COMPLEX_UNIT_PX, so the bitmap rendered by toBitmap() uses the intended pixel
sizes tied to VideoAspectRatio rather than device SP scaling.
| fun Context.copyProjectNameToClipboard(motionProject: MotionProject) { | ||
| val clipboard = getSystemService(Context.CLIPBOARD_SERVICE) as ClipboardManager | ||
| val clip = ClipData.newPlainText("Project Name", motionProject.name) | ||
| clipboard.setPrimaryClip(clip) | ||
| Toast.makeText(this, "Project name copied to clipboard", Toast.LENGTH_SHORT).show() | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Android 13 API 33 system clipboard copy confirmation toast duplicate avoid
💡 Result:
In Android 13 (API level 33) and higher, the system automatically displays a standard visual confirmation (toast-like UI) at the bottom of the screen whenever content is copied to the clipboard via ClipboardManager.setPrimaryClip. This provides users with feedback that content was successfully copied and a preview of the content. This system UI appears alongside any custom Toast or Snackbar your app shows after copying, leading to duplicate notifications (e.g., your "Copied!" toast + system's "Copied to clipboard" UI). To avoid duplicates: - Do not show custom Toast/Snackbar feedback after copying on Android 13+. - Only show custom feedback on Android 12L (API 32) and lower. Example Kotlin code: fun copyToClipboard(context: Context, text: String, label: String = "label") { val clipboard = context.getSystemService(Context.CLIPBOARD_SERVICE) as ClipboardManager val clip = ClipData.newPlainText(label, text) clipboard.setPrimaryClip(clip) // Only show custom toast on pre-Android 13 if (Build.VERSION.SDK_INT <= Build.VERSION_CODES.S_V2) { // S_V2 is Android 12L Toast.makeText(context, "Copied to clipboard", Toast.LENGTH_SHORT).show } } You cannot disable or override the system clipboard confirmation UI programmatically—it is a deliberate privacy/security feature. Multiple sources confirm it cannot be hidden from app code (e.g., GitHub issues, Stack Overflow). For sensitive content (e.g., passwords), flag it to hide the preview: val clipData = ClipData.newPlainText(label, text) clipData.description.extras = PersistableBundle.apply { putBoolean(ClipDescription.EXTRA_IS_SENSITIVE, true) } clipboard.setPrimaryClip(clipData) This still shows the confirmation but obfuscates the content preview.
Citations:
- 1: https://android-dot-devsite-v2-prod.appspot.com/guide/topics/text/copy-paste
- 2: https://android-dot-devsite-v2-prod.appspot.com/develop/ui/views/touch-and-input/copy-paste
- 3: https://github.com/TGX-Android/Telegram-X/issues/319
- 4: How to remove the default toast msg 'copied' react-native-clipboard/clipboard#245
- 5: https://stackoverflow.com/questions/76202222/how-to-disable-copied-to-clipboard-toast-after-copying-a-sentence-programmatic
- 6: https://stackoverflow.com/questions/74216531/how-to-override-or-disable-toast-when-copying-text-in-android-12-programmaticall
- 7: https://developer.android.com/privacy-and-security/risks/secure-clipboard-handling
Avoid duplicate clipboard confirmation on Android 13+ and localize user-facing strings.
Starting with Android 13 (API 33), the system displays its own visual confirmation whenever an app copies content to the clipboard. Showing a custom Toast on top of that creates duplicate feedback. The Android framework handles this automatically, and app-level confirmations should be suppressed on API 33+.
Additionally, both the ClipData label ("Project Name") and the toast message are hardcoded English strings. These should be extracted to strings.xml for proper localization and consistency with the app's language settings.
Suggested fix
fun Context.copyProjectNameToClipboard(motionProject: MotionProject) {
val clipboard = getSystemService(Context.CLIPBOARD_SERVICE) as ClipboardManager
- val clip = ClipData.newPlainText("Project Name", motionProject.name)
+ val clip = ClipData.newPlainText(getString(R.string.clipboard_label_project_name), motionProject.name)
clipboard.setPrimaryClip(clip)
- Toast.makeText(this, "Project name copied to clipboard", Toast.LENGTH_SHORT).show()
+ if (Build.VERSION.SDK_INT < Build.VERSION_CODES.TIRAMISU) {
+ Toast.makeText(this, getString(R.string.toast_project_name_copied), Toast.LENGTH_SHORT).show()
+ }
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@modules/motion-store/src/main/java/com/tejpratapsingh/motionstore/extensions/ContextExtensions.kt`
around lines 17 - 22, In
copyProjectNameToClipboard(Context.copyProjectNameToClipboard), stop showing the
Toast on Android 13+ by checking Build.VERSION.SDK_INT >=
Build.VERSION_CODES.TIRAMISU (suppress the Toast when true) and move both
hardcoded strings into resources (e.g., R.string.project_name_label for the
ClipData label and R.string.project_copied_to_clipboard for the confirmation
text), then use getString(...) for the label and toast text; ensure Build is
imported and only call Toast.makeText(...) when the API check indicates pre-33.
| val overlayImageView: ImageView = | ||
| ImageView(context).apply { | ||
| scaleType = ImageView.ScaleType.FIT_XY | ||
| } |
There was a problem hiding this comment.
FIT_XY will distort the preview when aspect ratios differ.
Given this PR adds multi-resolution support, the produced bitmap aspect ratio will frequently differ from previewLayout's on-screen aspect ratio. FIT_XY stretches non-uniformly, so the preview will not match the final exported video. Prefer FIT_CENTER (letterbox) or CENTER_CROP, or constrain previewLayout to the configured VideoAspectRatio.
🎨 Proposed change
val overlayImageView: ImageView =
ImageView(context).apply {
- scaleType = ImageView.ScaleType.FIT_XY
+ scaleType = ImageView.ScaleType.FIT_CENTER
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| val overlayImageView: ImageView = | |
| ImageView(context).apply { | |
| scaleType = ImageView.ScaleType.FIT_XY | |
| } | |
| val overlayImageView: ImageView = | |
| ImageView(context).apply { | |
| scaleType = ImageView.ScaleType.FIT_CENTER | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@modules/motionlib/src/main/java/com/tejpratapsingh/motionlib/ui/MotionVideoPlayer.kt`
around lines 96 - 99, The ImageView for the preview uses
ImageView.ScaleType.FIT_XY which will non-uniformly stretch bitmaps when the
produced bitmap aspect ratio differs from the on-screen preview; update the
overlayImageView setup (the ImageView created in MotionVideoPlayer.kt) to use a
scale type that preserves aspect ratio (e.g., ImageView.ScaleType.FIT_CENTER or
CENTER_CROP) or adjust previewLayout to respect the configured VideoAspectRatio
so previews match exported video; locate the overlayImageView initialization and
replace FIT_XY with the chosen scale type (or add code to constrain
previewLayout to VideoAspectRatio) to avoid distortion.
tejpratap46
force-pushed
the
feat/support-multi-res
branch
3 times, most recently
from
6d930d3 to
b69dbf6
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (3)
modules/motion-store/src/main/java/com/tejpratapsingh/motionstore/extensions/ContextExtensions.kt (1)
17-22:⚠️ Potential issue | 🟡 MinorAvoid duplicate clipboard feedback on Android 13+ and localize these strings.
Line 21 always shows a custom toast after clipboard copy; on API 33+ the OS already shows clipboard confirmation UI. Also, Line 19 and Line 21 use hardcoded English strings.
Suggested fix
import android.content.ClipData import android.content.ClipboardManager import android.content.Context +import android.os.Build import android.widget.Toast +import com.tejpratapsingh.motionstore.R import com.tejpratapsingh.motionstore.tables.MotionProject fun Context.copyProjectNameToClipboard(motionProject: MotionProject) { val clipboard = getSystemService(Context.CLIPBOARD_SERVICE) as ClipboardManager - val clip = ClipData.newPlainText("Project Name", motionProject.name) + val clip = ClipData.newPlainText( + getString(R.string.clipboard_label_project_name), + motionProject.name, + ) clipboard.setPrimaryClip(clip) - Toast.makeText(this, "Project name copied to clipboard", Toast.LENGTH_SHORT).show() + if (Build.VERSION.SDK_INT < Build.VERSION_CODES.TIRAMISU) { + Toast.makeText(this, getString(R.string.toast_project_name_copied), Toast.LENGTH_SHORT).show() + } }Android Developers documentation: for Android 13 (API 33)+, does ClipboardManager.setPrimaryClip already trigger system clipboard confirmation UI, and should apps suppress custom copy Toasts to avoid duplicate feedback?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@modules/motion-store/src/main/java/com/tejpratapsingh/motionstore/extensions/ContextExtensions.kt` around lines 17 - 22, In copyProjectNameToClipboard(MotionProject) replace hardcoded English strings with localized resources (e.g., use a string resource for the clip label and a string resource like R.string.project_name_copied for the user message) and suppress the custom Toast on Android 13+ by checking Build.VERSION.SDK_INT >= Build.VERSION_CODES.TIRAMISU (only show the Toast when SDK < TIRAMISU); keep using getSystemService/ClipboardManager and ClipData.newPlainText to set the clip but change the label/value to use the localized string resource names mentioned above.modules/motionlib/src/main/java/com/tejpratapsingh/motionlib/ui/MotionVideoPlayer.kt (2)
112-115:⚠️ Potential issue | 🟡 MinorUse an aspect-preserving scale type for the overlay preview.
Line 114 uses
FIT_XY, which will stretch frames when preview and export aspect ratios differ. That can make preview output inaccurate for multi-resolution flows.🎨 Proposed change
val overlayImageView: ImageView = ImageView(context).apply { - scaleType = ImageView.ScaleType.FIT_XY + scaleType = ImageView.ScaleType.FIT_CENTER }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@modules/motionlib/src/main/java/com/tejpratapsingh/motionlib/ui/MotionVideoPlayer.kt` around lines 112 - 115, The overlay ImageView in MotionVideoPlayer (overlayImageView) currently uses ImageView.ScaleType.FIT_XY which stretches the preview; change it to an aspect-preserving scale type such as ImageView.ScaleType.FIT_CENTER (or CENTER_INSIDE) and enable adjustViewBounds = true on the same overlayImageView to preserve preview aspect ratio and avoid distortion when preview and export resolutions differ.
58-63:⚠️ Potential issue | 🟠 MajorAvoid per-frame bitmap capture in
onProgressChanged.On Line 59,
getViewBitmap()is invoked for every progress callback, including playback-driven updates. This keeps bitmap generation on the UI path every frame and is likely to cause jank/allocation churn at higher resolutions. Consider a reusable buffer render path (or throttled overlay refresh).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@modules/motionlib/src/main/java/com/tejpratapsingh/motionlib/ui/MotionVideoPlayer.kt` around lines 58 - 63, The code is capturing a new bitmap every progress update by calling motionVideoProducer.motionComposerView.getViewBitmap() inside the onProgressChanged path (affecting currentTimeTextView and overlayImageView); change this to avoid per-frame allocations by reusing a single mutable Bitmap/Canvas buffer or throttling updates: create and cache a reusable Bitmap for the overlay and have getViewBitmap write into that buffer (or only call getViewBitmap when the user is actively scrubbing or at a lower refresh rate), then call overlayImageView.setImageBitmap(reusableBitmap) only when the buffer is updated; ensure the reusable buffer and update logic are tied to motionComposerView and onProgressChanged so playback frames do not trigger full bitmap captures.
🧹 Nitpick comments (1)
modules/motionlib/src/main/java/com/tejpratapsingh/motionlib/ui/custom/text/PopUpTextView.kt (1)
60-62: Short-circuit once the base class hides the view.
BaseContourMotionView.forFrame()already marks this view invisible outside the active frame range, but this override still does the interpolation and text invalidation work afterward. An early return here avoids wasted work for every out-of-range frame.♻️ Proposed change
override fun forFrame(frame: Int): MotionView { super.forFrame(frame) + if (visibility != VISIBLE) { + return this + } Timber.d("start = $startFrame, end = $endFrame, frame = $frame")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@modules/motionlib/src/main/java/com/tejpratapsingh/motionlib/ui/custom/text/PopUpTextView.kt` around lines 60 - 62, The override PopUpTextView.forFrame calls super.forFrame(frame) which may mark the view invisible outside its active range; after that call check the view's visibility and return early (no interpolation, no invalidate) if the view is not visible — e.g., test visibility != View.VISIBLE or !isShown and return the MotionView immediately. Update the forFrame override in PopUpTextView to short-circuit after super.forFrame(frame) using the view visibility check to avoid doing layout/text interpolation work for out-of-range frames.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@modules/motionlib/src/main/java/com/tejpratapsingh/motionlib/ui/custom/text/PopUpTextView.kt`:
- Around line 136-141: The early return when progress <= 0f prevents
unwrittenTextAlpha from being applied; in the method in PopUpTextView (the block
using progress, unwrittenAlpha, and maxTranslationY) remove the immediate return
and instead handle the progress == 0f case by computing alpha = unwrittenAlpha *
255 and translationY = maxTranslationY (so the word is visible at reduced alpha
and fully translated), while keeping the existing computation for 0 < progress
<= 1; ensure the drawing logic uses these computed alpha and translationY values
so unwrittenTextAlpha takes effect before animation starts.
- Around line 73-101: The code currently recreates a new SpannableString and
PopUpSpan for each word on every frame (in the block using spannableString,
wordArray, PopUpSpan and setting textView.text), causing heavy allocation;
instead, initialize and cache the SpannableString, the PopUpSpan instances and
their start/end ranges once (e.g., in the PopUpTextView setup or whenever the
source text/wordArray changes), store them in a list of span descriptors, and in
forFrame() simply update each cached PopUpSpan's progress property and call
textView.invalidate() (do not recreate spans or reassign textView.text each
frame). Ensure you handle text changes by rebuilding the cached spans only when
text/wordArray changes.
In
`@modules/motionlib/src/main/java/com/tejpratapsingh/motionlib/ui/MotionVideoPlayer.kt`:
- Around line 220-249: At startup and on loop reset ensure you perform a full
frame step (render + audio priming + progress update) instead of only setting
seekBar.progress first; call
motionVideoProducer.motionComposerView.forFrame(frame), then
startAudioIfNeeded(frame, motionVideoProducer.motionAudio) to prime audio, then
set seekBar.progress = frame, and only after that update startTime and
startFrame and/or call stopAllAudio on loop as currently needed; apply this
sequence both where the loop initially starts and inside the else reset branch
so the first frame is fully rendered and audio-synced (refer to
startAudioIfNeeded, motionComposerView.forFrame, stopAllAudio, seekBar.progress,
startTime/startFrame).
---
Duplicate comments:
In
`@modules/motion-store/src/main/java/com/tejpratapsingh/motionstore/extensions/ContextExtensions.kt`:
- Around line 17-22: In copyProjectNameToClipboard(MotionProject) replace
hardcoded English strings with localized resources (e.g., use a string resource
for the clip label and a string resource like R.string.project_name_copied for
the user message) and suppress the custom Toast on Android 13+ by checking
Build.VERSION.SDK_INT >= Build.VERSION_CODES.TIRAMISU (only show the Toast when
SDK < TIRAMISU); keep using getSystemService/ClipboardManager and
ClipData.newPlainText to set the clip but change the label/value to use the
localized string resource names mentioned above.
In
`@modules/motionlib/src/main/java/com/tejpratapsingh/motionlib/ui/MotionVideoPlayer.kt`:
- Around line 112-115: The overlay ImageView in MotionVideoPlayer
(overlayImageView) currently uses ImageView.ScaleType.FIT_XY which stretches the
preview; change it to an aspect-preserving scale type such as
ImageView.ScaleType.FIT_CENTER (or CENTER_INSIDE) and enable adjustViewBounds =
true on the same overlayImageView to preserve preview aspect ratio and avoid
distortion when preview and export resolutions differ.
- Around line 58-63: The code is capturing a new bitmap every progress update by
calling motionVideoProducer.motionComposerView.getViewBitmap() inside the
onProgressChanged path (affecting currentTimeTextView and overlayImageView);
change this to avoid per-frame allocations by reusing a single mutable
Bitmap/Canvas buffer or throttling updates: create and cache a reusable Bitmap
for the overlay and have getViewBitmap write into that buffer (or only call
getViewBitmap when the user is actively scrubbing or at a lower refresh rate),
then call overlayImageView.setImageBitmap(reusableBitmap) only when the buffer
is updated; ensure the reusable buffer and update logic are tied to
motionComposerView and onProgressChanged so playback frames do not trigger full
bitmap captures.
---
Nitpick comments:
In
`@modules/motionlib/src/main/java/com/tejpratapsingh/motionlib/ui/custom/text/PopUpTextView.kt`:
- Around line 60-62: The override PopUpTextView.forFrame calls
super.forFrame(frame) which may mark the view invisible outside its active
range; after that call check the view's visibility and return early (no
interpolation, no invalidate) if the view is not visible — e.g., test visibility
!= View.VISIBLE or !isShown and return the MotionView immediately. Update the
forFrame override in PopUpTextView to short-circuit after super.forFrame(frame)
using the view visibility check to avoid doing layout/text interpolation work
for out-of-range frames.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: f55574cf-bf91-42f8-95d7-fcee9b960ccd
📒 Files selected for processing (11)
modules/core/src/main/java/com/tejpratapsingh/motionlib/core/MotionDimensionProvider.ktmodules/core/src/main/java/com/tejpratapsingh/motionlib/core/MotionTextSizeProvider.ktmodules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/activity/SearchActivity.ktmodules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/compose/ProjectDetailsCompose.ktmodules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/motion/LyricsVideoProducer.ktmodules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/motion/MultiLyricsVideoProducer.ktmodules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/view/LyricsContainer.ktmodules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/view/LyricsTextView.ktmodules/motion-store/src/main/java/com/tejpratapsingh/motionstore/extensions/ContextExtensions.ktmodules/motionlib/src/main/java/com/tejpratapsingh/motionlib/ui/MotionVideoPlayer.ktmodules/motionlib/src/main/java/com/tejpratapsingh/motionlib/ui/custom/text/PopUpTextView.kt
💤 Files with no reviewable changes (3)
- modules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/compose/ProjectDetailsCompose.kt
- modules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/motion/LyricsVideoProducer.kt
- modules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/view/LyricsTextView.kt
✅ Files skipped from review due to trivial changes (1)
- modules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/view/LyricsContainer.kt
🚧 Files skipped from review as they are similar to previous changes (3)
- modules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/motion/MultiLyricsVideoProducer.kt
- modules/core/src/main/java/com/tejpratapsingh/motionlib/core/MotionTextSizeProvider.kt
- modules/core/src/main/java/com/tejpratapsingh/motionlib/core/MotionDimensionProvider.kt
| val spannableString = SpannableString(text) | ||
|
|
||
| var currentIdx = 0 | ||
| wordArray.forEachIndexed { index, word -> | ||
| val wordProgress = (progress - index).coerceIn(0f, 1f) | ||
|
|
||
| val span = PopUpSpan( | ||
| progress = wordProgress, | ||
| unwrittenAlpha = unwrittenTextAlpha, | ||
| maxTranslationY = maxTranslationY, | ||
| ) | ||
|
|
||
| val start = currentIdx | ||
| val end = currentIdx + word.length | ||
|
|
||
| if ((start < end) && (end <= text.length)) { | ||
| spannableString.setSpan( | ||
| span, | ||
| start, | ||
| end, | ||
| Spannable.SPAN_EXCLUSIVE_EXCLUSIVE | ||
| ) | ||
| } | ||
|
|
||
| currentIdx += word.length + 1 // +1 for the space | ||
| } | ||
|
|
||
| textView.text = spannableString | ||
| textView.invalidate() |
There was a problem hiding this comment.
Avoid rebuilding the entire spannable on every frame.
This block allocates a new SpannableString plus one PopUpSpan per word on every forFrame() call, then reassigns textView.text. In a render loop that becomes O(words × frames) allocation churn and can noticeably slow playback/export for longer lyrics. Please cache the word ranges/spans once and only update span progress during forFrame().
🧰 Tools
🪛 GitHub Check: CodeFactor
[warning] 88-88: modules/motionlib/src/main/java/com/tejpratapsingh/motionlib/ui/custom/text/PopUpTextView.kt#L88
Parentheses in (start < end) are unnecessary and can be replaced with: start < end. (detekt.UnnecessaryParentheses)
[warning] 88-88: modules/motionlib/src/main/java/com/tejpratapsingh/motionlib/ui/custom/text/PopUpTextView.kt#L88
Parentheses in (end <= text.length) are unnecessary and can be replaced with: end <= text.length. (detekt.UnnecessaryParentheses)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@modules/motionlib/src/main/java/com/tejpratapsingh/motionlib/ui/custom/text/PopUpTextView.kt`
around lines 73 - 101, The code currently recreates a new SpannableString and
PopUpSpan for each word on every frame (in the block using spannableString,
wordArray, PopUpSpan and setting textView.text), causing heavy allocation;
instead, initialize and cache the SpannableString, the PopUpSpan instances and
their start/end ranges once (e.g., in the PopUpTextView setup or whenever the
source text/wordArray changes), store them in a list of span descriptors, and in
forFrame() simply update each cached PopUpSpan's progress property and call
textView.invalidate() (do not recreate spans or reassign textView.text each
frame). Ensure you handle text changes by rebuilding the cached spans only when
text/wordArray changes.
| if (progress <= 0f) { | ||
| return | ||
| } | ||
|
|
||
| val alpha = (unwrittenAlpha + (1f - unwrittenAlpha) * progress) * 255 | ||
| val translationY = maxTranslationY * (1f - progress) |
There was a problem hiding this comment.
unwrittenTextAlpha never applies before a word starts animating.
Because Lines 136-138 return immediately, words with progress == 0f are fully hidden. That changes behavior from WordWriterTextView, which kept the unwritten portion visible at reduced alpha, and makes unwrittenTextAlpha ineffective for the pre-animation state.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@modules/motionlib/src/main/java/com/tejpratapsingh/motionlib/ui/custom/text/PopUpTextView.kt`
around lines 136 - 141, The early return when progress <= 0f prevents
unwrittenTextAlpha from being applied; in the method in PopUpTextView (the block
using progress, unwrittenAlpha, and maxTranslationY) remove the immediate return
and instead handle the progress == 0f case by computing alpha = unwrittenAlpha *
255 and translationY = maxTranslationY (so the word is visible at reduced alpha
and fully translated), while keeping the existing computation for 0 < progress
<= 1; ensure the drawing logic uses these computed alpha and translationY values
so unwrittenTextAlpha takes effect before animation starts.
b69dbf6 to
d3bf43f
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@modules/motionlib/src/main/java/com/tejpratapsingh/motionlib/ui/custom/text/PopUpTextView.kt`:
- Around line 42-43: The word-splitting and indexing in PopUpTextView (variables
wordArray, wordCount and the code that advances current word using a hardcoded
+1) fails on multiple whitespace; change splitting from text.split(" ") to a
regex-based word boundary scan (e.g. use Pattern/Matcher with \s+ or \b) and
compute each word's exact start/end offsets by matching against the original
text so span ranges are derived from matcher.start()/matcher.end() rather than
assuming a single-space increment; replace the hardcoded +1 advancement with the
actual matched length or matcher.end() value so animations target the correct
substrings.
In
`@modules/motionlib/src/main/java/com/tejpratapsingh/motionlib/ui/effects/SlideRightToLeftEffect.kt`:
- Line 19: The early return in SlideRightToLeftEffect (when frame !in
startFrame..endFrame) causes motionView.translationX to retain stale values
after seeks/loops; modify the logic in the method that checks frame (the block
using startFrame, endFrame and motionView) to clamp/reset translationX when
outside the active range (e.g., set motionView.translationX = 0f or to its
default position) instead of simply returning, and apply the same fix for the
other out-of-range checks referenced around lines 33-37 so the view is always
placed correctly when the effect is not active.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 258b3d0b-255f-4723-a66b-56561cfaf848
📒 Files selected for processing (14)
modules/core/src/main/java/com/tejpratapsingh/motionlib/core/MotionDimensionProvider.ktmodules/core/src/main/java/com/tejpratapsingh/motionlib/core/MotionTextSizeProvider.ktmodules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/activity/SearchActivity.ktmodules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/compose/ProjectDetailsCompose.ktmodules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/motion/LyricsVideoProducer.ktmodules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/motion/MultiLyricsVideoProducer.ktmodules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/view/LyricsContainer.ktmodules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/view/LyricsTextView.ktmodules/motion-store/src/main/java/com/tejpratapsingh/motionstore/extensions/ContextExtensions.ktmodules/motionlib/src/main/java/com/tejpratapsingh/motionlib/core/infra/AndroidVideoGenerator.ktmodules/motionlib/src/main/java/com/tejpratapsingh/motionlib/core/motion/BaseContourMotionView.ktmodules/motionlib/src/main/java/com/tejpratapsingh/motionlib/ui/MotionVideoPlayer.ktmodules/motionlib/src/main/java/com/tejpratapsingh/motionlib/ui/custom/text/PopUpTextView.ktmodules/motionlib/src/main/java/com/tejpratapsingh/motionlib/ui/effects/SlideRightToLeftEffect.kt
💤 Files with no reviewable changes (3)
- modules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/motion/LyricsVideoProducer.kt
- modules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/compose/ProjectDetailsCompose.kt
- modules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/view/LyricsTextView.kt
✅ Files skipped from review due to trivial changes (4)
- modules/motionlib/src/main/java/com/tejpratapsingh/motionlib/core/infra/AndroidVideoGenerator.kt
- modules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/activity/SearchActivity.kt
- modules/motion-store/src/main/java/com/tejpratapsingh/motionstore/extensions/ContextExtensions.kt
- modules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/view/LyricsContainer.kt
🚧 Files skipped from review as they are similar to previous changes (3)
- modules/core/src/main/java/com/tejpratapsingh/motionlib/core/MotionDimensionProvider.kt
- modules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/motion/MultiLyricsVideoProducer.kt
- modules/core/src/main/java/com/tejpratapsingh/motionlib/core/MotionTextSizeProvider.kt
| private val wordArray = text.split(" ") | ||
| private val wordCount: Int = wordArray.size |
There was a problem hiding this comment.
Word indexing breaks with multi-whitespace text (spaces/newlines/tabs).
Line 42 splits only on " ", and Line 97 advances by a hardcoded +1. With repeated spaces or line breaks, span ranges drift and animation can target wrong substrings.
💡 Suggested fix
- private val wordArray = text.split(" ")
- private val wordCount: Int = wordArray.size
+ private val wordRanges: List<IntRange> = Regex("""\S+""").findAll(text).map { it.range }.toList()
+ private val wordCount: Int = wordRanges.size
@@
- var currentIdx = 0
- wordArray.forEachIndexed { index, word ->
+ wordRanges.forEachIndexed { index, range ->
val wordProgress = (progress - index).coerceIn(0f, 1f)
@@
- val start = currentIdx
- val end = currentIdx + word.length
+ val start = range.first
+ val end = range.last + 1
@@
- currentIdx += word.length + 1 // +1 for the space
}Also applies to: 75-98
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@modules/motionlib/src/main/java/com/tejpratapsingh/motionlib/ui/custom/text/PopUpTextView.kt`
around lines 42 - 43, The word-splitting and indexing in PopUpTextView
(variables wordArray, wordCount and the code that advances current word using a
hardcoded +1) fails on multiple whitespace; change splitting from text.split("
") to a regex-based word boundary scan (e.g. use Pattern/Matcher with \s+ or \b)
and compute each word's exact start/end offsets by matching against the original
text so span ranges are derived from matcher.start()/matcher.end() rather than
assuming a single-space increment; replace the hardcoded +1 advancement with the
actual matched length or matcher.end() value so animations target the correct
substrings.
| if (motionView !is View) motionView | ||
| if (frame !in startFrame..endFrame) motionView | ||
| if (motionView !is View) return motionView | ||
| if (frame !in startFrame..endFrame) return motionView |
There was a problem hiding this comment.
Clamp translation outside the active range to prevent stale position after seek/loop.
At Line 19, returning early for out-of-range frames means translationX is not reset. If playback jumps backward (e.g., loop reset), the view can stay at its old translated position until the effect range is entered again.
💡 Suggested fix
override fun forFrame(frame: Int): MotionView {
if (motionView !is View) return motionView
- if (frame !in startFrame..endFrame) return motionView
val view = motionView as View
val motionConfig: MotionConfig = provideCurrentConfig()
+ val width = motionConfig.aspectRatio.width.toFloat()
+
+ val progress =
+ when {
+ frame <= startFrame -> 0f
+ frame >= endFrame -> 1f
+ else ->
+ MotionInterpolator.interpolateForRange(
+ interpolator = Interpolators(Easings.LINEAR),
+ currentFrame = frame,
+ frameRange = Pair(startFrame, endFrame),
+ valueRange = Pair(0f, 1f),
+ )
+ }
- val progress =
- MotionInterpolator.interpolateForRange(
- interpolator = Interpolators(Easings.LINEAR),
- currentFrame = frame,
- frameRange = Pair(startFrame, endFrame),
- valueRange = Pair(0f, 1f),
- )
-
- val width = motionConfig.aspectRatio.width.toFloat()
-
// Start at original position (0), move to the left (-width)
view.translationX = -width * progress
return motionView
}Also applies to: 33-37
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@modules/motionlib/src/main/java/com/tejpratapsingh/motionlib/ui/effects/SlideRightToLeftEffect.kt`
at line 19, The early return in SlideRightToLeftEffect (when frame !in
startFrame..endFrame) causes motionView.translationX to retain stale values
after seeks/loops; modify the logic in the method that checks frame (the block
using startFrame, endFrame and motionView) to clamp/reset translationX when
outside the active range (e.g., set motionView.translationX = 0f or to its
default position) instead of simply returning, and apply the same fix for the
other out-of-range checks referenced around lines 33-37 so the view is always
placed correctly when the effect is not active.
1 participant
Summary by CodeRabbit
New Features
Improvements
Code Quality