Terminal: Translate all Dispatcher().RunAsync([](){}) lambdas into coroutines

Created on 11 Dec 2019  路  6Comments  路  Source: microsoft/terminal

I believe there is a trivial mechanical translation that'll get us better code clarity and perhaps a better understanding of the flow of ownership through our code.

from

void TerminalPage::_SetFocusedTabIndex(int tabIndex)
{
    auto tab = _tabs.at(tabIndex);
    auto weakThis{ get_weak() };
    _tabView.Dispatcher().RunAsync(CoreDispatcherPriority::Normal, [tab, weakThis]() {
        if (auto page{ weakThis.get() })
        {
            page->_tabView.SelectedItem(tab->GetTabViewItem());
        }
    });
}

to

winrt::fire_and_forget TerminalPage::_SetFocusedTabIndex(int tabIndex)
{
    // GH#1117: This is a workaround because _tabView.SelectedIndex(tabIndex)
    //          sometimes set focus to an incorrect tab after removing some tabs
    auto tab = _tabs.at(tabIndex);
    auto weakThis{ get_weak() };

    co_await winrt::resume_foreground(_tabView.Dispatcher()); // transition to the dispatcher thread

    if (auto page{ weakThis.get() })
    {
        page->_tabView.SelectedItem(tab->GetTabViewItem());
    }
}

There's a bunch more information about concurrency and coroutines in C++/WinRT here.

Area-CodeHealth Help Wanted Issue-Task Product-Terminal Resolution-Fix-Committed

All 6 comments

Not to be in the business of nominating community members out of the blue to do labor on our behalf, but I really think @mkitzan might be great here: he's looked at a lot of ownership issues and dispatch queues lately, and this could be right up his alley.

Overall this does look better, though I'd want to be careful with this - the first pattern is a little more obvious when we're accidentally using this within the body of the dispatched lambda, while the second will make it easier for us to forget that we shouldn't be using this there.

Unless there's also another way for us to resume_foreground_unless_this_goes_away or something

To be fair, using this is totally valid after the weak ref is locked. It may even be preferred as it lets us avoid having to access member variables on someOtherObject->

There's some suggestions here, actually: instead of making these _weak_, we can make them _strong_ and perform the capture before the co_await.

Safely accessing the _this_ pointer in a class-member coroutine

Oh my gosh, it gets even better. Check this out!

For a weak reference, call get_weak. C++/WinRT ensures that the resulting delegate holds a weak reference. At the last minute, and behind the scenes, the delegate attempts to resolve the weak reference to a strong one, and only calls the member function if it's successful.

event_source.Event({ get_weak(), &EventRecipient::OnEvent });

Woah WinRT has it all! I'd be up to rework the Dispatcher().RunAsync calls to whatever pattern is decided on. I think the co routine pattern + get_strong() would be the most readable/understandable.
~Also, I've been trying to turn one of the TermControl instances to the { get_weak(), &EventRecipient::OnEvent } form. However, the pointer to member form doesn't capture information about the arguments to provide the function and probably has to have specific parameters.~

Was this page helpful?
0 / 5 - 0 ratings