Refactor scroll state management and enhance BaseScroll configurability

[kurobon-jp]

No description provided.

[octopus-review]

🐙 Octopus Review — PR #6

Summary

このPRは、スクロール状態管理のリファクタリングとエディタUIの共通化を行います。ScrollStatusをpublic enumに昇格させてイベント通知を追加し、Scrollerプロパティをnull安全な遅延初期化パターンに変更、また全EditorクラスをBaseScrollEditorに集約することで重複コードを大幅削減しています。さらにIsDraggable/IsScrollableをシリアライズ可能フィールドに変更し、エディタから設定可能にしています。

Score

Category	Score	Notes
Security	N/A	セキュリティ関連の変更なし
Code Quality	4/5	重複排除は良好、軽微な懸念点あり
Performance	5/5	問題なし
Error Handling	3/5	OnDisableの_tracker.Clear()削除、IEndDragHandlerのキャンセル処理に潜在的問題
Consistency	4/5	概ね一貫しているがOnValueChangedのみ_scroller直接参照が残存
Overall	3/5	Error Handlingの懸念点を要確認

Risk Assessment

Metric	Value
Overall Risk	🟡 Medium
Complexity	Medium
Test Coverage Impact	Needs Attention
Breaking Change	Yes（ScrollStatusのpublic化、IsDraggable/IsScrollableのシリアライズ化）

Positive Highlights

1. エディタコードの重複排除が見事: 5つのEditorクラスが全て共通のBaseScrollEditorを継承するよう整理され、ScrollerDrawerの生成・_isDraggable連動などのロジックが一箇所に集約された。メンテナンス性が大きく向上している。
2. Scrollerプロパティの遅延初期化: _scroller ??= new Scroller()により、インスペクタ上でScroller未設定のケースやエディタのOnValidate呼び出しのような早期アクセスに対して安全になった。
3. OnScrollStateChangedイベントの追加: ScrollStatusをpublicに昇格しイベントを公開することで、スクロール状態変化への外部リアクション（UIアニメーション等）が実装しやすくなった。

Important Files Changed

Filename	Overview
Assets/Editor/BaseScrollEditor.cs	新規追加。全ScrollEditorの基底クラス。共通フィールドのUI描画と_isDraggableによるScrollerの有効/無効連動ロジックを実装
Assets/Runtime/BaseScroll.cs	IsDraggable/IsScrollableのシリアライズ化、Scrollerプロパティの遅延初期化、OnScrollStateChangedイベントの公開、OnDisableからの_tracker.Clear()削除
Assets/Runtime/Utils/Scroller.cs	ScrollStatusenumをpublicに昇格、Statusプロパティ経由での状態変更でイベント発火するよう変更
Assets/Editor/CarouselScrollEditor.cs	BaseScrollEditor継承に変更、固有プロパティのみCreateInherentPropertiesでオーバーライド
Assets/Editor/FixedGridScroll.cs	同上
Assets/Editor/FixedListScrollEditor.cs	同上
Assets/Editor/LazyLayoutListScrollEditor.cs	同上
Assets/Editor/SizedListScrollEditor.cs	同上

Diagram

graph TD
    BaseScrollEditor["BaseScrollEditor (NEW)"]
    CarouselEd["CarouselScrollEditor"]
    FixedGridEd["FixedGridScrollEditor"]
    FixedListEd["FixedListScrollEditor"]
    LazyLayoutEd["LazyLayoutListScrollEditor"]
    SizedListEd["SizedListScrollEditor"]

    BaseScrollEditor --> CarouselEd
    BaseScrollEditor --> FixedGridEd
    BaseScrollEditor --> FixedListEd
    BaseScrollEditor --> LazyLayoutEd
    BaseScrollEditor --> SizedListEd

    BaseScroll["BaseScroll (MODIFIED)"]
    Scroller["Scroller (MODIFIED)"]
    ScrollStatus["ScrollStatus enum (PUBLIC)"]

    BaseScroll -- "lazy init via ??" --> Scroller
    BaseScroll -- "exposes event" --> ScrollStatusEvt["OnScrollStateChanged"]
    Scroller -- "Status property fires" --> ScrollStatusEvt
    Scroller -- "uses" --> ScrollStatus

    classDef added fill:#4ade80,stroke:#166534,color:#000
    classDef changed fill:#fbbf24,stroke:#92400e,color:#000
    classDef unchanged fill:#374151,stroke:#6b7280,color:#e5e7eb

    class BaseScrollEditor added
    class BaseScroll,Scroller changed
    class ScrollStatus added
    class CarouselEd,FixedGridEd,FixedListEd,LazyLayoutEd,SizedListEd changed
    class ScrollStatusEvt unchanged

Loading

Last reviewed commit: (latest in diff)

Checklist

• No hardcoded secrets or credentials
• Error handling is comprehensive（OnDisableの_trackerクリア削除、EndDragキャンセル要確認）
• Edge cases are covered（IsDraggable=false時のEndDrag処理）
• Naming is clear and consistent with codebase conventions
• No unnecessary dependencies added
• Database migrations are reversible (N/A)
• API changes are backward compatible（IsDraggable/IsScrollableのデフォルト値変更、ScrollStatusの可視性変更はbreaking change）

[octopus-review]

9 files reviewed, 6 findings | View scores & details

🟡🔵💡 Additional findings

Severity	File	Title	Description
🟡	Assets/Runtime/BaseScroll.cs:L85	OnDisableからの_tracker.Clear()削除によりエディタでDrivenRectTransformTrackerリークの可能性	以前はOnDisable内に#if UNITY_EDITORブロックで_tracker.Clear()が呼ばれていたが、このPRで削除された。_tracker（DrivenRectTransformTracker）はUnityエディタ上でRectTransformのプロパティをロックするために使われており、コンポーネント無効化時にクリアしないと、エディタ上でビューポートやコンテンツのPivot/SizeDeltaが編集不能のままになる可能性がある。OnValidateにはまだ_tracker.Clear()が残っているが、OnDisable時にもクリアすべきである。
🟡	Assets/Editor/CarouselScrollEditor.cs:L9	CarouselScrollEditorで_contentPaddingと_spaceが追加されたが、CarouselScrollに該当フィールドが存在するか不明	BaseScrollEditor.CreateInspectorGUIは_contentPaddingと_spaceを全サブクラスのインスペクタに追加しているが、旧CarouselScrollEditorではこれらのフィールドは描画されていなかった（_spaceのみ描画）。CarouselScrollクラスに_contentPaddingフィールドが存在しない場合、serializedObject.FindProperty("_contentPadding")はnullを返し、PropertyFieldに渡されても描画されないだけなので実害は少ないが、意図しないフィールドが表示される可能性がある。コードベースのコンテキストからCarouselScrollの定義が確認できないため、要確認を推奨する。
🟡	Assets/Runtime/BaseScroll.cs:L51	OnValueChangedプロパティのみ_scrollerフィールドを直接参照しており、遅延初期化パターンと非一貫	public ScrollEvent OnValueChanged => _scroller.OnValueChanged;は_scrollerフィールドを直接参照しているが、他の全プロパティはScroller（遅延初期化プロパティ）経由でアクセスしている。_scrollerがnullの場合（シリアライズされていない場合など）にNullReferenceExceptionが発生する可能性がある。一貫性のためにもScroller.OnValueChangedに変更すべきである。
🔵	Assets/Runtime/Utils/Scroller.cs:L7	ScrollStatusのpublic化はBreaking Changeであり、名前空間の公開APIとなる	ScrollStatusがprivate enumからpublic enumに昇格したことで、SimpleScroll名前空間の公開APIの一部となった。将来的にステータスの追加・削除・リネームが破壊的変更になる。現在Coastingは物理的な挙動（慣性コースティング）を表しているが、外部から参照されることを考慮した命名・ドキュメントの整備を検討すべきである。また、パッケージのsemantic versioningの更新が必要。
💡	Assets/Editor/BaseScrollEditor.cs:L31	BaseScrollEditor.csの末尾に改行がない	ファイル末尾に改行（newline）がない。Gitの差分やエディタツールによっては警告が表示される場合がある。コードベース内の他のファイルと合わせて末尾改行を追加することを推奨。

_{Reviewed by Octopus Review, an AI-powered PR review tool.}

[octopus-review]

🟠 IsDraggable=false時のOnEndDragでポインターIDがリセットされず次のドラッグが受け付けられない可能性

IsDraggableがfalseの場合、OnEndDrag内でScroller.Stop()を呼んでreturnしているが、その直前に_pointerId = int.MinValueのリセットが行われている（diff上ではL190）。一方、OnBeginDragでは!IsDraggableチェックが追加されたため、IsDraggableがfalse→trueに動的変更された場合でも問題は起きにくい。ただし、IsDraggable=falseでOnEndDragが呼ばれた際にScroller.Stop()を呼ぶことでOnScrollStateChangedイベントが発火し、呼び出し元が意図しない状態変化通知を受け取る可能性がある。Stop()呼び出しが本当に必要か、またIsIdlingチェックを挟むべきか検討を推奨する。

Suggested change

	_pointerId = int.MinValue;
	void IEndDragHandler.OnEndDrag(PointerEventData e)
	{
	if (DataSource == null || _pointerId != e.pointerId) return;
	_pointerId = int.MinValue;
	if (!IsDraggable)
	{
	if (!Scroller.IsIdling) Scroller.Stop();
	return;
	}
	var targetPosition = Scroller.OnEndDrag(e);
	OnDrag(targetPosition);
	}

📌 Grounding

Minimum fix scope: IEndDragHandler.OnEndDrag内のStop()呼び出しにIsIdlingガードを追加する1行の変更。

Suggested regression test:

// IsDraggable=falseでEndDragを呼んだ際にOnScrollStateChangedが不必要に発火しないことを確認
// scroll.IsDraggable = false;
// BeginDrag (IsDraggable=trueの状態で) → IsDraggable=falseに変更 → EndDrag
// OnScrollStateChangedの発火回数が期待値と一致することをアサート

🤖 AI Fix Prompt

Fix the following High (Bug) issue in `Assets/Runtime/BaseScroll.cs` at lines 193-200:

Problem: `IsDraggable`が`false`の場合、`OnEndDrag`内で`Scroller.Stop()`を呼んで`return`しているが、その直前に`_pointerId = int.MinValue`のリセットが行われている（diff上ではL190）。一方、`OnBeginDrag`では`!IsDraggable`チェックが追加されたため、`IsDraggable`が`false`→`true`に動的変更された場合でも問題は起きにくい。ただし、`IsDraggable=false`で`OnEndDrag`が呼ばれた際に`Scroller.Stop()`を呼ぶことで`OnScrollStateChanged`イベントが発火し、呼び出し元が意図しない状態変化通知を受け取る可能性がある。`Stop()`呼び出しが本当に必要か、また`IsIdling`チェックを挟むべきか検討を推奨する。

Suggested fix:
void IEndDragHandler.OnEndDrag(PointerEventData e)
{
    if (DataSource == null || _pointerId != e.pointerId) return;
    _pointerId = int.MinValue;
    if (!IsDraggable)
    {
        if (!Scroller.IsIdling) Scroller.Stop();
        return;
    }
    var targetPosition = Scroller.OnEndDrag(e);
    OnDrag(targetPosition);
}

Scope: `IEndDragHandler.OnEndDrag`内のStop()呼び出しにIsIdlingガードを追加する1行の変更。