ADFA-4826: Add shared IDE Compose theming in common-compose - #1653
ADFA-4826: Add shared IDE Compose theming in common-compose#1653itsaky-adfa wants to merge 2 commits into
Conversation
There was a problem hiding this comment.
Claude Code Review
This repository is configured for manual code reviews. Comment @claude review for a one-time review, or @claude review always to subscribe this PR to a review on every future push.
Tip: disable this comment in your organization's Code Review settings.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 Walkthrough
WalkthroughThe PR adds the ChangesShared Compose theming
Estimated code review effort: 3 (Moderate) | ~20 minutes Merge Risk: ⚪ Minimal · up to This PR centralizes existing Compose theme definitions without an intended behavior change, and the reported checks pass; no actionable merge-blocking risk remains beyond normal review. Sequence Diagram(s)sequenceDiagram
participant Context
participant IdeTheme
participant ideColorScheme
participant MaterialTheme
Context->>IdeTheme: Read context and system dark-mode state
IdeTheme->>ideColorScheme: Resolve XML theme attributes
ideColorScheme-->>IdeTheme: Return Material 3 ColorScheme
IdeTheme->>MaterialTheme: Apply ColorScheme, typography, and content color
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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
`@common-compose/src/test/java/com/itsaky/androidide/common/compose/IdeColorSchemeTest.kt`:
- Around line 7-10: Update IdeColorSchemeTest to use JUnit Jupiter by replacing
org.junit.Test with org.junit.jupiter.api.Test, and replace
assertEquals/assertTrue usages with equivalent Truth assertions throughout the
test class. Keep the existing test behavior and coverage unchanged.
🪄 Autofix
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 Plus
Run ID: fe123155-b3a9-4cdd-95e5-a6970f48d4aa
📒 Files selected for processing (10)
ARCHITECTURE.mdcommon-compose/build.gradle.ktscommon-compose/src/main/java/com/itsaky/androidide/common/compose/IdeColorScheme.ktcommon-compose/src/main/java/com/itsaky/androidide/common/compose/IdeTheme.ktcommon-compose/src/test/java/com/itsaky/androidide/common/compose/IdeColorSchemeTest.ktfloating-window/build.gradle.ktsfloating-window/src/main/java/com/itsaky/androidide/floating/ui/FloatingTheme.ktprofiler/build.gradle.ktsprofiler/src/main/java/org/appdevforall/cotg/profiler/ui/theme/ProfilerTheme.ktsettings.gradle.kts
| import org.junit.Assert.assertEquals | ||
| import org.junit.Assert.assertTrue | ||
| import org.junit.Test | ||
| import com.google.android.material.R as MaterialR |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Use JUnit Jupiter and Truth in this new test.
Replace org.junit.Test with org.junit.jupiter.api.Test. Replace assertEquals and assertTrue with Truth assertions. This keeps new tests consistent with the shared test conventions.
As per coding guidelines, “Use JUnit Jupiter, Truth, MockK for new tests.”
Also applies to: 57-121
🤖 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
`@common-compose/src/test/java/com/itsaky/androidide/common/compose/IdeColorSchemeTest.kt`
around lines 7 - 10, Update IdeColorSchemeTest to use JUnit Jupiter by replacing
org.junit.Test with org.junit.jupiter.api.Test, and replace
assertEquals/assertTrue usages with equivalent Truth assertions throughout the
test class. Keep the existing test behavior and coverage unchanged.
Source: Coding guidelines
d1b690e to
96e9901
Compare
New leaf module holding the Compose theme any module can opt into: IdeColorScheme derives a Material3 scheme from the IDE's own colour resources, IdeTheme applies it and seeds LocalContentColor so text on a themed surface inherits the right colour. Compose types are exposed as `api` because consumers write Compose against them. Modules that are not Compose depend on nothing new.
Both modules carried their own near-identical copy of the IDE colour derivation. They now delegate to common-compose, so there is one place where the IDE's Compose colours are defined.
96e9901 to
46cf261
Compare
Jira: ADFA-4826
Bottom of a 3-PR stack. Extracts the Compose colour scheme and theme that
profilerandfloating-windowhad each duplicated into a newcommon-composemodule, so the refactoring UI in the two PRs above has one theme to build on.What's here
common-composemodule:IdeColorScheme(light/dark IDE palettes) andIdeTheme.FloatingThemeandProfilerThemenow delegate to it instead of carrying their own copies (-111 lines).IdeColorSchemeTestcovers the palettes.2 commits, 10 files, +308/-111.
Verification
:common-compose:compileV8DebugKotlin,:profiler:compileV8DebugKotlin,:floating-window:compileV8DebugKotlinpass.:common-compose:testV7DebugUnitTest- 7 tests, 0 failures.spotlessCheckpasses.No behavioural change intended: both themes should render exactly as before.
Stack
Review and merge bottom-up.
Stack created with GitHub Stacks CLI • Give Feedback 💬