Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

2枚目以降のウィンドウでバリデーションエラー赤枠が残る #258

Closed
TN8001 opened this issue Apr 23, 2021 · 10 comments

Comments

@TN8001
Copy link

TN8001 commented Apr 23, 2021

C# - ReactiveProperty バリデーション 画面起動前に値をセットすると赤枠が消えない|teratail

こちらが元なのですが初めに出るWindowは問題ないのですが、ボタンを押して新たにWindowを出すとReactiveProperty側だけ赤枠が出るという症状が出ています(デザイナでも赤枠が出ます)

何か心当たりはあるでしょうか?

無題

以下再現するコードです。

<Window
  x:Class="Questions334418.MainWindow"
  xmlns="http://schemas.microsoft.com/winfx/2006/xaml/presentation"
  xmlns:x="http://schemas.microsoft.com/winfx/2006/xaml"
  xmlns:local="clr-namespace:Questions334418"
  Width="300"
  Height="200">
  <Window.DataContext>
    <local:ViewModel />
  </Window.DataContext>
  <StackPanel VerticalAlignment="Center">
    <Button Margin="5" Click="Button_Click" Content="open" />
    <TextBox Margin="5" Text="{Binding RP_Text.Value, UpdateSourceTrigger=PropertyChanged}" />
    <TextBox Margin="5" Text="{Binding OV_Text, UpdateSourceTrigger=PropertyChanged}" />
  </StackPanel>
</Window>
using CommunityToolkit.Mvvm.ComponentModel;
using Reactive.Bindings;
using System.ComponentModel.DataAnnotations;
using System.Windows;

namespace Questions334418
{
    public partial class MainWindow : Window
    {
        public MainWindow()
        {
            InitializeComponent();
            var vm = DataContext as ViewModel;
            vm.RP_Text.Value = "a";
            vm.OV_Text = "a";
        }

        private void Button_Click(object sender, RoutedEventArgs e) => new MainWindow().Show();
    }

    public class ViewModel : ObservableValidator
    {
        [Required]
        public ReactiveProperty<string> RP_Text { get; }

        [Required]
        public string OV_Text { get => text; set => SetProperty(ref text, value, true); }
        private string text;

        public ViewModel() => RP_Text = new ReactiveProperty<string>().SetValidateAttribute(() => RP_Text);
    }
}
<Project Sdk="Microsoft.NET.Sdk">

  <PropertyGroup>
    <OutputType>WinExe</OutputType>
    <TargetFramework>net5.0-windows</TargetFramework>
    <UseWPF>true</UseWPF>
  </PropertyGroup>

  <ItemGroup>
    <PackageReference Include="CommunityToolkit.Mvvm" Version="7.0.1" />
    <PackageReference Include="ReactiveProperty" Version="7.9.0" />
  </ItemGroup>

</Project>

どう関係するのかわかりませんが、xamlでのDataContextセットをやめてコードで

InitializeComponent();
DataContext = new ViewModel();
var vm = DataContext as ViewModel;
vm.RP_Text.Value = "a";

DataContext = new ViewModel();
var vm = DataContext as ViewModel;
vm.RP_Text.Value = "a";
InitializeComponent();

とすると問題が出ません。

しかし

DataContext = new ViewModel();
InitializeComponent();
var vm = DataContext as ViewModel;
vm.RP_Text.Value = "a";

とすると再発します(デザイナでは赤枠は出ません)

@soi013
Copy link
Contributor

soi013 commented Apr 24, 2021

ViewModelのコンストラクタを以下のように変更することで解決しそうですが、どうでしょうか。

public ViewModel() => RP_Text = new ReactiveProperty<string>(mode: ReactivePropertyMode.Default | ReactivePropertyMode.IgnoreInitialValidationError)
        .SetValidateAttribute(() => RP_Text);

ReactivePropertyMode.IgnoreInitialValidationErrorを有効にします。

参考
MVVM をリアクティブプログラミングで快適に ReactiveProperty オーバービュー 2020 年版 前編

@TN8001
Copy link
Author

TN8001 commented Apr 24, 2021

説明不足ですいません。
元質問の都合でIgnoreInitialValidationErrorはしたくないのです。
そして回避法はわかっています。

なぜ違いが出るのか。
もしも潜在的な不具合が隠れているのであれば報告しておきたい。

というのがこのissueの目的です。

@TN8001
Copy link
Author

TN8001 commented Apr 25, 2021

あれこれやっていて気が付いたのですが、これに関連した制限事項ということなんでしょうか?

UI スレッドへのイベントの自動ディスパッチ

ImmediateSchedulerを指定したところ、同じ挙動になりました!

コードでDataContextを設定するという回避法はあんまりなので、どうにかしたいと考えているのですがこれは解決法として適当でしょうか。

現状をまとめます。

ImmediateSchedulerする前提条件

  • WPFである
    大前提。だがWPF以外では使えないという注意は必要。
  • UIスレッドを複数作らない
    まず作らない。複数作るような奇特な人は、当然わかっていると思われる。

ImmediateSchedulerすべきアプリ条件

  • Window.DataContextViewModelを設定、あるいはViewModelDIしている
    これはかなりの人が該当するはず。
  • IgnoreInitialValidationErrorはしない(初期表示でもエラーが出てほしい)
    これは好みが分かれそう。やや少数派か?
  • 複数Windowを出している
    これが納得いかないのだがWPFの挙動の問題で、ReactivePropertyの関知することではない?

であるならImmediateSchedulerを指定することで同じ挙動になる。
上記を守っている限り一切ペナルティはない

という認識であっていますでしょうか?

@runceel
Copy link
Owner

runceel commented Apr 26, 2021

情報提供ありがとうございます。
以前も、同様の Issue SetValidateAttribute()とウィンドウ表示前のバリデーション(WPF) をいただいて、その時は WPF の挙動のせいであると思いクローズしました。

今回、追加で調査したところ以下のような Scheduler を自前で実装して App クラスの Startup イベントで ReactivePropertyScheduler.SetDefault(new WpfScheduler(Dispatcher)); のように設定することで、意図した通りの挙動になることを確認しました。

お手数をおかけしますが、そちらでも挙動が改善されるか確認してみていただけないでしょうか?

using System;
using System.Reactive.Concurrency;
using System.Reactive.Disposables;
using System.Threading;
using System.Windows.Threading;

namespace Reactive.Bindings.Schedulers
{
    public class WpfScheduler : LocalScheduler
    {
        private readonly Dispatcher _dispatcher;
        private readonly DispatcherSynchronizationContext _context;

        public WpfScheduler(Dispatcher dispatcher)
        {
            _dispatcher = dispatcher;
            _context = new DispatcherSynchronizationContext(dispatcher);
        }

        public override IDisposable Schedule<TState>(TState state, Func<IScheduler, TState, IDisposable> action)
        {
            if (action == null)
            {
                throw new ArgumentNullException(nameof(action));
            }

            if (_dispatcher.CheckAccess())
            {
                return action(this, state);
            }

            var d = new SingleAssignmentDisposable();

            _context.PostWithStartComplete(() =>
            {
                if (!d.IsDisposed)
                {
                    d.Disposable = action(this, state);
                }
            });

            return d;
        }

        public override IDisposable Schedule<TState>(TState state, TimeSpan dueTime, Func<IScheduler, TState, IDisposable> action)
        {
            if (action == null)
            {
                throw new ArgumentNullException(nameof(action));
            }

            var dt = Scheduler.Normalize(dueTime);
            if (dt.Ticks == 0)
            {
                return Schedule(state, action);
            }

            // Note that avoiding closure allocation here would introduce infinite generic recursion over the TState argument
            return DefaultScheduler.Instance.Schedule(state, dt, (_, state1) => Schedule(state1, action));
        }
    }
    internal static class SynchronizationContextExtensions
    {
        public static void PostWithStartComplete<T>(this SynchronizationContext context, Action<T> action, T state)
        {
            context.OperationStarted();

            context.Post(
                o =>
                {
                    try
                    {
                        action((T)o!);
                    }
                    finally
                    {
                        context.OperationCompleted();
                    }
                },
                state
            );
        }

        public static void PostWithStartComplete(this SynchronizationContext context, Action action)
        {
            context.OperationStarted();

            context.Post(
                _ =>
                {
                    try
                    {
                        action();
                    }
                    finally
                    {
                        context.OperationCompleted();
                    }
                },
                null
            );
        }
    }
}

@runceel
Copy link
Owner

runceel commented Apr 26, 2021

以下のような問題の合わせ技で自作 Scheduler を作らないとうまく動きませんでした。

  • WPF のウィンドウ表示前に設定された値に対して PropertyChanged イベントを SynchronizationContext.Current.Post を使って呼び出すと今回のようにバリデーションの表示が意図したものと異なる動きをする
  • デフォルトで ReactivePropertyScheduler が作る SynchronizationContextScheduler はデフォルトで同一スレッド上でも Post になる。
  • 上記を受けて SynchronizationContextScheduler のコンストラクター引数で alwaysPostfalse にして UI スレッド上からの操作では ImmediateScheduler と同じ動きにしようとしても Post されてしまう。
  • WPF 内部で設定される SynchronizationContext.Current に入っている DispatcherSynchronizationContext のインスタンスはアプリケーション内で一意ではなく、何かの単位でインスタンスが変わってしまう。そのため SynchronizationContextScheduleralwaysPostfalse にしても SynchronizationContext.Current のインスタンスが変わってしまうため Post されてしまう。 
  • アプリケーション起動時の UI スレッド上での操作は Post ではなく即座に実行する Scheduler を自作して設定することで問題が起きなくなった。

@TN8001
Copy link
Author

TN8001 commented Apr 26, 2021

お時間を割いていただきありがとうございます。
私の手元では元質問コードを起こしたものも簡略化した再現コードでも、期待通りの動作となりました!

ImmediateSchedulerについてですが、前提条件を守ったとしても使うべきではない
ということですね?

WpfSchedulerについては積極的に使ってほしいのか、問題が出たときのみのものなのか。
どういう理解をしたらいいでしょう?(すいません。内容について1ミリもわかっていません^^;

@runceel
Copy link
Owner

runceel commented Apr 26, 2021

ImmediateScheduler を使った場合は、かならず ReactivePropertyValue を変更させたスレッド上で即座に PropertyChangedErrorChanged イベントが発行されます。そのほかにも ReadOnlyReactiveCollectionReactiveCollection のイベントもそのような動作になります。

WPF の場合は PropertyChanged では問題になることは無いと思いますが、コレクションの変更系イベントは UI スレッド以外で発行されるとバインドしてたらアプリがクラッシュします。
UIスレッド以外ではコレクション操作やプロパティの変更を行わないように作っているなら ImmediateScheduler でも問題ありません。

WpfScheduler については、似たようなものを作って ReactiveProperty.Wpf パッケージに追加してドキュメントにも WPF アプリの場合は WpfScheduler で初期化するように案内を追加しようと考えています。
これは、UI スレッド以外からのコレクション操作やプロパティの変更を UI スレッド上にディスパッチするという機能を正しく動かしつつ今回の問題を起こさない一番良い方法だと思っているからです。

@runceel
Copy link
Owner

runceel commented Apr 26, 2021

余談ですが、最初の Window だと赤枠が出ないのは最初の Window が作成されるタイミングで ReactiveProperty が使う Scheduler が初期化されます。その時に SynchronizationContext.CurrentReactiveProperty 内部で保持するのですが、この時点では Window の SynchronizationContext.Current と同じインスタンスになるため PropertyChanged が即座に実行されて正しく動きます。

2 つ目以降の Window が表示されるときには SynchronizationContext.Current のインスタンスが別のものにさし変わっていて ReactiveProperty 内部で保持している SynchronizationContext のインスタンスと異なります。なので PropertyChanged イベントが遅れて実行されます。そのため赤枠の表示が残ってしまうという問題(何故、遅れてイベントを発行するとうまく動かないのかは WPF の内部を追わないと不明)が起きているようです。

@TN8001
Copy link
Author

TN8001 commented Apr 26, 2021

WPF の場合は PropertyChanged では問題になることは無いと思いますが、コレクションの変更系イベントは UI スレッド以外で発行されるとバインドしてたらアプリがクラッシュします。

なるほどコレクションでまずいのですね。

WpfScheduler については、似たようなものを作って ReactiveProperty.Wpf パッケージに追加してドキュメントにも WPF アプリの場合は WpfScheduler で初期化するように案内を追加しようと考えています。

了解しました。

大変勉強になりました。ありがとうございました。

@TN8001
Copy link
Author

TN8001 commented Apr 27, 2021

ご本人の確認も取れましたので閉じます。

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants