runceel / ReactiveProperty

ReactiveProperty provides MVVM and asynchronous support features under Reactive Extensions. Target frameworks are .NET 6+, .NET Framework 4.7.2 and .NET Standard 2.0.
MIT License
903 stars 101 forks source link

ReactivePropertyとして意図していないオブジェクトが抽出される #295

Closed dhq-boiler closed 3 years ago

dhq-boiler commented 3 years ago

こんにちは。今回は日本語で質問させていただきます。

主題

今バグに悩まされております。前回の質問 #275 で提案されたアイディアをベースに、「選択されたアイテムのみを抽出する」SelectedItemsという名のReactivePropertyを考えました。

ここでさらに機能追加をします。直線オブジェクトの場合は、その両端のスナップポイントのViewModelを代わりに抽出したい、すなわちConnectorBaseViewModelクラスのインスタンスの時は、その両端のスナップポイントオブジェクト(SnapPoint0VMとSnapPoint1VM)を抽出して、かつそのIsSelected = TrueであるViewModelのみを抽出したいです。

これを実現しようとしたコードが下記になります。 しかし、このコードは正しく動作しません。 (AllItemsプロパティは前回の質問 #275 で提案されたコードを少し加工していますが正しく動作しています。)

public class DiagramViewModel : BindableBase, IDiagramViewModel, IDisposable
    {
        :
        public ReadOnlyReactivePropertySlim<SelectableDesignerItemViewModelBase[]> AllItems { get; }

        public ReadOnlyReactivePropertySlim<SelectableDesignerItemViewModelBase[]> SelectedItems { get; }
        :

        public DiagramViewModel(MainWindowViewModel mainWindowViewModel, int width, int height)
        {
            :
            AllItems = Layers.CollectionChangedAsObservable()
                             .Select(_ => Layers.Select(x => x.LayerItemsChangedAsObservable()).Merge()
                                .Merge(this.ObserveProperty(y => y.BackgroundItem.Value).ToUnit()))
                             .Switch()
                             .Select(_ => Layers.SelectRecursive<LayerTreeViewItemBase, LayerTreeViewItemBase>(x => x.Children)
                                                .Where(x => x.GetType() == typeof(LayerItem))
                                                .Select(y => (y as LayerItem).Item.Value)
                                                .Union(new SelectableDesignerItemViewModelBase[] { BackgroundItem.Value })
                                                .ToArray())
                             .ToReadOnlyReactivePropertySlim(Array.Empty<SelectableDesignerItemViewModelBase>());

            :
            SelectedItems = Layers.CollectionChangedAsObservable()
                                  .Select(_ => Layers.Select(x => x.SelectedLayerItemsChangedAsObservable()).Merge()
                                      .Merge(Layers.SelectRecursive<LayerTreeViewItemBase, LayerTreeViewItemBase>(x => x.Children)
                                                   .Where(x => x.GetType() == typeof(LayerItem))
                                                   .Select(y => (y as LayerItem).Item.Value)
                                                   .OfType<ConnectorBaseViewModel>()
                                                   .SelectMany(x => new List<SnapPointViewModel>() { x.SnapPoint0VM.Value, x.SnapPoint1VM.Value })
                                                   .ToObservableCollection()
                                                   .ObserveElementProperty(x => x.IsSelected.Value)
                                                   .ToUnit()))
                                  .Switch()
                                  .Select(_ => Layers.SelectRecursive<LayerTreeViewItemBase, LayerTreeViewItemBase>(x => x.Children)
                                                     .Where(x => x.GetType() == typeof(LayerItem))
                                                     .Select(y => (y as LayerItem).Item.Value)
                                                     .Except(Layers.SelectRecursive<LayerTreeViewItemBase, LayerTreeViewItemBase>(x => x.Children)
                                                                   .Where(x => x.GetType() == typeof(LayerItem))
                                                                   .Select(y => (y as LayerItem).Item.Value)
                                                                   .OfType<ConnectorBaseViewModel>())
                                                     .Union(Layers.SelectRecursive<LayerTreeViewItemBase, LayerTreeViewItemBase>(x => x.Children)
                                                                  .Where(x => x.GetType() == typeof(LayerItem))
                                                                  .Select(y => (y as LayerItem).Item.Value)
                                                                  .OfType<ConnectorBaseViewModel>()
                                                                  .SelectMany(x => new List<SnapPointViewModel>() { x.SnapPoint0VM.Value, x.SnapPoint1VM.Value })
                                                            )
                                                     .Where(z => z.IsSelected.Value == true)
                                                     .OrderBy(z => z.SelectedOrder.Value)
                                                     .ToArray())
                                  .ToReadOnlyReactivePropertySlim(Array.Empty<SelectableDesignerItemViewModelBase>());
        }
    }

以下がテストケースになりますが、挙げている1つのテストケースは失敗します。

using boilersGraphics.Models;
using boilersGraphics.ViewModels;
using NUnit.Framework;
using System.Linq;
using System.Windows;

namespace Question20210925.Test
{
    [TestFixture]
    public class SelectedItemsTest
    {
        :
        //他のテストはすべて合格します。
  //下記のテストは不合格になります。
        :
        [Test]
        public void 直線の頂点1つが選択されている()
        {
            boilersGraphics.App.IsTest = true;
            var mainWindowViewModel = new MainWindowViewModel(null);
            var viewModel = new DiagramViewModel(mainWindowViewModel, 1000, 1000);
            viewModel.Layers.Clear();
            var layer1 = new Layer();
            layer1.Name.Value = "レイヤー1";
            viewModel.Layers.Add(layer1);
            layer1.IsSelected.Value = true;

            var item1 = new StraightConnectorViewModel(viewModel, new Point(10, 10), new Point(20, 20));
            viewModel.AddItemCommand.Execute(item1);

            layer1.Children[0].IsSelected.Value = true;
            item1.SnapPoint0VM.Value.IsSelected.Value = true;
            item1.SnapPoint1VM.Value.IsSelected.Value = false;

            Assert.That(viewModel.SelectedItems.Value.ToList(), Has.Count.EqualTo(1)); //Expected: property Count equal to 1 But was:  2
            Assert.That(viewModel.SelectedItems.Value.ElementAt(0).IsSelected.Value, Is.True);
        }
    }
}

背景

この問題の背景を説明しますと、私が開発している当プロジェクトは描画されたアイテムをクリックして選択したり、なげなわツールで選択すると、内部的にDiagramViewModelクラスのSelectedItemsプロパティで選択されたアイテムをアイテム全体から抽出して返すような仕組みになっています。これはReactivePropertyにより実現しました。

しかし、先日、直線の頂点の片方を選択した時に、矢印キーで移動させると、その選択した直線の頂点の片方が動くようにプログラミングしたつもりだったのですが、本記事で挙げているテストコードで失敗になってしまいました。 つまり、直線の頂点の片方を選択した時に、矢印キーで移動させると、その選択した直線の頂点の片方が動くはずが、その選択した直線の頂点の両方が選択されてしまい、結果的に直線全体が動いてしまうという現象が起きています。

概説図

ちなみに、本現象を質問として掲載するために当プロジェクトの最小機能版を作成したのですが、 それを動かして試してみた様子をGifアニメーションで確認できるようにしてみました。

最小機能版で試す

ちょっとよく見ないとわかりにくいのですが、

  1. 直線を描画
  2. なげなわツールで直線の頂点の片方を選択
  3. 選択ツール(マウスポインターの形のツール)で何もないところをクリック(直線を矢印キーで動かすための儀式です、たぶんなんらかのバグ)
  4. 再度なげなわツールで直線の頂点の片方を選択
  5. 矢印キーで直線を動かす(1ピクセル単位で動くのでぱっと見わかりづらいです) Gifアニメーションは上記のような内容です。

直線全体が動いてしまう原因はSelectedItemsでIsSelected=TrueのSnapPointViewModelだけでなく、IsSelected=FalseのSnapPointViewModelまで抽出されているためです。だからこの問題の動作の原因はSelectedItemsの実装方法にあると言ってよさそうです。

ソースコード

ソースコードは下記の通りです。 boiler's Graphics https://github.com/dhq-boiler/boiler-s-Graphics コミット:aa22aeb付近(develop)

上記のプロジェクトは巨大でわかりにくいので、これの最小機能版を作りました。(それでもコード数・ファイル数は多いですが、不必要なツールは削ってあるのでわかりやすいと思います) https://github.com/dhq-boiler/Qiita/tree/develop/Question20210925

その他

本質問をする前に、Qiitaにて質問を投稿しています。 こちらに2つほど回答はついたのですが、有力な手がかりは得られませんでした。

[WPF]直線の片方の頂点を選択した時、SelectedItemsが直線の両方の頂点を示してしまう https://qiita.com/dhq_boiler/questions/0ed17c0b14eb796cd72e

すみません、長くなりましたがよろしくお願いいたします。

runceel commented 3 years ago

とりあえず単体テスト通すだけならこれで行けました。 他の問題が起こるかどうかはプログラムが結構大きいので、私が見た範囲内では判断できません。

https://github.com/runceel/Qiita/commit/ca51f9d538cd06e2a4bdb4075665930f46ab76e3

ポイントは

  1. 自分が処理をトリガーしたいタイミングできちんと IObservable<Unit> から OnNext が呼ばれるようになっているか
  2. OnNext が呼ばれたときに思った通りに値の変換が行えているか

にわけてクエリを書いてチェックすることだと思います。 今回の例だと SelectedItems プロパティの値を集めなおしたいタイミングを決めるための以下の部分が間違ってました。

https://github.com/runceel/Qiita/commit/ca51f9d538cd06e2a4bdb4075665930f46ab76e3

この Switch の後に Do メソッドでも入れてブレークポイントはって、思ったタイミングでブレークするかどうかをデバッグしてみると良いと思います。今回の場合は直線の頂点の選択状態が変わっても OnNext は呼ばれていませんでした。なので、そこが意図されてると思われる動きの通りになるように変更しました。

dhq-boiler commented 3 years ago

すごい...単体テストも通ってますし、副作用もどうやらなさそうです。 バグの修正と解説までしていただき、ありがとうございます! これで作業を先に進められそうです。とても助かりました。 Switchメソッドの後のDoメソッドでブレークポイントを貼るテクニックはこれから使っていきます。