Introduce a basic ApplicationState class #10513

lhecker · 2021-06-25T15:43:06Z

This commit introduces a basic ApplicationState class, without being used for anything yet to aid reviewers. At a later point actual usages of this new class may be added separately.

References

This commit is an initial step towards implementing #8324.

PR Checklist

I work here
Tests added/passed

Validation Steps Performed

Creating a state.json with {"generatedProfiles":["{53e75ed9-2b63-4118-856d-0510c4f6b97e}"]} updates the ApplicationState, as observed through a debugger ✔️
Deleting the "generatedProfiles" field sets the corresponding field back to nullopt ✔️

lhecker · 2021-06-25T16:08:21Z

src/cascadia/TerminalApp/AppLogic.cpp

-        _dialogLock{},
-        _loadedInitialSettings{ false },
-        _settingsLoadedResult{ S_OK }
+        _reloadState{ std::chrono::milliseconds(100), []() { ApplicationState::SharedInstance().Reload(); } }


Similar to the existing settings-reload throttler, this does the same but for state.json.
Since til::throttled_func cannot be copied or moved, this type must be initialized in the member list initializer (instead of more ergonomically in the constructor function body).

lhecker · 2021-06-25T16:09:23Z

src/cascadia/TerminalApp/AppLogic.cpp

+        _reloadSettings = std::make_shared<ThrottledFuncTrailing<>>(_root->Dispatcher(), std::chrono::milliseconds(100), [weakSelf = get_weak()]() {
+            if (auto self{ weakSelf.get() })
+            {
+                self->_ReloadSettings();
+            }
+        });


This replaces the previous, manual settings.json load delay & throttling we had before.

lhecker · 2021-06-25T16:16:07Z

src/cascadia/TerminalApp/AppLogic.cpp

+            // settings.json. This second case will oftentimes happen with text
+            // editors, who will write a temp file, then rename it to be the
+            // actual file you wrote. So listen for that too.
+            wil::FolderChangeEvents::FileName | wil::FolderChangeEvents::LastWriteTime,


We previously listened for wil::FolderChangeEvents::All and then filtered the events, but this isn't necessary...

FileName maps to FILE_NOTIFY_CHANGE_FILE_NAME and includes renaming, creating, or deleting a file name.

LastWriteTime maps to FILE_NOTIFY_CHANGE_LAST_WRITE and includes any writes that are flushed to disk.

lhecker · 2021-06-25T16:18:35Z

src/cascadia/TerminalSettingsModel/CascadiaSettingsSerialization.cpp

-            //      create the defaults.
-            LOG_LAST_ERROR();
-            return std::nullopt;
+            std::filesystem::rename(pathToLegacySettingsFile, pathToSettingsFile);


std::filesystem::rename attempts to be atomic and we don't have to use Win32 for that.

src/cascadia/TerminalSettingsModel/CascadiaSettingsSerialization.cpp

lhecker · 2021-06-25T16:20:02Z

src/cascadia/TerminalSettingsModel/FileUtils.cpp

+
+namespace Microsoft::Terminal::Settings::Model
+{
+    std::filesystem::path GetBaseSettingsPath()


This is an almost 1:1 copy of CascadiaSettings::SettingsPath().

lhecker · 2021-06-25T16:21:34Z

src/cascadia/TerminalSettingsModel/FileUtils.cpp

+        return baseSettingsPath;
+    }
+
+    std::string ReadUTF8File(const std::filesystem::path& path)


This function unifies our many, many manual ways to read files from the disk.
This ensures we focus unsafe API usage in a single function and can easily introduce features like:

Preventing us from returning cut-off files / files that were changed while reading

Retrying in case something goes wrong temporarily.

lhecker · 2021-06-25T16:23:43Z

src/cascadia/TerminalSettingsModel/FileUtils.cpp

+        {
+            if (exception.GetErrorCode() == HRESULT_FROM_WIN32(ERROR_FILE_NOT_FOUND))
+            {
+                return {};


Yeah I know exceptions for control flow are bad, but I didn't have a good idea how to write this without being too roundabout.

lhecker · 2021-06-25T16:24:09Z

src/cascadia/TerminalSettingsModel/FileUtils.cpp

+        }
+    }
+
+    void WriteUTF8FileAtomic(const std::filesystem::path& path, const std::string_view content)


src/cascadia/TerminalSettingsModel/FileUtils.cpp

miniksa · 2021-06-28T23:22:13Z

src/cascadia/TerminalSettingsModel/FileUtils.cpp

+        auto tmpPath = path;
+        tmpPath += L".tmp";
+
+        WriteUTF8File(tmpPath, content);


What if the path with .tmp suffix already exists? What if the rename fails?

I'm assuming the user will never create a .tmp file themselves...
Overwriting it is absolutely fine since the actually important file (state.json) will only be overwritten by the rename operation. And the rename is basically an atomic operation so it shouldn't ever fail midway.

miniksa · 2021-06-28T23:23:10Z

src/cascadia/TerminalSettingsModel/FileUtils.cpp

+
+            if (til::starts_with(buffer, Utf8Bom))
+            {
+                buffer.erase(0, Utf8Bom.size());


Man I really hope this doesn't reallocate.

No, but it does a slow 🍑 memmove! I just don't think a lot of people will have a UTF-8 BOM and I don't want to plan for it any more than necessary, because BOMs in UTF8 are an aBOMination. *ba dum tss*

miniksa · 2021-06-28T23:24:00Z

src/cascadia/TerminalSettingsModel/ApplicationState.h

+// It provides X with the following arguments:
+//   (type, function name, JSON key, ...variadic construction arguments)
+#define MTSM_APPLICATION_STATE_FIELDS(X) \
+    X(std::unordered_set<winrt::guid>, GeneratedProfiles, "generatedProfiles")


In theory, more things go here later, right? And you need more Traits to convert them?

Yeah all fields will just go into this macro (and into the .idl).
But hopefully we won't need more converters. This new one is just a hack, because I wanted to have an unordered set for later, but I don't think we'll need it anywhere else.

DHowett · 2021-06-29T15:32:35Z

src/cascadia/TerminalSettingsModel/ApplicationState.cpp

+
+        std::string TypeDescription() const
+        {
+            return fmt::format("std::unordered_set<{}>", ConversionTrait<GUID>{}.TypeDescription());


This message is intended for display to the user, but at the same time, the state deserializer is meant to be silent and discard-on-failure 😄

What would you suggest? Should I remove the implementation of this function and return an empty string?

my recommendation would be something less C++y and more javascripty. "array (of {})" or something similar. I don't wholly mind if you leave it like this

or even like, "list of guids"

src/cascadia/TerminalSettingsModel/CascadiaSettingsSerialization.cpp

lhecker · 2021-06-29T15:44:32Z

@DHowett It's interesting how I cannot reply to your comment.
Either way: I'll revert the change to GetModuleFileNameW. The prior version is technically more correct, right?

DHowett · 2021-06-29T16:02:20Z

The prior version is technically more correct

technically correct, which "is the best kind of correct"

carlos-zamora · 2021-06-28T17:42:00Z

src/cascadia/TerminalSettingsModel/FileUtils.h

+// Copyright (c) Microsoft Corporation.
+// Licensed under the MIT license.


nit: add a short abstract and slap your name on it haha

I'm a bit confused by our header styles: til uses this brief format, while some files uses the verbose one. Can I use the brief one in new code? Or should it always be the verbose one outside of til?
But I'm also not sure if anyone reads the verbose variant to be honest...

So, generally, we should put the verbose one in the header, but the brief one in the cpp.

For files/classes that are pretty self-explanatory, the brief one is fine imo. Like, this one is probably fine because "FileUtils" is pretty clear that this is just a bunch of useful functions when working with files.

For the other one (ApplicationState), it's more useful because...

it's more complicated of a concept. What is this class used for?

if we recognize the name, it's easier to reach out to them and find them (easier than a git blame, but not by much)

src/cascadia/TerminalSettingsModel/FileUtils.cpp

src/cascadia/TerminalSettingsModel/ApplicationState.h

src/cascadia/TerminalSettingsModel/ApplicationState.cpp

src/cascadia/TerminalSettingsModel/CascadiaSettingsSerialization.cpp

ghost · 2021-06-30T00:23:06Z

Hello @lhecker!

Because this pull request has the AutoMerge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (`@msftbot`) and give me an instruction to get started! Learn more here.

lhecker added 2 commits June 25, 2021 17:39

Introduce a basic ApplicationState class

7d45da3

Fix spell check

48d12b5

This comment has been minimized.

Sign in to view

Fix formatting check

2c12673

lhecker commented Jun 25, 2021

View reviewed changes

lhecker marked this pull request as ready for review June 28, 2021 15:54

miniksa reviewed Jun 28, 2021

View reviewed changes

src/cascadia/TerminalSettingsModel/FileUtils.cpp Outdated Show resolved Hide resolved

miniksa reviewed Jun 28, 2021

View reviewed changes

DHowett approved these changes Jun 29, 2021

View reviewed changes

carlos-zamora reviewed Jun 29, 2021

View reviewed changes

carlos-zamora assigned lhecker Jun 29, 2021

Address reviewer comments & improve docs

08ef30b

carlos-zamora approved these changes Jun 29, 2021

View reviewed changes

lhecker merged commit ab5a8d7 into main Jun 30, 2021

lhecker deleted the dev/lhecker/app-state branch June 30, 2021 00:25

zadjii-msft mentioned this pull request Jul 19, 2021

Default profiles appear even if removed from the config.json #8270

Closed

zadjii-msft mentioned this pull request Jan 8, 2024

Restore previously closed session's state (Buffer contents) #961

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce a basic ApplicationState class #10513

Introduce a basic ApplicationState class #10513

lhecker commented Jun 25, 2021 •

edited

Loading

This comment has been minimized.

lhecker Jun 25, 2021

lhecker Jun 25, 2021

lhecker Jun 25, 2021

lhecker Jun 25, 2021

lhecker Jun 25, 2021

lhecker Jun 25, 2021

lhecker Jun 25, 2021

lhecker Jun 25, 2021

miniksa Jun 28, 2021

lhecker Jun 28, 2021

miniksa Jun 28, 2021

lhecker Jun 28, 2021 •

edited

Loading

miniksa Jun 28, 2021

lhecker Jun 28, 2021

DHowett Jun 29, 2021

lhecker Jun 29, 2021

DHowett Jun 29, 2021

DHowett Jun 29, 2021

lhecker commented Jun 29, 2021

DHowett commented Jun 29, 2021

carlos-zamora Jun 28, 2021

lhecker Jun 29, 2021

carlos-zamora Jun 29, 2021

ghost commented Jun 30, 2021

		// Copyright (c) Microsoft Corporation.
		// Licensed under the MIT license.

Introduce a basic ApplicationState class #10513

Introduce a basic ApplicationState class #10513

Conversation

lhecker commented Jun 25, 2021 • edited Loading

References

PR Checklist

Validation Steps Performed

This comment has been minimized.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lhecker Jun 28, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lhecker commented Jun 29, 2021

DHowett commented Jun 29, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost commented Jun 30, 2021

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.

lhecker commented Jun 25, 2021 •

edited

Loading

lhecker Jun 28, 2021 •

edited

Loading

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (`@msftbot`) and give me an instruction to get started! Learn more here.