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

Add an action for identifying windows #9523

Merged
33 commits merged into from
Mar 30, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
33 commits
Select commit Hold shift + click to select a range
1c7da00
Rebase all the changes on main
zadjii-msft Feb 25, 2021
97818c6
fix a bug and fix the tests
zadjii-msft Feb 25, 2021
2ec9415
fix tests
zadjii-msft Feb 25, 2021
e13e1e7
Good ole Java
zadjii-msft Feb 25, 2021
136ce6d
finish that test
zadjii-msft Feb 25, 2021
ec97c43
macros are life
zadjii-msft Feb 26, 2021
fa26f7f
THIS NEEDS TO GO TO THE PARENT
zadjii-msft Feb 26, 2021
a391455
Plumb the events up and down
zadjii-msft Feb 26, 2021
001f545
Bind the labels to the actual TerminalPage object
zadjii-msft Mar 1, 2021
9270e0f
bind the name, id down to the actual page
zadjii-msft Mar 1, 2021
1f0dceb
add an action for just identifying one single window
zadjii-msft Mar 1, 2021
e238daa
THIS SHOULD GO TO THE PARENT BRANCH
zadjii-msft Mar 2, 2021
27d12a2
THIS NEEDS TO GO TO THE PARENT
zadjii-msft Feb 26, 2021
9d6f47f
yea you better believe the Toast just worked
zadjii-msft Mar 2, 2021
c202257
feedback from review
zadjii-msft Mar 4, 2021
eaaa204
initial review feedback
zadjii-msft Mar 16, 2021
9f54229
Merge remote-tracking branch 'origin/main' into dev/migrie/f/name-win…
zadjii-msft Mar 16, 2021
bba09e3
Address PR concerns
zadjii-msft Mar 16, 2021
0799a19
Merge branch 'dev/migrie/f/name-windows-3' into dev/migrie/f/identify…
zadjii-msft Mar 17, 2021
1105328
That's better
zadjii-msft Mar 17, 2021
97b4935
Cleanup for review
zadjii-msft Mar 17, 2021
765c969
Merge branch 'dev/migrie/f/name-windows-3' into dev/migrie/f/identify…
zadjii-msft Mar 17, 2021
b66503c
Merge remote-tracking branch 'origin/main' into dev/migrie/f/identify…
zadjii-msft Mar 17, 2021
b7703d2
Merge remote-tracking branch 'origin/main' into dev/migrie/f/identify…
zadjii-msft Mar 17, 2021
d035e50
Merge remote-tracking branch 'origin/main' into dev/migrie/f/identify…
zadjii-msft Mar 17, 2021
d64aa61
Merge remote-tracking branch 'origin/main' into dev/migrie/f/identify…
zadjii-msft Mar 24, 2021
8b6e0bc
I'm overcomplicating this
zadjii-msft Mar 24, 2021
7c775d5
Revert "I'm overcomplicating this"
zadjii-msft Mar 24, 2021
b1b94f6
Mainly: delayload the TeachingTip
zadjii-msft Mar 24, 2021
31cb1b7
This is the thing dustin suggested, and I like it
zadjii-msft Mar 24, 2021
5904c58
This will make @dhowett happy
zadjii-msft Mar 26, 2021
2ff4c60
Merge remote-tracking branch 'origin/main' into dev/migrie/f/identify…
zadjii-msft Mar 30, 2021
ff2ce4b
runformat
zadjii-msft Mar 30, 2021
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
60 changes: 60 additions & 0 deletions src/cascadia/Remoting/Monarch.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -75,6 +75,7 @@ namespace winrt::Microsoft::Terminal::Remoting::implementation
auto newPeasantsId = peasant.GetID();
// Add an event listener to the peasant's WindowActivated event.
peasant.WindowActivated({ this, &Monarch::_peasantWindowActivated });
peasant.IdentifyWindowsRequested({ this, &Monarch::_identifyWindows });

_peasants[newPeasantsId] = peasant;

Expand Down Expand Up @@ -571,4 +572,63 @@ namespace winrt::Microsoft::Terminal::Remoting::implementation
result->WindowName(targetWindowName);
return *result;
}

// Method Description:
// - Helper for doing something on each and every peasant, with no regard
// for if the peasant is living or dead.
// - We'll try calling callback on every peasant.
// - If any single peasant is dead, then we'll call errorCallback, and move on.
// - We're taking an errorCallback here, because the thing we usually want
// to do is TraceLog a message, but TraceLoggingWrite is actually a macro
// that _requires_ the second arg to be a string literal. It can't just be
// a variable.
// Arguments:
// - callback: The function to call on each peasant
// - errorCallback: The function to call if a peasant is dead.
// Return Value:
// - <none>
void Monarch::_forAllPeasantsIgnoringTheDead(std::function<void(const Remoting::IPeasant&, const uint64_t)> callback,
std::function<void(const uint64_t)> errorCallback)
{
for (const auto& [id, p] : _peasants)
{
try
{
callback(p, id);
}
catch (...)
{
LOG_CAUGHT_EXCEPTION();
// If this fails, we don't _really_ care. Just move on to the
// next one. Someone else will clean up the dead peasant.
errorCallback(id);
}
}
}

// Method Description:
// - This is an event handler for the IdentifyWindowsRequested event. A
// Peasant may raise that event if they want _all_ windows to identify
// themselves.
// - This will tell each and every peasant to identify themselves. This will
// eventually propagate down to TerminalPage::IdentifyWindow.
// Arguments:
// - <unused>
// Return Value:
// - <none>
void Monarch::_identifyWindows(const winrt::Windows::Foundation::IInspectable& /*sender*/,
const winrt::Windows::Foundation::IInspectable& /*args*/)
{
// Notify all the peasants to display their ID.
auto callback = [](auto&& p, auto&& /*id*/) {
p.DisplayWindowId();
};
Comment on lines +623 to +625
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wait... if we're not using id, should we just remove it from the first param in _forAllPeasantsIgnoringTheDead?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mike: writes generic function

Reviewers: is it possibly too generic?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Meh, I presumed a future user of this helper might want it.

auto onError = [](auto&& id) {
TraceLoggingWrite(g_hRemotingProvider,
"Monarch_identifyWindows_Failed",
TraceLoggingInt64(id, "peasantID", "The ID of the peasant which we could not identify"),
TraceLoggingLevel(WINEVENT_LEVEL_VERBOSE));
};
_forAllPeasantsIgnoringTheDead(callback, onError);
}
}
5 changes: 5 additions & 0 deletions src/cascadia/Remoting/Monarch.h
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,11 @@ namespace winrt::Microsoft::Terminal::Remoting::implementation
void _doHandleActivatePeasant(const winrt::com_ptr<winrt::Microsoft::Terminal::Remoting::implementation::WindowActivatedArgs>& args);
void _clearOldMruEntries(const uint64_t peasantID);

void _identifyWindows(const winrt::Windows::Foundation::IInspectable& sender,
const winrt::Windows::Foundation::IInspectable& args);

void _forAllPeasantsIgnoringTheDead(std::function<void(const winrt::Microsoft::Terminal::Remoting::IPeasant&, const uint64_t)> callback,
std::function<void(const uint64_t)> errorCallback);
friend class RemotingUnitTests::RemotingTests;
};
}
Expand Down
47 changes: 47 additions & 0 deletions src/cascadia/Remoting/Peasant.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -117,4 +117,51 @@ namespace winrt::Microsoft::Terminal::Remoting::implementation
return _lastActivatedArgs;
}

// Method Description:
// - Tell this window to display it's window ID. We'll raise a
// DisplayWindowIdRequested event, which will get handled in the AppHost,
// and used to tell the app to display the ID toast.
// Arguments:
// - <none>
// Return Value:
// - <none>
void Peasant::DisplayWindowId()
{
// Not worried about try/catching this. The handler is in AppHost, which
// is in-proc for us.
_DisplayWindowIdRequestedHandlers(*this, nullptr);
}

// Method Description:
// - Raises an event to ask that all windows be identified. This will come
// back to us when the Monarch handles the event and calls our
// DisplayWindowId method.
// Arguments:
// - <none>
// Return Value:
// - <none>
void Peasant::RequestIdentifyWindows()
{
bool successfullyNotified = false;

try
{
// Try/catch this, because the other side of this event is handled
// by the monarch. The monarch might have died. If they have, this
// will throw an exception. Just eat it, the election thread will
// handle hooking up the new one.
_IdentifyWindowsRequestedHandlers(*this, nullptr);
successfullyNotified = true;
}
catch (...)
{
LOG_CAUGHT_EXCEPTION();
}
TraceLoggingWrite(g_hRemotingProvider,
"Peasant_RequestIdentifyWindows",
TraceLoggingUInt64(GetID(), "peasantID", "Our ID"),
TraceLoggingBoolean(successfullyNotified, "successfullyNotified", "true if we successfully notified the monarch"),
TraceLoggingLevel(WINEVENT_LEVEL_VERBOSE));
}

}
4 changes: 4 additions & 0 deletions src/cascadia/Remoting/Peasant.h
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,8 @@ namespace winrt::Microsoft::Terminal::Remoting::implementation

bool ExecuteCommandline(const winrt::Microsoft::Terminal::Remoting::CommandlineArgs& args);
void ActivateWindow(const winrt::Microsoft::Terminal::Remoting::WindowActivatedArgs& args);
void RequestIdentifyWindows();
void DisplayWindowId();

winrt::Microsoft::Terminal::Remoting::WindowActivatedArgs GetLastActivatedArgs();

Expand All @@ -30,6 +32,8 @@ namespace winrt::Microsoft::Terminal::Remoting::implementation

TYPED_EVENT(WindowActivated, winrt::Windows::Foundation::IInspectable, winrt::Microsoft::Terminal::Remoting::WindowActivatedArgs);
TYPED_EVENT(ExecuteCommandlineRequested, winrt::Windows::Foundation::IInspectable, winrt::Microsoft::Terminal::Remoting::CommandlineArgs);
TYPED_EVENT(IdentifyWindowsRequested, winrt::Windows::Foundation::IInspectable, winrt::Windows::Foundation::IInspectable);
TYPED_EVENT(DisplayWindowIdRequested, winrt::Windows::Foundation::IInspectable, winrt::Windows::Foundation::IInspectable);

private:
Peasant(const uint64_t testPID);
Expand Down
4 changes: 4 additions & 0 deletions src/cascadia/Remoting/Peasant.idl
Original file line number Diff line number Diff line change
Expand Up @@ -34,9 +34,13 @@ namespace Microsoft.Terminal.Remoting
void ActivateWindow(WindowActivatedArgs args);
WindowActivatedArgs GetLastActivatedArgs();
String WindowName { get; };
void RequestIdentifyWindows(); // Tells us to raise a IdentifyWindowsRequested
void DisplayWindowId(); // Tells us to display its own ID (which causes a DisplayWindowIdRequested to be raised)

event Windows.Foundation.TypedEventHandler<Object, WindowActivatedArgs> WindowActivated;
event Windows.Foundation.TypedEventHandler<Object, CommandlineArgs> ExecuteCommandlineRequested;
event Windows.Foundation.TypedEventHandler<Object, Object> IdentifyWindowsRequested;
event Windows.Foundation.TypedEventHandler<Object, Object> DisplayWindowIdRequested;
};

[default_interface] runtimeclass Peasant : IPeasant
Expand Down
37 changes: 35 additions & 2 deletions src/cascadia/TerminalApp/AppActionHandlers.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -552,8 +552,8 @@ namespace winrt::TerminalApp::implementation
}
}

void TerminalPage::_HandleOpenTabSearch(const IInspectable& /*sender*/,
const ActionEventArgs& args)
void TerminalPage::_HandleTabSearch(const IInspectable& /*sender*/,
const ActionEventArgs& args)
{
CommandPalette().SetTabs(_tabs, _mruTabs);
CommandPalette().EnableTabSearchMode();
Expand Down Expand Up @@ -675,4 +675,37 @@ namespace winrt::TerminalApp::implementation
actionArgs.Handled(true);
}

// Method Description:
// - Raise a IdentifyWindowsRequested event. This will bubble up to the
// AppLogic, to the AppHost, to the Peasant, to the Monarch, then get
// distributed down to _all_ the Peasants, as to display info about the
// window in _every_ Peasant window.
// - This action is also buggy right now, because TeachingTips behave
// weird in XAML Islands. See microsoft-ui-xaml#4382
// Arguments:
// - <unused>
// Return Value:
// - <none>
void TerminalPage::_HandleIdentifyWindows(const IInspectable& /*sender*/,
const ActionEventArgs& args)
{
_IdentifyWindowsRequestedHandlers(*this, nullptr);
args.Handled(true);
}

// Method Description:
// - Display the "Toast" with the name and ID of this window.
// - Unlike _HandleIdentifyWindow**s**, this event just displays the window
// ID and name in the current window. It does not involve any bubbling
// up/down the page/logic/host/manager/peasant/monarch.
// Arguments:
// - <unused>
// Return Value:
// - <none>
void TerminalPage::_HandleIdentifyWindow(const IInspectable& /*sender*/,
const ActionEventArgs& args)
{
IdentifyWindow();
args.Handled(true);
}
zadjii-msft marked this conversation as resolved.
Show resolved Hide resolved
}
30 changes: 30 additions & 0 deletions src/cascadia/TerminalApp/AppLogic.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1380,4 +1380,34 @@ namespace winrt::TerminalApp::implementation
return _root ? _root->AlwaysOnTop() : false;
}

void AppLogic::IdentifyWindow()
{
if (_root)
{
_root->IdentifyWindow();
}
}

winrt::hstring AppLogic::WindowName()
{
return _root ? _root->WindowName() : L"";
zadjii-msft marked this conversation as resolved.
Show resolved Hide resolved
}
void AppLogic::WindowName(const winrt::hstring& name)
{
if (_root)
{
_root->WindowName(name);
}
}
uint64_t AppLogic::WindowId()
{
return _root ? _root->WindowId() : 0;
}
void AppLogic::WindowId(const uint64_t& id)
{
if (_root)
{
_root->WindowId(id);
}
}
}
7 changes: 7 additions & 0 deletions src/cascadia/TerminalApp/AppLogic.h
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,12 @@ namespace winrt::TerminalApp::implementation
bool Fullscreen() const;
bool AlwaysOnTop() const;

void IdentifyWindow();
winrt::hstring WindowName();
void WindowName(const winrt::hstring& name);
uint64_t WindowId();
void WindowId(const uint64_t& id);

Windows::Foundation::Size GetLaunchDimensions(uint32_t dpi);
bool CenterOnLaunch();
TerminalApp::InitialPosition GetInitialPosition(int64_t defaultInitialX, int64_t defaultInitialY);
Expand Down Expand Up @@ -149,6 +155,7 @@ namespace winrt::TerminalApp::implementation
FORWARDED_TYPED_EVENT(AlwaysOnTopChanged, winrt::Windows::Foundation::IInspectable, winrt::Windows::Foundation::IInspectable, _root, AlwaysOnTopChanged);
FORWARDED_TYPED_EVENT(RaiseVisualBell, winrt::Windows::Foundation::IInspectable, winrt::Windows::Foundation::IInspectable, _root, RaiseVisualBell);
FORWARDED_TYPED_EVENT(SetTaskbarProgress, winrt::Windows::Foundation::IInspectable, winrt::Windows::Foundation::IInspectable, _root, SetTaskbarProgress);
FORWARDED_TYPED_EVENT(IdentifyWindowsRequested, Windows::Foundation::IInspectable, Windows::Foundation::IInspectable, _root, IdentifyWindowsRequested);

#ifdef UNIT_TESTING
friend class TerminalAppLocalTests::CommandlineTest;
Expand Down
5 changes: 5 additions & 0 deletions src/cascadia/TerminalApp/AppLogic.idl
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,10 @@ namespace TerminalApp
Boolean Fullscreen { get; };
Boolean AlwaysOnTop { get; };

void IdentifyWindow();
String WindowName;
UInt64 WindowId;

Windows.Foundation.Size GetLaunchDimensions(UInt32 dpi);
Boolean CenterOnLaunch { get; };

Expand Down Expand Up @@ -78,5 +82,6 @@ namespace TerminalApp
event Windows.Foundation.TypedEventHandler<Object, Object> AlwaysOnTopChanged;
event Windows.Foundation.TypedEventHandler<Object, Object> RaiseVisualBell;
event Windows.Foundation.TypedEventHandler<Object, Object> SetTaskbarProgress;
event Windows.Foundation.TypedEventHandler<Object, Object> IdentifyWindowsRequested;
}
}
8 changes: 8 additions & 0 deletions src/cascadia/TerminalApp/Resources/en-US/Resources.resw
Original file line number Diff line number Diff line change
Expand Up @@ -595,6 +595,14 @@
<data name="CommandPalette_MoreOptions.[using:Windows.UI.Xaml.Automation]AutomationProperties.HelpText" xml:space="preserve">
<value>More options</value>
</data>
<data name="WindowIdLabel" xml:space="preserve">
<value>Window</value>
<comment>This is displayed as a label for a number, like "Window: 10"</comment>
</data>
<data name="UnnamedWindowName" xml:space="preserve">
<value>unnamed window</value>
<comment>text used to identify when a window hasn't been assigned a name by the user</comment>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
<comment>text used to identify when a window hasn't been assigned a name by the user</comment>
<comment>Text used to identify when a window hasn't been assigned a name by the user</comment>

</data>
<data name="WindowMaximizeButtonToolTip" xml:space="preserve">
<value>Maximize</value>
</data>
Expand Down
Loading