Fix focusFollowMouse

[zadjii-msft]

Because this looks like it's entirely broken in main, and possibly in 1.17(?)

We didn't take a strong ref to the coroutine parameter. As to be expected, that explodes.

Closes #15412

[zadjii-msft]

@lhecker you have any clever ideas on how to like, do this with some sort of smart template? Like, I want to just say

events.titleToken = control.TitleChanged(til::OnForeground{ get_weak(), TabViewItem().Dispatcher(), &TerminalTab::_controlTitleChanged });

But methinks that wouldn't work, cause I'd have to keep the til::OnForeground somewhere to keep the inner function pointer alive.

[lhecker]

To be entirely honest I prefer the previous code, because while it's redundant it's more concise than having separate functions for this. I feel like it's easier to read and follow that way.

Hmm I think you could use a function generator for this. Something like this:

const auto generator = [dispatcher, weakThis](auto func) {
    return [=](auto sender, auto&&) -> winrt::fire_and_forget {
        co_await wil::resume_foreground(dispatcher);

        if (const auto tab = weakThis.get())
        {
            if constexpr (std::is_invocable_v<decltype(func), TerminalTab, IInspectable>)
            {
                (tab.get()->*func)(sender);
            }
            else
            {
                (tab.get()->*func)();
            }
        }
    };
};

events.titleToken = control.TitleChanged(generator(&TerminalTab::UpdateTitle));
events.colorToken = control.TabColorChanged(generator(&TerminalTab::_RecalculateAndApplyTabColor));
events.taskbarToken = control.SetTaskbarProgress(generator(&TerminalTab::_UpdateProgressState));
events.readOnlyToken = control.ReadOnlyChanged(generator(&TerminalTab::_RecalculateAndApplyReadOnly));
events.readOnlyToken = control.FocusFollowMouseRequested(generator(&TerminalTab::_FocusFollowMouseRequested));

void TerminalTab::_FocusFollowMouseRequested(const IInspectable& sender)
{
    if (_focusState != FocusState::Unfocused)
    {
        if (const auto termControl{ sender.try_as<winrt::Microsoft::Terminal::Control::TermControl>() })
        {
            termControl.Focus(FocusState::Pointer);
        }
    }
}

But I'm not 100% sure if it's better than the current code. The current code is very verbose, but it's easy to follow and requires no special knowledge. Everything's very close to where it's being used thanks to the lambdas. Additionally in a crash, the line numbers would give a quick indication which lambda failed, whereas the above generator or an equal til::OnForeground would not.

[lhecker]

A til::OnForeground struct would look very similar btw. Like this basically (pseudocode):

template<typename Weak, typename Func>
struct OnForeground {
    OnForeground(Weak weak, CoreDispatcher dispatcher, Func func) :
        _weak{ std::move(weak) },
        _dispatcher{ std::move(dispatcher) },
        _func{ std::move(func) }
    {}

    void operator()(auto sender, auto&&) const -> winrt::fire_and_forget
    {
        co_await wil::resume_foreground(dispatcher);

        if (const auto strong = _weak.get())
        {
            if constexpr (std::is_invocable_v<decltype(func), decltype(strong), decltype(sender)>)
            {
                (strong.get()->*_func)(sender);
            }
            else
            {
                (strong.get()->*_func)();
            }
        }
    }

private:
    Weak _weak;
    CoreDispatcher _dispatcher;
    Func _func;
};

This is basically identical to what the compiler generators for the "generator" lambdas. It has the same downsides too. It's a bit like a std::mem_fn, whose use is equally discouraged, because all these wrappers use dynamic dispatch for func, whereas the lambdas use static dispatch.

[zadjii-msft]

These are all good points. I can swap back to the original format. I've been trying to move away from these lambda callbacks wherever possible, but seeing how all these methods were just b o i l e r p l a t e , they really don't make sense.

Unless I we had something dumb like

winrt::Windows::Foundation::IAsyncOperation<T> _toFg(weak_ref<T> weak, CoreDispatcher dispatcher)
{
    co_await wil::resume_foreground(dispatcher);
	co_return weak.get();
}
winrt::fire_and_forget TerminalTab::_controlReadOnlyChanged(Windows::Foundation::IInspectable sender, Windows::Foundation::IInspectable e)
{
    if (auto tab{ co_await _toFg(get_weak(), TabViewItem().Dispatcher()) })
    {
        tab->_RecalculateAndApplyReadOnly();
    }
}

nah that's not really helping either.

[DHowett]

I don't understand totally - what state is this change in? I would love to merge in a very surgical fix for 1.17.

[DHowett]

(I am also cool with this fix if it isn't exactly surgical and just pulls things out into member functions. As long as it can be backported cleanly.)

[DHowett]

YIKES THAT IS SUBTLE

To be clear: i am 100% okay with your bigger fix, I just wanted to make sure it didn't have any unintended side effects that would be hard to review.

Do you want to bring that one in to main in a different PR?