fix: resolve code-review findings in binders and collections

[VPDPersonal]

Summary

Fixes from the max-effort code review of #25 (Aspid.MVVM 1.1.0). 12 of 13 findings addressed:

• CollectionViewModelBinder — rebuild positional views on Move (the sibling-index swap displayed the wrong order for moves of distance > 1), guard the view array against out-of-range growth, and dedup the four granular hooks into RebuildFromCollection().
• ImageSpriteBinder / ImageSpriteMonoBinder — clear Image.sprite on unbind so it no longer references the destroyed generated sprite; share a CreateSprite helper.
• RendererMaterialColorSwitcherBinder — cache the Renderer.materials array to drop the per-value Material[] allocation; clear on unbind.
• OrCompositeCollectionFilter — an all-empty / all-null filter now passes all items (consistent with And), implemented allocation-free.
• VirtualizedList — OnMove window check uses < (matching OnAdded/OnRemoved); window-gate the batch OnAdded/OnRemoved instead of an unconditional Refresh().
• ToCultureStringExtensions — delegate the ToCultureString overloads to the existing ToCultureInfo helper.
• ObservableListViewModelBinder twins — extract shared view-list bookkeeping into ObservableListViewModelBinderHelper.

Notes for review

• Authored with the Unity Editor not running — must be compiled in Unity before review. Changes were verified by static analysis only.
• Two findings intentionally excluded:
  • NumberToBoolConverter float→double (finding 5) — reverted: _value is a [SerializeField], so the type change risks resetting the threshold on existing serialized assets. The edge case (threshold > 2^24) is not worth the migration risk.
  • Command-binder base-class extraction (finding 9) — deferred: needs Unity in the loop (serialization of [SerializeReference] fields plus the Mono/non-Mono gameObject visibility difference make a blind refactor unsafe).

Linked issues

Refs #25

[VPDPersonal]

@claude once

[claude]

Claude finished @VPDPersonal's task in 2m 50s —— View job

---

Code Review

Overall, the changes are clean and well-targeted. The four-hook dedup, RebuildFromCollection, CreateSprite extraction, and ObservableListViewModelBinderHelper are all straightforward wins. I found one newly-introduced potential bug, one pre-existing indexing hole now worth fixing, and a few minor items.

---

P1 — Bug introduced by this PR

RendererMaterialColorSwitcherBinder.OnUnbound does not call base.OnUnbound()

RendererMaterialColorSwitcherBinder.cs:68–71

protected override void OnUnbound()
{
    _materials = null;   // ← base.OnUnbound() is never called
}

This override is new in this PR — the method didn't exist before. Before this change the base class's OnUnbound ran automatically; now it's silently suppressed. SwitcherColorBinder<Renderer> → TargetBinder<Renderer, bool> may clear the target reference, cancel subscriptions, or call a chain of OnUnbound hooks. Not forwarding the call is a correctness risk.

Fix:

protected override void OnUnbound()
{
    _materials = null;
    base.OnUnbound();
}

Fix this →

---

P2 — Pre-existing indexing hole worth closing now

CollectionViewModelBinder.OnReplace — no bounds guard

CollectionViewModelBinder.cs:83–89

protected override void OnReplace(IViewModel? oldItem, IViewModel? newItem, int newStartingIndex)
{
    _views[newStartingIndex].Deinitialize();   // ← throws if newStartingIndex >= _views.Length
    if (newItem is not null)
        _views[newStartingIndex].Initialize(newItem);
}

OnAdded(IReadOnlyCollection) got a guard for index >= _views.Length in this PR. The same logic applies to OnReplace: if the collection has more items than pre-allocated views, a Replace on an out-of-range index will throw. The fix mirrors what was done in OnAdded:

protected override void OnReplace(IViewModel? oldItem, IViewModel? newItem, int newStartingIndex)
{
    if (newStartingIndex >= _views.Length) return;
    _views[newStartingIndex].Deinitialize();
    if (newItem is not null)
        _views[newStartingIndex].Initialize(newItem);
}

Fix this →

---

P3 — OnUnbound base-call pattern (pre-existing, same family)

ImageSpriteBinder.OnUnbound (line 51) and ImageSpriteMonoBinder.OnUnbound (line 46) also omit base.OnUnbound(). These were pre-existing omissions, not introduced here, but since unbind cleanup is the focus of this PR it's worth noting for a follow-up. Whether they need the call depends on what TargetBinder / ComponentMonoBinder do in their own OnUnbound — worth verifying.

---

Positive notes

• OrCompositeCollectionFilter — the hasPredicate sentinel correctly makes an all-null/empty filter pass everything, matches And semantics, zero allocations. Clean.
• ToCultureStringExtensions — delegating to ToCultureInfo() is a textbook DRY fix. DefaultThreadCurrentCulture / DefaultThreadCurrentUICulture returning null is handled identically to before (.ToString(null) uses current thread culture in both paths).
• RebuildFromCollection() — the dedup is clean. The private doc comment makes the intent clear.
• ObservableListViewModelBinderHelper — a solid minimal internal helper. Keeping it in the same file as ObservableListViewModelBinder is reasonable. OnMove correctly syncs both the List<T> and the sibling index.
• VirtualizedList OnMove fix — changing <= to < is the right fix and matches the OnAdded/OnRemoved gates. The window logic reads consistently now.
• ImageSpriteBinder.CreateSprite as internal static — correct visibility for cross-class reuse within the assembly without exposing it publicly.

---

Minor

• Several modified files are missing a trailing newline (\ No newline at end of file in the diff for ToCultureStringExtensions.cs, ImageSpriteBinder.cs, ImageSpriteMonoBinder.cs, RendererMaterialColorSwitcherBinder.cs, ObservableListViewModelMonoBinder.cs). Not blocking, but worth cleaning up in a pass.

---

Summary: One newly-introduced regression (missing base.OnUnbound() in RendererMaterialColorSwitcherBinder), one pre-existing gap worth closing (OnReplace bounds check), and a handful of style nits. Everything else is correct.

[VPDPersonal]

Addressed review feedback — 200dee2f

• P1 — RendererMaterialColorSwitcherBinder.OnUnbound missing base.OnUnbound() ✅ Fixed. The override added earlier in this PR was dropping the base cleanup; it now calls base.OnUnbound().
• P2 — CollectionViewModelBinder.OnReplace bounds guard ✅ Added if (newStartingIndex >= _views.Length) return;, mirroring the OnAdded guard from this PR.
• P3 — sprite binders omitting base.OnUnbound() ✅ Added to both ImageSpriteBinder and ImageSpriteMonoBinder for consistency with the codebase convention.
• Minor — trailing newlines ✅ Added on the flagged files.

Also, by request, the helpers were moved into their own files instead of living inside a binder class:

• ObservableListViewModelBinderHelper.cs
• SpriteBinderHelper.cs (holds the former ImageSpriteBinder.CreateSprite)

Still to do: compile/verify in Unity (this branch was authored with the Editor not running).