Caliburn-Micro / Caliburn.Micro

A small, yet powerful framework, designed for building applications across all XAML platforms. Its strong support for MV* patterns will enable you to build your solution quickly, without the need to sacrifice code quality or testability.
http://caliburnmicro.com/
MIT License
2.79k stars 776 forks source link

ShowPopupAsync repeatedly binds ViewModel to View #798

Open giosali opened 2 years ago

giosali commented 2 years ago

Overview

Repeatedly calling WindowManager.ShowPopupAsync seems to repeatedly bind the provided ViewModel to its View on top of previous binds. This can lead to a situation where binded methods will be called the same number of times that ShowPopupAsync was called with the same ViewModel.

Platform

Steps to Reproduce

I've created a dummy project with everything already set up if you'd like to use that instead. If you choose to use the dummy project, skip ahead to step 7.

  1. Create a WPF project using .NET 6
  2. Create the following project layout:
Project layout
DummyWpfApp/
    ViewModels/
        PopupViewModel.cs
        ShellViewModel.cs
    Views/
        PopupView.xaml  # This is a UserControl
        ShellView.xaml
    App.xaml
    Bootstrapper.cs
  1. Your ShellViewModel.cs file should resemble the following:
public class ShellViewModel : Conductor<object>
    {
        public ShellViewModel()
        {
        }

        private PopupViewModel Popup { get; set; } = new();

        public async void OpenPopup(object sender, RoutedEventArgs e)
        {
            if (Popup.IsActive)
            {
                Debug.WriteLine("OpenPopup: Popup.IsActive = true ... ShowOrHide()");
                Popup.ShowOrHide();
            }
            else
            {
                Debug.WriteLine("OpenPopup: Popup.IsActive = false ... WindowManager()");
                IWindowManager manager = new WindowManager();
                await manager.ShowPopupAsync(Popup);
            }
        }
    }
  1. In your ShellView.xaml file, place a Button and bind it to OpenPopup in ShellViewModel.cs:
<Window x:Class="DummyWpfApp.Views.ShellView"
        xmlns="http://schemas.microsoft.com/winfx/2006/xaml/presentation"
        xmlns:x="http://schemas.microsoft.com/winfx/2006/xaml"
        xmlns:d="http://schemas.microsoft.com/expression/blend/2008"
        xmlns:mc="http://schemas.openxmlformats.org/markup-compatibility/2006"
        xmlns:local="clr-namespace:DummyWpfApp.Views"
        xmlns:cal="http://caliburnmicro.com"
        mc:Ignorable="d"
        Title="ShellView" Height="450" Width="450">
    <StackPanel>
        <Button Content="Open Popup"
                cal:Message.Attach="[Event Click] = [Action OpenPopup($this, $eventargs)]"/>
    </StackPanel>
</Window>
  1. Open the PopupViewModel.cs file and enter the following:
public class PopupViewModel : Screen
    {
        private string _userInput;

        public PopupViewModel()
        {
        }

        public string UserInput
        {
            get => _userInput;
            set
            {
                _userInput = value;
                NotifyOfPropertyChange(() => UserInput);
            }
        }

        public void TextBox_TextChanged(object sender, RoutedEventArgs e)
        {
            Debug.WriteLine("TextBox_TextChanged CALLED");
        }

        public void Close(object sender, RoutedEventArgs e)
        {
            ShowOrHide();
        }

        public void ShowOrHide()
        {
            if (GetView() is Popup popup)
            {
                popup.IsOpen = !popup.IsOpen;
            }
        }
    }
  1. Have your PopupView.xaml file resemble the following:
<UserControl x:Class="DummyWpfApp.Views.PopupView"
             xmlns="http://schemas.microsoft.com/winfx/2006/xaml/presentation"
             xmlns:x="http://schemas.microsoft.com/winfx/2006/xaml"
             xmlns:mc="http://schemas.openxmlformats.org/markup-compatibility/2006" 
             xmlns:d="http://schemas.microsoft.com/expression/blend/2008" 
             xmlns:local="clr-namespace:DummyWpfApp.Views"
             xmlns:cal="http://caliburnmicro.com"
             mc:Ignorable="d" 
             d:DesignHeight="450" d:DesignWidth="800"
             Width="100"
             Height="100">
    <StackPanel>
        <TextBox x:Name="UserInput"
                 cal:Message.Attach="[Event TextChanged] = [Action TextBox_TextChanged($this, $eventargs)]"/>
        <Button Content="Close"
                cal:Message.Attach="[Event Click] = [Action Close($this, $eventargs)]"/>
    </StackPanel>
</UserControl>

The most important part in this step is to make sure that the TextBox control's x:Name directive is set to UserInput so that it binds to the same-name property in the PopupViewModel.cs file.

  1. Build and run the project
  2. When the ShellView window opens, click on the button that says "Open Popup", and type a single letter into the TextBox of the PopupView window. TextBox_TextChanged CALLED will appear once in the output for Debug. Click the "Close" button in the PopupView window
  3. Repeat step 8, except this time, take note of the number of times TextBox_TextChanged is printed to the Debug output each time you type. It should be 2 times per keystroke
  4. Repeat step 8 again and now TextBox_TextChanged will be printed 3 times per keystroke. For every time you repeat step 8, that is how many times the message will be printed to the output

Expected Behavior

The TextChanged event of a TextBox with an x:Name directive should not be called for each time that WindowManager.ShowPopupAsync has been called with a given ViewModel.

Additional Remarks

I'm not sure if this is the intended behavior. While playing around with WindowManager.ShowWindowAsync, I found that this same behavior doesn't occur which leads me to believe it's specific to ShowPopupAsync. Interestingly enough, if you don't include the x:Name directive for the TextBox, the TextChanged method will only be called once regardless of how many times you've called ShowPopupAsync.

KasperSK commented 2 years ago

I think it might be the binding that causes the TextChanged event to fire twice. I cannot explain why it only happens the second time the popup is opened. I will have a look at it.

KasperSK commented 2 years ago

I had a look at your repo and I can see that it does indeed happen, however if I continued to open and close the popup it stopped again.

giosali commented 2 years ago

I think I see what you mean. It doesn't seem to "rebind" each time the popup opens; instead, it only rebinds each time you open the popup and type into the TextBox in the popup.

I also added a ListBox control and binded it's SelectedItem property to a property in the ViewModel and the same issue happens with the SelectionChanged event. Each time you open the popup and interact with the items in the ListBox, you'll see the number of times the event gets called increases each time the popup opens and you change the selected item.

So after playing with it a while longer, I think I found a solution but it seems a little hackish. I'm using the BindingOperations.ClearBinding method inside of the Unloaded event of a control that contains any bindings to the ViewModel.

Here's an example with the TextBox:

<TextBox Text="{Binding UserInput, Mode=TwoWay, UpdateSourceTrigger=PropertyChanged}"
         cal:Message.Attach="[Event TextChanged] = [Action TextBox_TextChanged($this, $eventargs)];
                             [Event Unloaded] = [Action TextBox_Unloaded($source, $eventargs)]"/>

Then, inside of the ViewModel:

public void TextBox_Unloaded(object sender, RoutedEventArgs e)
{
    BindingOperations.ClearBinding(sender as TextBox, TextBox.TextProperty);
}

Now whenever I type into the TextBox (regardless of how many times I've opened it and typed into it), the TextChanged event only gets called once. What do you think though? Should we close the issue or leave it open?

KasperSK commented 2 years ago

Interesting findings, lets leave the issue open I will look at WindowManager and see if I can spot the difference between Popup and Window.

eeevans commented 2 years ago

The issue is that the two types are handled differently by Caliburn.Micro and the Windows Popup control that is used under the hood.

In essence when you set popup.IsOpen to false, it actually closes the underlying windows Popup control and removes the View from the ViewModel. So your Popup.IsActive check on the PopupViewModel in ShellViewModel is never true. So you call ShowPopupAsync again which will fire the Message.Attach again when the View is bound to the Popup windows control. I suspect Windows sometimes reuses the Popup control that has the bindings to the click events and wires up another set. I say that because sometimes I would see it would fire the number of times I had shown the popup and other times it would go back down to 1 even though I had open/close it multiple times. That is just a guess though.

This does not happen for hiding a window so the Window.IsActive works as you expect.

Here are the relevant lines in WindowManager that are different for each type.

        public virtual async Task ShowWindowAsync(object rootModel, object context = null, IDictionary<string, object> settings = null)
        {
            NavigationWindow navWindow = null;

            var application = Application.Current;
            if (application != null && application.MainWindow != null)
            {
                navWindow = application.MainWindow as NavigationWindow;
            }

            if (navWindow != null)
            {
                var window = await CreatePageAsync(rootModel, context, settings);
                navWindow.Navigate(window);
            }
            else
            {
                var window = await CreateWindowAsync(rootModel, false, context, settings);

                window.Show();
            }
        }

        /// <summary>
        /// Shows a popup at the current mouse position.
        /// </summary>
        /// <param name="rootModel">The root model.</param>
        /// <param name="context">The view context.</param>
        /// <param name="settings">The optional popup settings.</param>
        public virtual async Task ShowPopupAsync(object rootModel, object context = null, IDictionary<string, object> settings = null)
        {
            var popup = CreatePopup(rootModel, settings);
            var view = ViewLocator.LocateForModel(rootModel, popup, context);

            popup.Child = view;
            popup.SetValue(View.IsGeneratedProperty, true);

            ViewModelBinder.Bind(rootModel, popup, null);
            Action.SetTargetWithoutContext(view, rootModel);

            if (rootModel is IActivate activator)
            {
                await activator.ActivateAsync();
            }

            if (rootModel is IDeactivate deactivator)
            {
                popup.Closed += async (s, e) => await deactivator.DeactivateAsync(true);
            }

            popup.IsOpen = true;
            popup.CaptureMouse();
        }

Then in Screen

        async Task IDeactivate.DeactivateAsync(bool close, CancellationToken cancellationToken)
        {
            if (IsActive || IsInitialized && close)
            {
                AttemptingDeactivation?.Invoke(this, new DeactivationEventArgs
                {
                    WasClosed = close
                });

                Log.Info("Deactivating {0}.", this);
                await OnDeactivateAsync(close, cancellationToken);
                IsActive = false;

                await (Deactivated?.InvokeAllAsync(this, new DeactivationEventArgs
                {
                    WasClosed = close
                }) ?? Task.FromResult(true));

                if (close)
                {
                    Views.Clear();
                    Log.Info("Closed {0}.", this);
                }
            }
        }

And in Popup

        // Close the window
        private void HideWindow()
        {
            bool animating = SetupAnimations(false);

            SetHitTestable(false);
            ReleasePopupCapture();

            // NOTE: It is important that we destroy the windows at less than Render priority because Menus will allow
            //       all Render-priority queue items to be processed before firing the click event and we don't want
            //       to have disposed the window at the time that we route the event.
            //       Setting to inactive to allow any animations in ShowWindow to take effect first.

            _asyncDestroy = new DispatcherTimer(DispatcherPriority.Input);
            _asyncDestroy.Tick += delegate(object sender, EventArgs args)
            {
                _asyncDestroy.Stop();
                _asyncDestroy = null;

                DestroyWindow();
            };

            // Wait for the animation (if any) to complete before destroying the window
            _asyncDestroy.Interval = animating ? AnimationDelayTime : TimeSpan.Zero;
            _asyncDestroy.Start();

            if (!animating)
                _secHelper.HideWindow();
        }
vb2ae commented 2 years ago

I added the following class to the project on my local win 11 computer from your repo. Added a DebugLog so I can see what Caliburn.Micro is doing

public class DebugLog : ILog
{
    private const string ErrorText = "ERROR";
    private const string WarnText = "WARN";
    private const string InfoText = "INFO";
    private const string DebugText = "DEBUG";
    private readonly Type logType;

    public DebugLog(Type type)
    {
        logType = type;
    }
    private string CreateLogMessage(string format, params object[] args)
    {
        return string.Format("[{0}] {1}", DateTime.Now.ToString("o"), string.Format(format, args));
    }

    public void Error(Exception exception)
    {
        Trace.WriteLine(CreateLogMessage(exception.ToString()), ErrorText);
    }

    public void Info(string format, params object[] args)
    {
        Trace.WriteLine(CreateLogMessage(format, args), InfoText);
    }
    public void Warn(string format, params object[] args)
    {
        Trace.WriteLine(CreateLogMessage(format, args), WarnText);
    }

    public void Error(string format, params object[] args)
    {
        Trace.WriteLine(CreateLogMessage(format, args), ErrorText);
    }

    public void Debug(string format, params object[] args)
    {
        Trace.WriteLine(CreateLogMessage(format, args), DebugText);
    }

}

changed the bootstrapper constructor to this to get the Caliburn.Micro logging

    public Bootstrapper()
    {
        LogManager.GetLog = type => new DebugLog(type);

        Initialize();
    }

I changed the PopupViewModel event handler a little

    public void TextBox_TextChanged(object sender, RoutedEventArgs e)
    {
        Debug.WriteLine($"TextBox_TextChanged CALLED popup {UserInput}");
    }

And the ShowOrHide method

   public void ShowOrHide()
    {
        if (GetView() is Popup popup)
        {
          Debug.WriteLine($"Show or hide popup {!popup.IsOpen}");
          popup.IsOpen = !popup.IsOpen;
        }
    }

Not see what you are describing

TextBox_TextChanged CALLED popup t INFO: [2022-03-30T11:45:27.2820032-04:00] Action: TextBox_TextChanged availability update. INFO: [2022-03-30T11:45:27.2857600-04:00] Invoking Action: TextBox_TextChanged. TextBox_TextChanged CALLED popup te INFO: [2022-03-30T11:45:27.6935995-04:00] Action: TextBox_TextChanged availability update. INFO: [2022-03-30T11:45:27.6978991-04:00] Invoking Action: TextBox_TextChanged. TextBox_TextChanged CALLED popup tex INFO: [2022-03-30T11:45:27.7539689-04:00] Action: TextBox_TextChanged availability update. INFO: [2022-03-30T11:45:27.7575561-04:00] Invoking Action: TextBox_TextChanged. TextBox_TextChanged CALLED popup text

INFO: [2022-03-30T11:45:29.0565915-04:00] Action: TextBox_TextChanged availability update. INFO: [2022-03-30T11:45:29.0599117-04:00] Invoking Action: TextBox_TextChanged. TextBox_TextChanged CALLED popup text changed INFO: [2022-03-30T11:45:30.7793605-04:00] Invoking Action: Close. Show or hide popup False INFO: [2022-03-30T11:45:30.8084843-04:00] Deactivating DummyWpfApp.ViewModels.PopupViewModel. INFO: [2022-03-30T11:45:30.8135662-04:00] Closed DummyWpfApp.ViewModels.PopupViewModel. INFO: [2022-03-30T11:45:32.2127672-04:00] Invoking Action: OpenPopup. OpenPopup: Popup.IsActive = false ... WindowManager() INFO: [2022-03-30T11:45:32.2205341-04:00] Binding System.Windows.Controls.Primitives.Popup and DummyWpfApp.ViewModels.PopupViewModel. INFO: [2022-03-30T11:45:32.2233436-04:00] Setting DC of System.Windows.Controls.Primitives.Popup to DummyWpfApp.ViewModels.PopupViewModel. INFO: [2022-03-30T11:45:32.2261563-04:00] Attaching message handler DummyWpfApp.ViewModels.PopupViewModel to System.Windows.Controls.Primitives.Popup. INFO: [2022-03-30T11:45:32.2286556-04:00] Attaching System.Windows.Controls.Primitives.Popup to DummyWpfApp.ViewModels.PopupViewModel. INFO: [2022-03-30T11:45:32.2314959-04:00] Action Convention Not Applied: No actionable element for get_UserInput. INFO: [2022-03-30T11:45:32.2340190-04:00] Action Convention Not Applied: No actionable element for set_UserInput. INFO: [2022-03-30T11:45:32.2370251-04:00] Action Convention Not Applied: No actionable element for TextBox_TextChanged. INFO: [2022-03-30T11:45:32.2398947-04:00] Action Convention Not Applied: No actionable element for Close. INFO: [2022-03-30T11:45:32.2427484-04:00] Action Convention Not Applied: No actionable element for ShowOrHide. INFO: [2022-03-30T11:45:32.2453822-04:00] Action Convention Not Applied: No actionable element for get_IsInitialized. INFO: [2022-03-30T11:45:32.2486987-04:00] Action Convention Not Applied: No actionable element for get_Parent. INFO: [2022-03-30T11:45:32.2518293-04:00] Action Convention Not Applied: No actionable element for set_Parent. INFO: [2022-03-30T11:45:32.2556339-04:00] Action Convention Not Applied: No actionable element for get_DisplayName. INFO: [2022-03-30T11:45:32.2587693-04:00] Action Convention Not Applied: No actionable element for set_DisplayName. INFO: [2022-03-30T11:45:32.2623181-04:00] Action Convention Not Applied: No actionable element for get_IsActive. INFO: [2022-03-30T11:45:32.2652638-04:00] Action Convention Not Applied: No actionable element for add_Activated. INFO: [2022-03-30T11:45:32.2682868-04:00] Action Convention Not Applied: No actionable element for remove_Activated. INFO: [2022-03-30T11:45:32.2718551-04:00] Action Convention Not Applied: No actionable element for add_AttemptingDeactivation. INFO: [2022-03-30T11:45:32.2750084-04:00] Action Convention Not Applied: No actionable element for remove_AttemptingDeactivation. INFO: [2022-03-30T11:45:32.2779627-04:00] Action Convention Not Applied: No actionable element for add_Deactivated. INFO: [2022-03-30T11:45:32.2808755-04:00] Action Convention Not Applied: No actionable element for remove_Deactivated. INFO: [2022-03-30T11:45:32.2845029-04:00] Action Convention Not Applied: No actionable element for CanCloseAsync. INFO: [2022-03-30T11:45:32.2881333-04:00] Action Convention Not Applied: No actionable element for TryCloseAsync. INFO: [2022-03-30T11:45:32.2919307-04:00] Action Convention Not Applied: No actionable element for add_ViewAttached. INFO: [2022-03-30T11:45:32.2955208-04:00] Action Convention Not Applied: No actionable element for remove_ViewAttached. INFO: [2022-03-30T11:45:32.2992612-04:00] Action Convention Not Applied: No actionable element for GetView. INFO: [2022-03-30T11:45:32.3033893-04:00] Action Convention Not Applied: No actionable element for add_PropertyChanged. INFO: [2022-03-30T11:45:32.3073270-04:00] Action Convention Not Applied: No actionable element for remove_PropertyChanged. INFO: [2022-03-30T11:45:32.3109295-04:00] Action Convention Not Applied: No actionable element for get_IsNotifying. INFO: [2022-03-30T11:45:32.3144317-04:00] Action Convention Not Applied: No actionable element for set_IsNotifying. INFO: [2022-03-30T11:45:32.3181319-04:00] Action Convention Not Applied: No actionable element for Refresh. INFO: [2022-03-30T11:45:32.3216813-04:00] Action Convention Not Applied: No actionable element for NotifyOfPropertyChange. INFO: [2022-03-30T11:45:32.3252953-04:00] Action Convention Not Applied: No actionable element for NotifyOfPropertyChange. INFO: [2022-03-30T11:45:32.3288305-04:00] Action Convention Not Applied: No actionable element for Set. INFO: [2022-03-30T11:45:32.3325245-04:00] Action Convention Not Applied: No actionable element for GetType. INFO: [2022-03-30T11:45:32.3361940-04:00] Action Convention Not Applied: No actionable element for ToString. INFO: [2022-03-30T11:45:32.3400261-04:00] Action Convention Not Applied: No actionable element for Equals. INFO: [2022-03-30T11:45:32.3437012-04:00] Action Convention Not Applied: No actionable element for GetHashCode. INFO: [2022-03-30T11:45:32.3488103-04:00] Binding Convention Applied: Element UserInput. INFO: [2022-03-30T11:45:32.3529774-04:00] Attaching message handler DummyWpfApp.ViewModels.PopupViewModel to DummyWpfApp.Views.PopupView. INFO: [2022-03-30T11:45:32.3566793-04:00] Activating DummyWpfApp.ViewModels.PopupViewModel. INFO: [2022-03-30T11:45:32.3944185-04:00] Action: TextBox_TextChanged availability update. INFO: [2022-03-30T11:45:32.3967812-04:00] Action: TextBox_TextChanged availability update. INFO: [2022-03-30T11:45:32.4002181-04:00] Action: Close availability update. INFO: [2022-03-30T11:45:32.4029088-04:00] Action: Close availability update. INFO: [2022-03-30T11:45:34.0504182-04:00] Invoking Action: TextBox_TextChanged. TextBox_TextChanged CALLED popup text changed INFO: [2022-03-30T11:45:34.0556309-04:00] Action: TextBox_TextChanged availability update. INFO: [2022-03-30T11:45:34.0580702-04:00] Action: TextBox_TextChanged availability update. INFO: [2022-03-30T11:45:34.0604466-04:00] Invoking Action: TextBox_TextChanged. TextBox_TextChanged CALLED popup text changed INFO: [2022-03-30T11:45:34.4183953-04:00] Invoking Action: TextBox_TextChanged. TextBox_TextChanged CALLED popup text changed a INFO: [2022-03-30T11:45:34.4266935-04:00] Action: TextBox_TextChanged availability update. INFO: [2022-03-30T11:45:34.4302549-04:00] Action: TextBox_TextChanged availability update. INFO: [2022-03-30T11:45:34.4334045-04:00] Invoking Action: TextBox_TextChanged. TextBox_TextChanged CALLED popup text changed a INFO: [2022-03-30T11:45:34.6427975-04:00] Invoking Action: TextBox_TextChanged. TextBox_TextChanged CALLED popup text changed ag INFO: [2022-03-30T11:45:34.6503927-04:00] Action: TextBox_TextChanged availability update. INFO: [2022-03-30T11:45:34.6541212-04:00] Action: TextBox_TextChanged availability update. INFO: [2022-03-30T11:45:34.6581704-04:00] Invoking Action: TextBox_TextChanged. TextBox_TextChanged CALLED popup text changed ag INFO: [2022-03-30T11:45:34.8431923-04:00] Invoking Action: TextBox_TextChanged. TextBox_TextChanged CALLED popup text changed aga

If you are getting different results please post them