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 support for renaming windows #9662

Merged
55 commits merged into from
Apr 2, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
55 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
99c2976
Action boilerplate for both these actions
zadjii-msft Mar 2, 2021
c09b5e9
bubble these up and down too
zadjii-msft Mar 2, 2021
aa51c07
THIS SHOULD GO TO THE PARENT BRANCH
zadjii-msft Mar 2, 2021
e818f8d
Okay the window renamer is pretty slick
zadjii-msft Mar 2, 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
e8edc71
Merge branch 'dev/migrie/f/identifyWindows' into dev/migrie/f/rename-…
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
ee3b87b
Merge remote-tracking branch 'origin/dev/migrie/f/identifyWindows' in…
zadjii-msft Mar 30, 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
a9a0f06
Merge branch 'dev/migrie/f/identifyWindows' into dev/migrie/f/rename-…
zadjii-msft Mar 30, 2021
3dab6ad
runxamlformat
zadjii-msft Mar 30, 2021
2c1f468
Start writing tests, UNSURPRISINGLY THE TEST DONT WORK ANYMORE
zadjii-msft Mar 30, 2021
06b4168
This fixes part of the tests
zadjii-msft Mar 30, 2021
205eee7
Make the comments clearer
zadjii-msft Mar 30, 2021
673149c
Merge remote-tracking branch 'origin/main' into dev/migrie/f/rename-w…
zadjii-msft Mar 30, 2021
4d66517
Merge branch 'deb/migrie/b/9659-tests-march-21' into dev/migrie/f/ren…
zadjii-msft Mar 30, 2021
f76de3c
implement some tests for window renaming
zadjii-msft Mar 30, 2021
d7dd615
Add remoting tests too, because I'm a good dveloper
zadjii-msft Mar 30, 2021
1664737
I can type that word 100 times and I will never get it right
zadjii-msft Mar 30, 2021
53adc15
add to the defaults and the schema, because I'm a good person
zadjii-msft Mar 30, 2021
e0ea241
Merge remote-tracking branch 'origin/main' into dev/migrie/f/rename-w…
zadjii-msft Mar 30, 2021
e207f4e
Merge remote-tracking branch 'origin/main' into dev/migrie/f/rename-w…
zadjii-msft Apr 1, 2021
edff3bb
nits for carlos
zadjii-msft Apr 1, 2021
631d3ed
Nits for dustin
zadjii-msft Apr 2, 2021
312db84
that's what I get for having two different git clones
zadjii-msft Apr 2, 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
36 changes: 36 additions & 0 deletions doc/cascadia/profiles.schema.json
Original file line number Diff line number Diff line change
Expand Up @@ -85,12 +85,14 @@
"openNewTabDropdown",
"openSettings",
"openTabColorPicker",
"openWindowRenamer",
"paste",
"prevTab",
"renameTab",
"openTabRenamer",
"resetFontSize",
"resizePane",
"renameWindow",
"scrollDown",
"scrollDownPage",
"scrollUp",
Expand Down Expand Up @@ -651,6 +653,38 @@
}
]
},
"RenameTabAction": {
"description": "Arguments corresponding to a renameTab Action",
"allOf": [
{ "$ref": "#/definitions/ShortcutAction" },
{
"properties": {
"action": { "type": "string", "pattern": "renameTab" },
"title": {
"type": "string",
"default": "",
"description": "A title to assign to the tab. If omitted or null, this action will restore the tab's title to the original value."
}
}
}
]
},
zadjii-msft marked this conversation as resolved.
Show resolved Hide resolved
"RenameWindowAction": {
"description": "Arguments corresponding to a renameWindow Action",
"allOf": [
{ "$ref": "#/definitions/ShortcutAction" },
{
"properties": {
"action": { "type": "string", "pattern": "renameWindow" },
"name": {
"type": "string",
"default": "",
"description": "A name to assign to the window."
}
}
}
]
},
"Keybinding": {
"additionalProperties": false,
"properties": {
Expand Down Expand Up @@ -679,6 +713,8 @@
{ "$ref": "#/definitions/NewWindowAction" },
{ "$ref": "#/definitions/NextTabAction" },
{ "$ref": "#/definitions/PrevTabAction" },
{ "$ref": "#/definitions/RenameTabAction" },
{ "$ref": "#/definitions/RenameWindowAction" },
{ "type": "null" }
]
},
Expand Down
56 changes: 56 additions & 0 deletions src/cascadia/LocalTests_TerminalApp/TabTests.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -85,6 +85,9 @@ namespace TerminalAppLocalTests
TEST_METHOD(NextMRUTab);
TEST_METHOD(VerifyCommandPaletteTabSwitcherOrder);

TEST_METHOD(TestWindowRenameSuccessful);
TEST_METHOD(TestWindowRenameFailure);

TEST_CLASS_SETUP(ClassSetup)
{
return true;
Expand Down Expand Up @@ -948,4 +951,57 @@ namespace TerminalAppLocalTests
// will also dismiss itself immediately when that's called. So we can't
// really inspect the contents of the list in this test, unfortunately.
}

void TabTests::TestWindowRenameSuccessful()
{
auto page = _commonSetup();
zadjii-msft marked this conversation as resolved.
Show resolved Hide resolved
page->RenameWindowRequested([&page](auto&&, const winrt::TerminalApp::RenameWindowRequestedArgs args) {
// In the real terminal, this would bounce up to the monarch and
// come back down. Instead, immediately call back and set the name.
page->WindowName(args.ProposedName());
});

bool windowNameChanged = false;
page->PropertyChanged([&page, &windowNameChanged](auto&&, const winrt::WUX::Data::PropertyChangedEventArgs& args) mutable {
if (args.PropertyName() == L"WindowNameForDisplay")
{
windowNameChanged = true;
}
});

TestOnUIThread([&page]() {
page->_RequestWindowRename(winrt::hstring{ L"Foo" });
});
TestOnUIThread([&]() {
VERIFY_ARE_EQUAL(L"Foo", page->_WindowName);
VERIFY_IS_TRUE(windowNameChanged,
L"The window name should have changed, and we should have raised a notification that WindowNameForDisplay changed");
});
}
void TabTests::TestWindowRenameFailure()
{
auto page = _commonSetup();
page->RenameWindowRequested([&page](auto&&, auto&&) {
// In the real terminal, this would bounce up to the monarch and
// come back down. Instead, immediately call back to tell the terminal it failed.
page->RenameFailed();
});

bool windowNameChanged = false;

page->PropertyChanged([&page, &windowNameChanged](auto&&, const winrt::WUX::Data::PropertyChangedEventArgs& args) mutable {
if (args.PropertyName() == L"WindowNameForDisplay")
{
windowNameChanged = true;
}
});

TestOnUIThread([&page]() {
page->_RequestWindowRename(winrt::hstring{ L"Foo" });
});
TestOnUIThread([&]() {
VERIFY_IS_FALSE(windowNameChanged,
L"The window name should not have changed, we should have rejected the change.");
});
}
}
1 change: 1 addition & 0 deletions src/cascadia/LocalTests_TerminalApp/pch.h
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,7 @@ Author(s):
#include <winrt/Windows.ui.input.h>
#include <winrt/Windows.UI.Xaml.Controls.h>
#include <winrt/Windows.UI.Xaml.Controls.Primitives.h>
#include <winrt/Windows.UI.Xaml.Data.h>
#include <winrt/Windows.ui.xaml.media.h>
#include <winrt/Windows.ui.xaml.input.h>
#include <winrt/Windows.UI.Xaml.Markup.h>
Expand Down
6 changes: 6 additions & 0 deletions src/cascadia/Remoting/Microsoft.Terminal.RemotingLib.vcxproj
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,9 @@
<ClInclude Include="ProposeCommandlineResult.h">
<DependentUpon>Monarch.idl</DependentUpon>
</ClInclude>
<ClInclude Include="RenameRequestArgs.h">
<DependentUpon>Peasant.idl</DependentUpon>
</ClInclude>
<ClInclude Include="WindowActivatedArgs.h">
<DependentUpon>Peasant.idl</DependentUpon>
</ClInclude>
Expand All @@ -51,6 +54,9 @@
<ClCompile Include="ProposeCommandlineResult.cpp">
<DependentUpon>Monarch.idl</DependentUpon>
</ClCompile>
<ClCompile Include="RenameRequestArgs.cpp">
<DependentUpon>Peasant.idl</DependentUpon>
</ClCompile>
<ClCompile Include="WindowActivatedArgs.cpp">
<DependentUpon>Peasant.idl</DependentUpon>
</ClCompile>
Expand Down
49 changes: 49 additions & 0 deletions src/cascadia/Remoting/Monarch.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,7 @@ namespace winrt::Microsoft::Terminal::Remoting::implementation
// Add an event listener to the peasant's WindowActivated event.
peasant.WindowActivated({ this, &Monarch::_peasantWindowActivated });
peasant.IdentifyWindowsRequested({ this, &Monarch::_identifyWindows });
peasant.RenameRequested({ this, &Monarch::_renameRequested });

_peasants[newPeasantsId] = peasant;

Expand Down Expand Up @@ -631,4 +632,52 @@ namespace winrt::Microsoft::Terminal::Remoting::implementation
};
_forAllPeasantsIgnoringTheDead(callback, onError);
}

// Method Description:
// - This is an event handler for the RenameRequested event. A
// Peasant may raise that event when they want to be renamed to something else.
// - We will check if there are any other windows with this name. If there
// are, then we'll reject the rename by setting args.Succeeded=false.
// - If there aren't any other windows with this name, then we'll set
// args.Succeeded=true, allowing the window to keep this name.
// Arguments:
// - args: Contains the requested window name and a boolean (Succeeded)
// indicating if the request was successful.
// Return Value:
// - <none>
void Monarch::_renameRequested(const winrt::Windows::Foundation::IInspectable& /*sender*/,
const winrt::Microsoft::Terminal::Remoting::RenameRequestArgs& args)
{
bool successfullyRenamed = false;

try
{
args.Succeeded(false);
const auto name{ args.NewName() };
// Try to find a peasant that currently has this name
const auto id = _lookupPeasantIdForName(name);
if (_getPeasant(id) == nullptr)
zadjii-msft marked this conversation as resolved.
Show resolved Hide resolved
{
// If there is one, then oh no! The requestor is not allowed to
// be renamed.
args.Succeeded(true);
successfullyRenamed = true;
}

TraceLoggingWrite(g_hRemotingProvider,
"Monarch_renameRequested",
TraceLoggingWideString(name.c_str(), "name", "The newly proposed name"),
TraceLoggingInt64(successfullyRenamed, "successfullyRenamed", "true if the peasant is allowed to rename themselves to that name."),
TraceLoggingLevel(WINEVENT_LEVEL_VERBOSE));
}
catch (...)
{
LOG_CAUGHT_EXCEPTION();
// If this fails, we don't _really_ care. The peasant died, but
// they're the only one who cares about the result.
TraceLoggingWrite(g_hRemotingProvider,
"Monarch_renameRequested_Failed",
TraceLoggingLevel(WINEVENT_LEVEL_VERBOSE));
}
}
}
8 changes: 6 additions & 2 deletions src/cascadia/Remoting/Monarch.h
Original file line number Diff line number Diff line change
Expand Up @@ -74,11 +74,15 @@ 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 _forAllPeasantsIgnoringTheDead(std::function<void(const winrt::Microsoft::Terminal::Remoting::IPeasant&, const uint64_t)> callback,
std::function<void(const uint64_t)> errorCallback);

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);
void _renameRequested(const winrt::Windows::Foundation::IInspectable& sender,
const winrt::Microsoft::Terminal::Remoting::RenameRequestArgs& args);

friend class RemotingUnitTests::RemotingTests;
};
}
Expand Down
30 changes: 30 additions & 0 deletions src/cascadia/Remoting/Peasant.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -164,4 +164,34 @@ namespace winrt::Microsoft::Terminal::Remoting::implementation
TraceLoggingLevel(WINEVENT_LEVEL_VERBOSE));
}

void Peasant::RequestRename(const winrt::Microsoft::Terminal::Remoting::RenameRequestArgs& args)
{
bool successfullyNotified = false;
const auto oldName{ _WindowName };
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.
_RenameRequestedHandlers(*this, args);
if (args.Succeeded())
{
_WindowName = args.NewName();
}
successfullyNotified = true;
}
catch (...)
{
LOG_CAUGHT_EXCEPTION();
}
TraceLoggingWrite(g_hRemotingProvider,
"Peasant_RequestRename",
TraceLoggingUInt64(GetID(), "peasantID", "Our ID"),
TraceLoggingWideString(oldName.c_str(), "oldName", "Our old name"),
TraceLoggingWideString(args.NewName().c_str(), "newName", "The proposed name"),
TraceLoggingBoolean(args.Succeeded(), "succeeded", "true if the monarch ok'd this new name for us."),
TraceLoggingBoolean(successfullyNotified, "successfullyNotified", "true if we successfully notified the monarch"),
TraceLoggingLevel(WINEVENT_LEVEL_VERBOSE));
}
}
3 changes: 3 additions & 0 deletions src/cascadia/Remoting/Peasant.h
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@

#include "Peasant.g.h"
#include "../cascadia/inc/cppwinrt_utils.h"
#include "RenameRequestArgs.h"

namespace RemotingUnitTests
{
Expand All @@ -24,6 +25,7 @@ namespace winrt::Microsoft::Terminal::Remoting::implementation
void ActivateWindow(const winrt::Microsoft::Terminal::Remoting::WindowActivatedArgs& args);
void RequestIdentifyWindows();
void DisplayWindowId();
void RequestRename(const winrt::Microsoft::Terminal::Remoting::RenameRequestArgs& args);

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

Expand All @@ -34,6 +36,7 @@ namespace winrt::Microsoft::Terminal::Remoting::implementation
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);
TYPED_EVENT(RenameRequested, winrt::Windows::Foundation::IInspectable, winrt::Microsoft::Terminal::Remoting::RenameRequestArgs);

private:
Peasant(const uint64_t testPID);
Expand Down
10 changes: 10 additions & 0 deletions src/cascadia/Remoting/Peasant.idl
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,13 @@ namespace Microsoft.Terminal.Remoting
String CurrentDirectory();
};

runtimeclass RenameRequestArgs
{
RenameRequestArgs(String newName);
String NewName { get; };
Boolean Succeeded;
};

runtimeclass WindowActivatedArgs
{
WindowActivatedArgs(UInt64 peasantID, Guid desktopID, Windows.Foundation.DateTime activatedTime);
Expand All @@ -37,10 +44,13 @@ namespace Microsoft.Terminal.Remoting
void RequestIdentifyWindows(); // Tells us to raise a IdentifyWindowsRequested
void DisplayWindowId(); // Tells us to display its own ID (which causes a DisplayWindowIdRequested to be raised)

void RequestRename(RenameRequestArgs args); // Tells us to raise a RenameRequested

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;
event Windows.Foundation.TypedEventHandler<Object, RenameRequestArgs> RenameRequested;
};

[default_interface] runtimeclass Peasant : IPeasant
Expand Down
5 changes: 5 additions & 0 deletions src/cascadia/Remoting/RenameRequestArgs.cpp
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
// Copyright (c) Microsoft Corporation.
// Licensed under the MIT license.
#include "pch.h"
#include "RenameRequestArgs.h"
#include "RenameRequestArgs.g.cpp"
30 changes: 30 additions & 0 deletions src/cascadia/Remoting/RenameRequestArgs.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
/*++
Copyright (c) Microsoft Corporation
Licensed under the MIT license.

Class Name:
- RenameRequestArgs.h

--*/
#pragma once

#include "RenameRequestArgs.g.h"
#include "../cascadia/inc/cppwinrt_utils.h"

namespace winrt::Microsoft::Terminal::Remoting::implementation
{
struct RenameRequestArgs : public RenameRequestArgsT<RenameRequestArgs>
{
WINRT_PROPERTY(winrt::hstring, NewName);
WINRT_PROPERTY(bool, Succeeded, false);

public:
RenameRequestArgs(winrt::hstring newName) :
_NewName{ newName } {};
};
}

namespace winrt::Microsoft::Terminal::Remoting::factory_implementation
{
BASIC_FACTORY(RenameRequestArgs);
}
Loading