Skip to content

Commit

Permalink
cleanup. 18 TODOs remain
Browse files Browse the repository at this point in the history
  • Loading branch information
@zadjii-msft
zadjii-msft committed Feb 8, 2023
1 parent 3fb8e8c commit 4d5f6d2
Show file tree
Hide file tree
Showing 4 changed files with 16 additions and 130 deletions.
12 changes: 0 additions & 12 deletions src/cascadia/TerminalApp/TerminalPage.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -293,18 +293,6 @@ namespace winrt::TerminalApp::implementation
ShowSetAsDefaultInfoBar();
}

// // Method Description;
// // - Checks if the current terminal window should load or save its layout information.
// // Arguments:
// // - settings: The settings to use as this may be called before the page is
// // fully initialized.
// // Return Value:
// // - true if the ApplicationState should be used.
// bool TerminalPage::ShouldUsePersistedLayout(CascadiaSettings& settings) const
// {
// return settings.GlobalSettings().FirstWindowPreference() == FirstWindowPreference::PersistedWindowLayout;
// }

// Method Description:
// - This is a bit of trickiness: If we're running unelevated, and the user
// passed in only --elevate actions, the we don't _actually_ want to
Expand Down
44 changes: 9 additions & 35 deletions src/cascadia/TerminalApp/TerminalWindow.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -141,16 +141,13 @@ namespace winrt::TerminalApp::implementation
_root->WindowProperties(*this);
_dialog = ContentDialog{};

// Pass commandline args into the TerminalPage.
// * we first got these in SetStartupCommandline, but at that time, we
// don't have a page yet.
// Pass commandline args into the TerminalPage. If we were supposed to
// load from a persisted layout, do that instead.
auto foundLayout = false;

if (const auto& layout = LoadPersistedLayout())
{
if (layout.TabLayout().Size() > 0)
{
// _startupActions = layout.TabLayout();
std::vector<Settings::Model::ActionAndArgs> actions;
for (const auto& a : layout.TabLayout())
{
Expand Down Expand Up @@ -269,7 +266,9 @@ namespace winrt::TerminalApp::implementation

_RefreshThemeRoutine();

auto args = winrt::make_self<SystemMenuChangeArgs>(RS_(L"SettingsMenuItem"), SystemMenuChangeAction::Add, SystemMenuItemHandler(this, &TerminalWindow::_OpenSettingsUI));
auto args = winrt::make_self<SystemMenuChangeArgs>(RS_(L"SettingsMenuItem"),
SystemMenuChangeAction::Add,
SystemMenuItemHandler(this, &TerminalWindow::_OpenSettingsUI));
_SystemMenuChangeRequestedHandlers(*this, *args);

TraceLoggingWrite(
Expand Down Expand Up @@ -334,7 +333,7 @@ namespace winrt::TerminalApp::implementation
// - dialog: the dialog object that is going to show up
// Return value:
// - an IAsyncOperation with the dialog result
winrt::Windows::Foundation::IAsyncOperation<ContentDialogResult> TerminalWindow::ShowDialog(winrt::Windows::UI::Xaml::Controls::ContentDialog dialog)
winrt::Windows::Foundation::IAsyncOperation<ContentDialogResult> TerminalWindow::ShowDialog(winrt::WUX::Controls::ContentDialog dialog)
{
// DON'T release this lock in a wil::scope_exit. The scope_exit will get
// called when we await, which is not what we want.
Expand Down Expand Up @@ -949,12 +948,7 @@ namespace winrt::TerminalApp::implementation
}

////////////////////////////////////////////////////////////////////////////

// bool TerminalWindow::HasSettingsStartupActions() const noexcept
// {
// return _hasSettingsStartupActions;
// }
void TerminalWindow::SetSettingsStartupArgs(const std::vector<winrt::Microsoft::Terminal::Settings::Model::ActionAndArgs>& actions)
void TerminalWindow::SetSettingsStartupArgs(const std::vector<ActionAndArgs>& actions)
{
for (const auto& action : actions)
{
Expand Down Expand Up @@ -999,18 +993,11 @@ namespace winrt::TerminalApp::implementation
// Instead, we'll handle that in Initialize, when we first instantiate the page.
}

// If we have a -s param passed to us to load a saved layout, cache that now.
if (const auto idx = _appArgs.GetPersistedLayoutIdx())
{
SetPersistedLayoutIdx(idx.value());
}
// else
// {
// if (_settings.GlobalSettings().ShouldUsePersistedLayout() &&
// args.size() <= 1)
// {
// SetPersistedLayoutIdx(0);
// }
// }
return result;
}

Expand Down Expand Up @@ -1122,18 +1109,6 @@ namespace winrt::TerminalApp::implementation
_root->SetNumberOfOpenWindows(num);
}
}

// // Method Description;
// // - Checks if the current terminal window should load or save its layout information.
// // Arguments:
// // - settings: The settings to use as this may be called before the page is
// // fully initialized.
// // Return Value:
// // - true if the ApplicationState should be used.
// bool TerminalWindow::ShouldUsePersistedLayout() const
// {
// return _settings.GlobalSettings().ShouldUsePersistedLayout();
// }
////////////////////////////////////////////////////////////////////////////

void TerminalWindow::IdentifyWindow()
Expand Down Expand Up @@ -1167,12 +1142,11 @@ namespace winrt::TerminalApp::implementation
{
return _settings.GlobalSettings().AutoHideWindow();
}
// TODO! Arg should be a SettingsLoadEventArgs{ result, warnings, error, settings}

void TerminalWindow::UpdateSettingsHandler(const winrt::IInspectable& /*sender*/,
const winrt::TerminalApp::SettingsLoadEventArgs& args)
{
UpdateSettings(args);
// _root->SetSettings(_settings, true);
}

////////////////////////////////////////////////////////////////////////////
Expand Down
20 changes: 6 additions & 14 deletions src/cascadia/WindowsTerminal/AppHost.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -177,21 +177,8 @@ void AppHost::SetTaskbarProgress(const winrt::Windows::Foundation::IInspectable&
// - <none>
// Return Value:
// - <none>
void AppHost::_HandleCommandlineArgs(/*const winrt::Microsoft::Terminal::Remoting::WindowRequestedArgs& args*/)
void AppHost::_HandleCommandlineArgs()
{
// std::vector<winrt::hstring> args;
// // _buildArgsFromCommandline(args);
// auto cwd{ wil::GetCurrentDirectoryW<std::wstring>() };

// Remoting::CommandlineArgs eventArgs{ { args }, { cwd } };
// _windowManager2.ProposeCommandline(eventArgs);

// _shouldCreateWindow = _windowManager2.ShouldCreateWindow();
// if (!_shouldCreateWindow)
// {
// return;
// }

// We did want to make a window, so let's instantiate it here.
// We don't have XAML yet, but we do have other stuff.
_windowLogic = _appLogic.CreateNewWindow();
Expand Down Expand Up @@ -249,6 +236,11 @@ void AppHost::_HandleCommandlineArgs(/*const winrt::Microsoft::Terminal::Remotin
_revokers.peasantDisplayWindowIdRequested = _peasant.DisplayWindowIdRequested(winrt::auto_revoke, { this, &AppHost::_DisplayWindowId });
_revokers.peasantQuitRequested = _peasant.QuitRequested(winrt::auto_revoke, { this, &AppHost::_QuitRequested });

// This is logic that almost seems like it belongs on the WindowEmperor.
// It probably does. However, it needs to muck with our own window so
// much, that there was no reasonable way of moving this. Moving it also
// seemed to reorder bits of init so much that everything broke. So
// we'll leave it here.
const auto numPeasants = _windowManager2.GetNumberOfPeasants();
if (numPeasants == 1)
{
Expand Down
70 changes: 1 addition & 69 deletions src/cascadia/WindowsTerminal/WindowEmperor.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -79,8 +79,8 @@ bool WindowEmperor::HandleCommandlineArgs()
_manager.RequestNewWindow([this](auto&&, const Remoting::WindowRequestedArgs& args) {
CreateNewWindowThread(args, false);
});

_becomeMonarch();
// _attemptWindowRestore(eventArgs);
}

return result.ShouldCreateWindow();
Expand Down Expand Up @@ -137,23 +137,6 @@ void WindowEmperor::CreateNewWindowThread(Remoting::WindowRequestedArgs args, co
auto func = [this, args, peasant, firstWindow]() {
auto window{ std::make_shared<WindowThread>(_app.Logic(), args, _manager, peasant) };
_windows.push_back(window);

// if (firstWindow)
// {
// const auto layouts = ApplicationState::SharedInstance().PersistedWindowLayouts();
// if (_app.Logic().ShouldUsePersistedLayout() &&
// layouts &&
// layouts.Size() > 0)
// {
// if (
// !window->Logic().HasCommandlineArguments() &&
// !_app.Logic().HasSettingsStartupActions())
// {
// window->Logic().SetPersistedLayoutIdx(0);
// }
// }
// }

return window->WindowProc();
};

Expand Down Expand Up @@ -217,57 +200,6 @@ void WindowEmperor::_numberOfWindowsChanged(const winrt::Windows::Foundation::II
}
}

void WindowEmperor::_attemptWindowRestore(const Remoting::CommandlineArgs& args)
{
// At this point, we know there's exactly one peasant.
// * WindowLogic / TerminalPage should start with htat value initialized to 1
// * emperor should listen for windowManager.NumberOfOpenWindowsChanged and then propogate that to all its WindowThreads
// * we should store WindowThread ptrs in the vector, not std::thread's you idiot
// * WindowThread should own the thread, don't you think OH but do we want thre WindowThread to live on main? or on the std::thread - probably the second

// const auto numPeasants = _windowManager2.GetNumberOfPeasants();
const auto layouts = ApplicationState::SharedInstance().PersistedWindowLayouts();

if (_app.Logic().ShouldUsePersistedLayout() &&
layouts &&
layouts.Size() > 0)
{
// We want to create a window for every saved layout.
// If we are the only window, and no commandline arguments were provided
// then we should just use the current window to load the first layout.
// Otherwise create this window normally with its commandline, and create
// a new window using the first saved layout information.
// The 2nd+ layout will always get a new window.

// _windowLogic.HasCommandlineArguments === args.size() > 1
uint32_t startIdx = args.Commandline().size() > 1 ? 0 : 1;

// Create new windows for each of the other saved layouts.
for (const auto size = layouts.Size(); startIdx < size; startIdx += 1)
{
auto newWindowArgs = fmt::format(L"{0} -w new -s {1}", args.Commandline()[0], startIdx);

STARTUPINFO si;
memset(&si, 0, sizeof(si));
si.cb = sizeof(si);
wil::unique_process_information pi;

LOG_IF_WIN32_BOOL_FALSE(CreateProcessW(nullptr,
newWindowArgs.data(),
nullptr, // lpProcessAttributes
nullptr, // lpThreadAttributes
false, // bInheritHandles
DETACHED_PROCESS | CREATE_UNICODE_ENVIRONMENT, // doCreationFlags
nullptr, // lpEnvironment
nullptr, // lpStartingDirectory
&si, // lpStartupInfo
&pi // lpProcessInformation
));
}
}
// _windowLogic.SetNumberOfOpenWindows(numPeasants);
}

winrt::Windows::Foundation::IAsyncAction WindowEmperor::_SaveWindowLayouts()
{
// Make sure we run on a background thread to not block anything.
Expand Down

1 comment on commit 4d5f6d2

@github-actions
Copy link

Choose a reason for hiding this comment

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

@check-spelling-bot Report

🔴 Please review

See the 📜action log for details.

Unrecognized words (9)

APeasant
Apphost
applogic
becuase
chould
connectecd
definted
initilized
wehn

Previously acknowledged words that are now absent CLA demoable everytime Hirots inthread reingest unmark :arrow_right:
To accept ✔️ these unrecognized words as correct and remove the previously acknowledged and now absent words, run the following commands

... in a clone of the [email protected]:microsoft/terminal.git repository
on the dev/migrie/oop/3/ainulindale branch (ℹ️ how do I use this?):

curl -s -S -L 'https://raw.githubusercontent.com/check-spelling/check-spelling/v0.0.21/apply.pl' |
perl - 'https://github.com/microsoft/terminal/actions/runs/4129340192/attempts/1'
Errors (1)

See the 📜action log for details.

❌ Errors Count
❌ forbidden-pattern 1

See ❌ Event descriptions for more information.

✏️ Contributor please read this

By default the command suggestion will generate a file named based on your commit. That's generally ok as long as you add the file to your commit. Someone can reorganize it later.

⚠️ The command is written for posix shells. If it doesn't work for you, you can manually add (one word per line) / remove items to expect.txt and the excludes.txt files.

If the listed items are:

  • ... misspelled, then please correct them instead of using the command.
  • ... names, please add them to .github/actions/spelling/allow/names.txt.
  • ... APIs, you can add them to a file in .github/actions/spelling/allow/.
  • ... just things you're using, please add them to an appropriate file in .github/actions/spelling/expect/.
  • ... tokens you only need in one place and shouldn't generally be used, you can add an item in an appropriate file in .github/actions/spelling/patterns/.

See the README.md in each directory for more information.

🔬 You can test your commits without appending to a PR by creating a new branch with that extra change and pushing it to your fork. The check-spelling action will run in response to your push -- it doesn't require an open pull request. By using such a branch, you can limit the number of typos your peers see you make. 😉

If the flagged items are 🤯 false positives

If items relate to a ...

  • binary file (or some other file you wouldn't want to check at all).

    Please add a file path to the excludes.txt file matching the containing file.

    File paths are Perl 5 Regular Expressions - you can test yours before committing to verify it will match your files.

    ^ refers to the file's path from the root of the repository, so ^README\.md$ would exclude README.md (on whichever branch you're using).

  • well-formed pattern.

    If you can write a pattern that would match it,
    try adding it to the patterns.txt file.

    Patterns are Perl 5 Regular Expressions - you can test yours before committing to verify it will match your lines.

    Note that patterns can't match multiline strings.

Please sign in to comment.