fix: correct @JvmName in ViewGroupBindings, fix nullable in findRootView, add tests#166
fix: correct @JvmName in ViewGroupBindings, fix nullable in findRootView, add tests#166kirich1409 wants to merge 2 commits intodevelopfrom
Conversation
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Review Summary by QodoAdd thread-safety and fix code quality issues across view binding modules
WalkthroughsDescription• Added thread-safety with @Volatile and synchronized double-checked locking to LazyViewBindingProperty, ActivityViewBindingProperty, and FragmentViewBindingProperty to prevent race conditions • Switched ViewBindingCache to ConcurrentHashMap and marked impl field as @Volatile for thread-safe caching • Fixed nullable bug in DialogFragment.findRootView() using requireViewByIdCompat instead of findViewById • Corrected @JvmName annotations in ViewGroupBindings.kt from "viewBindingFragment" to "viewBindingViewGroup"/"viewBindingViewGroupInflate" • Added comprehensive tests for concurrent access, clear() behavior, and binding recreation Diagramflowchart LR
A["LazyViewBindingProperty"] -->|"add @Volatile + synchronized"| B["Thread-safe getValue/clear"]
C["ActivityViewBindingProperty"] -->|"add @Volatile + synchronized"| B
D["FragmentViewBindingProperty"] -->|"add @Volatile + synchronized"| B
E["ViewBindingCache"] -->|"use ConcurrentHashMap + @Volatile"| F["Thread-safe reflection cache"]
G["DialogFragment.findRootView"] -->|"fix nullable return"| H["Use requireViewByIdCompat"]
I["ViewGroupBindings"] -->|"correct @JvmName annotations"| J["Fix Java interop"]
B -->|"verify with"| K["Concurrent access tests"]
F -->|"verify with"| K
File Changes1. vbpd-core/src/main/kotlin/dev/androidbroadcast/vbpd/ViewBindingProperty.kt
|
Code Review by Qodo
1. clear test lacks visibility
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
| } | ||
|
|
||
| @Test | ||
| fun `clear does not affect getValue`() { |
There was a problem hiding this comment.
1. clear test lacks visibility 📘 Rule violation ✓ Correctness
The added test function has implicit public visibility because it declares fun without a visibility modifier. This violates the requirement that all public Kotlin declarations must declare visibility explicitly.
Agent Prompt
## Issue description
A newly added Kotlin declaration is implicitly `public` because it omits a visibility modifier.
## Issue Context
The project compliance rules require explicit visibility modifiers for any declaration that would otherwise be public by default.
## Fix Focus Areas
- vbpd-core/src/test/kotlin/dev/androidbroadcast/vbpd/EagerViewBindingPropertyTest.kt[44-53]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| } | ||
| } else { | ||
| return requireView().findViewById(viewBindingRootId) | ||
| return requireView().requireViewByIdCompat(viewBindingRootId) |
There was a problem hiding this comment.
2. Dialog rootid 0 throws 🐞 Bug ✓ Correctness
DialogFragment.findRootView() treats viewBindingRootId==0 as valid when showsDialog=true (returns decorView), but when showsDialog=false it unconditionally calls requireViewByIdCompat(viewBindingRootId) and will throw for 0, making the API behavior inconsistent depending on showsDialog.
Agent Prompt
### Issue description
`DialogFragment.findRootView()` is inconsistent: it supports `viewBindingRootId == 0` when `showsDialog == true` (returns `decorView`), but will throw when `showsDialog == false` because it unconditionally calls `requireViewByIdCompat(0)`.
### Issue Context
This function already encodes `0` as a sentinel value in the dialog path, so the non-dialog path should handle it consistently.
### Fix Focus Areas
- vbpd/src/main/kotlin/dev/androidbroadcast/vbpd/internal/VbpdUtils.kt[45-56]
### Suggested change (concept)
In the `else` branch:
- `val root = requireView()`
- `return if (viewBindingRootId != 0) root.requireViewByIdCompat(viewBindingRootId) else root`
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
There was a problem hiding this comment.
Pull request overview
This PR addresses several thread-safety, correctness, and code quality issues across the vbpd, vbpd-core, and vbpd-reflection modules of the View Binding Property Delegate library.
Changes:
- Thread-safety hardening:
LazyViewBindingProperty,ActivityViewBindingProperty, andFragmentViewBindingPropertynow use@Volatile+ double-checked locking (synchronized) to prevent race conditions on concurrentgetValue()/clear()calls;ViewBindingCacheswitched toConcurrentHashMapwith@Volatile impl - Bug fixes: Corrected
DialogFragment.findRootView()to use non-nullrequireViewByIdCompatinstead of nullablefindViewById(preventing a return-type contract violation), and fixed all four@JvmNameannotations onViewGroupBindings.ktextension functions (were incorrectly annotated as"viewBindingFragment") - New tests: Added concurrent-access test for
LazyViewBindingProperty, andclear()behavior tests forEagerViewBindingPropertyandActivityViewBindingProperty
Reviewed changes
Copilot reviewed 9 out of 10 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
vbpd-core/src/main/kotlin/.../ViewBindingProperty.kt |
Adds @Volatile + synchronized double-checked locking to LazyViewBindingProperty.getValue and clear |
vbpd/src/main/kotlin/.../ActivityViewBindings.kt |
Adds @Volatile to lifecycleCallbacks and wraps registration/clearing in synchronized(this) |
vbpd/src/main/kotlin/.../FragmentViewBindings.kt |
Same thread-safety treatment as ActivityViewBindings.kt for fragment lifecycle callbacks |
vbpd/src/main/kotlin/.../internal/VbpdUtils.kt |
Replaces nullable findViewById with non-null requireViewByIdCompat in DialogFragment.findRootView non-dialog branch |
vbpd-reflection/src/main/kotlin/.../ViewGroupBindings.kt |
Corrects @JvmName on all four ViewGroup.viewBinding overloads from "viewBindingFragment" to correct names |
vbpd-reflection/src/main/kotlin/.../ViewBindingCache.kt |
Adds @Volatile to impl and switches cache maps to ConcurrentHashMap |
vbpd-core/src/test/kotlin/.../LazyViewBindingPropertyTest.kt |
Adds concurrent-access test verifying viewBinder is called exactly once |
vbpd-core/src/test/kotlin/.../EagerViewBindingPropertyTest.kt |
Adds test verifying clear() has no effect on EagerViewBindingProperty |
vbpd/src/test/kotlin/.../ActivityViewBindingPropertyTest.kt |
Adds test verifying clear() resets binding and allows recreation |
.gitignore |
Adds .worktrees/ to ignore list |
Comments suppressed due to low confidence (1)
vbpd-reflection/src/main/kotlin/dev/androidbroadcast/vbpd/ViewBindingCache.kt:35
- Using
ConcurrentHashMap.getOrPutdoes NOT provide thread-safe atomic put-if-absent semantics. The KotlingetOrPutextension function is defined onMutableMapand is not atomic — under concurrent access, it can invoke the value-producing lambda multiple times and may overwrite entries. For true thread-safety onConcurrentHashMap, usecomputeIfAbsentinstead, which guarantees the lambda is called at most once per key.
private val inflateCache = ConcurrentHashMap<Class<out ViewBinding>, InflateViewBinding<*>>()
private val bindCache = ConcurrentHashMap<Class<out ViewBinding>, BindViewBinding<*>>()
@Suppress("UNCHECKED_CAST")
override fun <T : ViewBinding> getInflateWithLayoutInflater(viewBindingClass: Class<T>): InflateViewBinding<T> =
inflateCache.getOrPut(viewBindingClass) {
InflateViewBinding(viewBindingClass)
} as InflateViewBinding<T>
@Suppress("UNCHECKED_CAST")
override fun <T : ViewBinding> getBind(viewBindingClass: Class<T>): BindViewBinding<T> =
bindCache.getOrPut(viewBindingClass) {
BindViewBinding(viewBindingClass)
} as BindViewBinding<T>
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
| @CallSuper | ||
| public override fun clear() { | ||
| viewBinding = null | ||
| synchronized(this) { | ||
| viewBinding = null | ||
| } | ||
| } |
There was a problem hiding this comment.
Both ActivityViewBindingProperty.clear() and FragmentViewBindingProperty.clear() call super.clear() (which does synchronized(this) { viewBinding = null }) from within their own synchronized(this) block. While JVM synchronized is reentrant for the same thread so this does not deadlock, it introduces an unnecessary nested lock acquisition. Since viewBinding is already protected by the outer synchronized(this), the inner lock in the parent clear() is redundant. Consider refactoring LazyViewBindingProperty to provide a non-synchronized internal method (e.g. a protected fun clearViewBinding()) that subclasses can call within their own synchronized block.
…iew, add tests - Fix @JvmName("viewBindingFragment") → "viewBindingViewGroup"/"viewBindingViewGroupInflate" in reflection ViewGroupBindings (copy-paste error from FragmentViewBindings) - Fix DialogFragment.findRootView() using findViewById (nullable) instead of requireViewByIdCompat (non-null) when showsDialog=false - Add EagerViewBindingProperty.clear() test verifying no-op behavior - Add ActivityViewBindingProperty test verifying binding recreation after clear() Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
kirich1409
force-pushed
the
fix/code-quality-fixes
branch
from
f0feb15 to
0619668
Compare
kirich1409
changed the title
Summary
@JvmName("viewBindingFragment")→"viewBindingViewGroup"/"viewBindingViewGroupInflate"in reflectionViewGroupBindings.kt(copy-paste error)DialogFragment.findRootView()returning nullable fromfindViewByIdinstead of usingrequireViewByIdCompatwhenshowsDialog=falseEagerViewBindingProperty.clear()test verifying no-op behaviorActivityViewBindingPropertytest verifying binding recreation afterclear()Test plan
./gradlew checkpasses@JvmNamechange doesn't break Java callers (binary-incompatible for reflection module's ViewGroup extensions from Java)🤖 Generated with Claude Code