Reduce usage of Json::Value throughout Terminal.Settings.Model by lhecker · Pull Request #11184 · microsoft/terminal

lhecker · 2021-09-09T00:49:33Z

This commit reduces the code surface that interacts with raw JSON data,
reducing code complexity and improving maintainability.
Files that needed to be changed drastically were additionally
cleaned up to remove any code cruft that has accrued over time.

In order to facility this the following changes were made:

Move JSON handling from CascadiaSettings into SettingsLoader
This allows us to use STL containers for data model instances.
For instance profiles are now added to a hashmap for O(1) lookup.
JSON parsing within SettingsLoader doesn't differentiate between user,
inbox and fragment JSON data, reducing code complexity and size.
It also centralizes common concerns, like profile deduplication and
ensuring that all profiles are assigned a GUID.
Direct JSON modification, like the insertion of dynamic profiles into
settings.json were removed. This vastly reduces code complexity,
but unfortunately removes support for comments in JSON on first start.
ColorSchemes cannot be layered. As such its LayerJson API was replaced
with FromJson, allowing us to remove JSON-based color scheme validation.
Profiles used to test their wish to layer using ShouldBeLayered, which
was replaced with a GUID-based hashmap lookup on previously parsed profiles.

Further changes were made as improvements upon the previous changes:

Compact the JSON files embedded binary, saving 28kB
Prevent double-initialization of the color table in ColorScheme
Making til::color getters constexpr, allow better optimizations

The result is a reduction of:

48kB binary size for the Settings.Model.dll
5-10% startup duration
26% code for the CascadiaSettings class
1% overall code in this project

Furthermore this results in the following breaking changes:

The long deprecated "globals" settings object will not be detected and no
warning will be created during load.
The initial creation of a new settings.json will not produce helpful comments.

Both cases are caused by the removal of manual JSON handling and the
move to representing the settings file with model objects instead

PR Checklist

Closes Add support for profiles.defaults in defaults.json #5276
Closes Generate GUID's when it's requested (Profile) #7421
I work here
Tests added/passed

Validation Steps Performed

Out-of-box-experience is identical to before ✔️
(Except for the settings.json file lacking comments.)
Existing user settings load correctly ✔️
New WSL instances are added to user settings ✔️
New fragments are added to user settings ✔️
All profiles are assigned GUIDs ✔️

DHowett · 2021-09-09T13:46:30Z

src/cascadia/TerminalSettingsEditor/Launch.xaml

                                        x:Load="false">
                    <ComboBox x:Name="DefaultTerminal"
-                              ItemsSource="{x:Bind State.Settings.DefaultTerminals, Mode=OneWay}"
+                              ItemsSource="{x:Bind State.Settings.DefaultTerminals}"


Hmm.. why the change here? I guess we never reload this in practice...

When I started testing my changes, the SUI was crashing on load and with @carlos-zamora I was able to determine that I made a mistake with my DefaultTerminals() implementation. After fixing my issue only this change remained. I believe this change to be correct as DefaultTerminals is unable to post updates (it's a pull-based API and cannot "push" updates).
But I do want to minimize the PR complexity and would thus be entirely happy to revert any change we deem unnecessary.

I think this is fine? Only concern is if we're still updating the ItemsSource when we reload the page. But that's more a concern in Launch.cpp::OnNavigatedTo(). Mind double checking that?

Yep it refreshes _State.

DHowett

48/55 files reviewed! Would you mind sharing the output from running the LocalTests harness?

src/cascadia/TerminalSettingsModel/userDefaults.json

src/cascadia/TerminalSettingsModel/Profile.cpp

DHowett · 2021-09-09T18:50:18Z

src/cascadia/TerminalSettingsModel/Profile.h

-        static com_ptr<Profile> CloneInheritanceGraph(com_ptr<Profile> oldProfile, com_ptr<Profile> newProfile, std::unordered_map<void*, com_ptr<Profile>>& visited);
-        static com_ptr<Profile> CopySettings(com_ptr<Profile> source);
-        static void InsertParentHelper(com_ptr<Profile> child, com_ptr<Profile> parent, std::optional<size_t> index = std::nullopt);
+        static void CopyInheritanceGraph(std::unordered_map<const Profile*, winrt::com_ptr<Profile>>& visited, const std::vector<winrt::com_ptr<Profile>>& source, std::vector<winrt::com_ptr<Profile>>& target);


The reduction of visited from many to one suggests that we'll have an inheritance issue -- you may need to explain why there are no longer multiple parents or interlinked branches in the tree.

I've added a comment and renamed the two functions to make the intent more clear.
But they're still a basic algorithm to clone a directed acyclic graph now.

What I did here is to split up the previous function, which did "clone this node and its sub-graph", into:

"clone this node and its sub-graph" and

"clone these child-nodes and their sub-graphs"

The former uses the latter to clone the children of a node, whereas the latter uses the former to clone each child-node in a loop. The recursion thus now hops back and forth between two functions. This simplifies the implementation a lot.

DHowett · 2021-09-09T18:52:59Z

src/cascadia/LocalTests_SettingsModel/SerializationTests.cpp

-        settings->_ApplyDefaultsFromUserSettings();
-        settings->LayerJson(settings->_userSettings);
-        settings->_ValidateSettings();
+            "$help" : "https://aka.ms/terminal-documentation",


CURIOUS! What does this do?

Since I removed the helpful comments from userDefaults.json, I thought a documentation link might be useful for some. It's the first field in the settings.json.

src/cascadia/TerminalApp/AppLogic.cpp

src/cascadia/TerminalSettingsModel/AzureCloudShellGenerator.h

src/cascadia/TerminalSettingsModel/GlobalAppSettings.h

DHowett · 2021-09-09T19:06:57Z

src/cascadia/LocalTests_SettingsModel/DeserializationTests.cpp

-        VERIFY_ARE_EQUAL(L"NeitherShouldThisOne", settings->_allProfiles.GetAt(4).Name());
-    }
-
-    void DeserializationTests::TestExplodingNameOnlyProfiles()


How do we validate that this invariant is still true?

I considered this test in particular a duplicate of TestLayeringNameOnlyProfiles which is extremely similar. So I only fixed one of the two tests. Did I maybe miss something?

carlos-zamora

Ok! I've taken a look at everything except...

the tests
Profile: copying the inheritance graph

A few memorable notes:

Take a look at #7421 and #5276. You might be able to close them out.
userDefaults.json still needs those comments. I didn't check if we're still populating the special variables in there (or I've forgotten how you do it). But we have to make sure we don't break that.
SettingsLoader would be extra nice if we moved away from knowing which layer we're in. I envision a world where we can easily load a chain of files. That's basically what you're already doing with fragments. So might as well change the wording a bit to check that off the list.

src/cascadia/TerminalSettingsModel/userDefaults.json

src/cascadia/TerminalSettingsModel/DefaultProfileUtils.h

src/cascadia/TerminalSettingsModel/TerminalWarnings.idl

src/cascadia/TerminalApp/AppLogic.cpp

src/cascadia/TerminalSettingsModel/CascadiaSettingsSerialization.cpp

carlos-zamora · 2021-09-10T00:58:35Z

src/cascadia/TerminalSettingsModel/CascadiaSettingsSerialization.cpp

+    if (activeProfiles.empty())
    {
-        pScheme->LayerJson(schemeJson);
+        throw SettingsException(SettingsLoadErrors::AllProfilesHidden);


What if a user only wants their dynamic/fragment profiles? Won't that trigger this exception?

For sure! But this code is equivalent to the current code.

src/cascadia/TerminalSettingsModel/Profile.cpp

carlos-zamora

Ok. Reviewed everything except 4 of the tests. But I'll get to those when the PR is out of draft.

README.md

src/cascadia/TerminalSettingsModel/Profile.cpp

carlos-zamora · 2021-09-10T18:06:28Z

src/cascadia/TerminalSettingsModel/Profile.cpp

+    {
+        clone = CopySettings();
+        CopyInheritanceGraph(visited, _parents, clone->_parents);
+        clone->_FinalizeInheritance();


Don't we also need to add the clone to visited here? That way if we encounter it again, clone will be populated above?

or just always emplace in CopyInheritanceGraph.

Ha! This is some evil C++ abuse! 😄
I'm using the operator[] a few lines above to get a reference a node in visited.
If the node doesn't exist the operator[] will default-initialize it (which is an empty com_ptr). In this if-condition I'm checking for emptiness and then assign clone with a clone. Since clone is a reference into the hashmap it writes it into the hashmap directly.

src/cascadia/TerminalSettingsModel/Profile.cpp

src/cascadia/CascadiaPackage/Package-Dev.appxmanifest

DHowett · 2021-09-14T16:49:47Z

You gotta write up a PR body!

DHowett · 2021-09-14T21:23:19Z

src/cascadia/TerminalSettingsModel/AzureCloudShellGenerator.cpp

-        azureCloudShellProfile.ConnectionType(AzureConnection::ConnectionType());
-        profiles.emplace_back(azureCloudShellProfile);
+        auto azureCloudShellProfile{ CreateDefaultProfile(AzureGeneratorNamespace, L"Azure Cloud Shell") };
+        azureCloudShellProfile->Commandline(L"Azure");


We don't. It's just ballast.

src/cascadia/TerminalSettingsModel/CascadiaSettingsSerialization.cpp

DHowett · 2021-09-14T21:30:20Z

src/cascadia/TerminalSettingsModel/CascadiaSettingsSerialization.cpp

        co_await winrt::resume_background();
-        auto v{ co_await task };
+        finalVal.emplace(co_await task);
+        latch.count_down();


thanks for making this use latch ;P

src/cascadia/TerminalSettingsModel/CascadiaSettingsSerialization.cpp

DHowett · 2021-09-14T21:41:42Z

src/cascadia/TerminalSettingsModel/CascadiaSettingsSerialization.cpp

-        if (fullFile.isMember(JsonKey(SchemesKey)))
+    // profiles.defaults
+    {
+        settings.baseLayerProfile = Profile::FromJson(defaultsObject);


This works even if defaults has been deleted (and is therefore a json null), correct?

Yeah. Most tests don't specify a defaults object for instance.

src/cascadia/TerminalSettingsModel/CascadiaSettings.cpp

DHowett · 2021-09-17T20:11:25Z

@msftbot make sure @zadjii-msft signs off

ghost · 2021-09-17T20:11:29Z

Hello @DHowett!

Because you've given me some instructions on how to help merge this pull request, I'll be modifying my merge approach. Here's how I understand your requirements for merging this pull request:

I'll only merge this pull request if it's approved by @zadjii-msft

If this doesn't seem right to you, you can tell me to cancel these instructions and use the auto-merge policy that has been configured for this repository. Try telling me "forget everything I just told you".

DHowett

This feels robust and effective, and I appreciate that I can now almost keep all the logic in my head at once. I've bothered Leonard numerous times during the evenings to tell him that I found "yet another" issue, and... I think I'm out of issues. I can't complain about the code style or the comments any more than I have, and I think this is ready to go.

@carlos-zamora, can you re-review?

I want 3 people to sign off on this as it's a big one. @zadjii-msft is my designated third. 😄

carlos-zamora

I suggest also updating the PR body to mention any "breaking" changes we're ok with. That way it's a lot easier to track than having to look through this PR again. Some that come to mind are:

removal of "globals" warning
removal of comments on initial settings.json load

All the other comments I have don't have anything to do with the logic of this code. Happy to see this merge soon!

src/cascadia/TerminalSettingsModel/DefaultProfileUtils.h

src/cascadia/TerminalSettingsModel/DefaultProfileUtils.cpp

carlos-zamora · 2021-09-20T18:38:49Z

src/cascadia/TerminalSettingsModel/CascadiaSettings.h

+        ParsedSettings inboxSettings;
+        ParsedSettings userSettings;


(bump. Still want some kind of resolution on this one.)

src/cascadia/TerminalSettingsModel/CascadiaSettingsSerialization.cpp

src/cascadia/TerminalSettingsModel/CascadiaSettings.cpp

src/cascadia/TerminalSettingsModel/ColorScheme.h

src/cascadia/TerminalSettingsModel/CascadiaSettingsSerialization.cpp

lhecker · 2021-09-20T23:32:37Z

@carlos-zamora Due to the large number of comments and GitHub collapsing them (y u do dis?) let me comment here regarding inboxSettings/parentSettings & userSettings/childSettings:

Would it be possible to rename it to something like this? That way, if we ever want to add a chain of layers, we can reuse this?

I'd like to refrain from this for now. I feel like that a name that's as specific as possible helps newcomers to understand the code more easily. It also allows us to develop a mental model that solves exactly just the problem of only solving the problem of "merging builtin with user-given settings", instead of creating a more general solution. In my personal opinion it should be easy for any of us to rename the member variables and functions to something else without involving sweeping changes. After all the inbox/user terminology is concentrated entirely in SettingsLoader (and the CascadiaSettings constructor) and thus doesn't require us to touch public API code.

…json-cleanup

zadjii-msft

46/67. Finished CascadiaSettingsSerialization, I suspect most of the remainder is tests. Boy GH does not like reviewing this diff thou, so big

src/cascadia/TerminalSettingsModel/AppearanceConfig.cpp

zadjii-msft · 2021-09-21T15:56:47Z

src/cascadia/TerminalApp/Resources/en-US/Resources.resw

-  <data name="LegacyGlobalsProperty" xml:space="preserve">
-    <value>The "globals" property is deprecated - your settings might need updating. </value>
-    <comment>{Locked="\"globals\""} </comment>
-  </data>
-  <data name="LegacyGlobalsPropertyHrefUrl" xml:space="preserve">
-    <value>https://go.microsoft.com/fwlink/?linkid=2128258</value>
-    <comment>{Locked}This is a FWLink, so it will be localized with the fwlink tool</comment>
-  </data>
-  <data name="LegacyGlobalsPropertyHrefLabel" xml:space="preserve">
-    <value>For more info, see this web page.</value>
-  </data>


bump for an answer to this one @DHowett

zadjii-msft · 2021-09-21T15:59:03Z

src/cascadia/TerminalSettingsModel/CascadiaSettings.h

+        ParsedSettings inboxSettings;
+        ParsedSettings userSettings;


meh, if there's a chain, then there's gonna be more than two anyways, so parent/child doesn't really matter, yea? it's just gonna be a vector of layers

src/cascadia/TerminalSettingsModel/CascadiaSettingsSerialization.cpp

zadjii-msft · 2021-09-21T17:35:38Z

src/cascadia/TerminalSettingsModel/CascadiaSettingsSerialization.cpp

-    if (!isDefaultSettings)
+    // FYI: The static_cast ensures we don't move the profile into
+    // `profilesByGuid`, even though we still need it later for `profiles`.
+    if (settings.profilesByGuid.emplace(profile->Guid(), static_cast<const winrt::com_ptr<Profile>&>(profile)).second)


If i'm getting this right, say we have two profiles in the settings.json:

{ "name": "foo" }, { "name": "foo", "background": "#123456" },

we'll ignore the second one entirely, because it's a duplicate (and later use duplicateProfile to raise a warning?)

Don't we currently just layer the second one on top of the first?

We can restore this old behavior if we'd like. I just found it somewhat inconsistent with the duplicateProfile warning and assumed that the likelihood of duplicate profiles in a single file is low.
I'll add a note to the PR body.

Should I restore the old behavior?

zadjii-msft · 2021-09-21T17:58:15Z

src/cascadia/TerminalSettingsModel/CascadiaSettingsSerialization.cpp

+bool SettingsLoader::DisableDeletedProfiles()
+{
+    const auto& state = winrt::get_self<ApplicationState>(ApplicationState::SharedInstance());
+    auto generatedProfileIds = state->GeneratedProfiles();


can GeneratedProfiles be null (like the first time the file is created, for ex)?

No it returns a STL class actually (which is why I use get_self here).

zadjii-msft · 2021-09-21T18:08:19Z

src/cascadia/TerminalSettingsModel/CascadiaSettingsSerialization.cpp

-            _CatchRethrowSerializationExceptionWithLocationInfo(resultPtr->_userSettingsString);
+            // If inserted is true, then this is a generated profile that doesn't exist in the user's settings.
+            // While emplace() has already created an appropriate entry in .profilesByGuid, we still need to
+            // add it to .profiles (which is basically a sorted list of .profilesByGuid's values).


AH okay we CreateChild here because there wasn't an entry in the user's settings for this default profile. The child now acts like it belongs in the user settings, so when we write the file back out, we'll actually serialize that child to settings.json. Okay, that now makes some sense. Had to read this and the fragments version a few times to figure out why we were making a child here

Added a comment. 👍

zadjii-msft

I'm only blocking for:

The Opacity thing
The resources thing

y'all can feel free to dismiss me if that's sorted out after 5pm Wisconsin time.

I feel sad at the general loss of the json patching - I once had a mind to just do json patching for all settings writing, but the SUI is good enough at this point that it doesn't really matter anymore. I'll miss the comments, but whatever.

src/cascadia/TerminalSettingsModel/Profile.cpp

zadjii-msft · 2021-09-21T20:25:07Z

src/cascadia/TerminalSettingsModel/CascadiaSettings.cpp

+    OSVERSIONINFOEXW osver{};
    osver.dwOSVersionInfoSize = sizeof(osver);
-    osver.dwBuildNumber = 21359;
+    osver.dwBuildNumber = 22000;


yea this is probably a good call

zadjii-msft · 2021-09-21T20:34:43Z

src/cascadia/ut_app/DynamicProfileTests.cpp

@@ -1,673 +0,0 @@
-// Copyright (c) Microsoft Corporation.


these guys didn't really get resurrected anywhere, did they?

TestGenGuidsForProfiles was moved into ProfileTests.cpp.
I've removed the others because the SettingsLoader makes no distinction between the source of builtin profiles: "inbox" JSON data and generated profiles are treated the exact same way. SettingsLoader::GenerateProfiles calls SettingsLoader::_executeGenerator which basically only adds generated profiles straight to said inboxSettings.profiles vector.
Considering the extensive coverage of inbox JSON related tests in SerializationTests.cpp I've thus decided to not port these tests to the new API and saved myself some work.

Would you like me to restore the tests?

lhecker · 2021-09-22T00:55:36Z

I feel sad at the general loss of the json patching [...]

Yeah me too. It might be worthwhile to re-add such support in the future actually! Considering that we had no JSON comment support for our SUI convinced me that it's not a significant loss if we remove the remaining support for it (basically: splicing generated profiles into the settings file).
Unfortunately our current JSON comment support depends on JsonCPP which I wouldn't call a library I enjoy. Alternatives like the excellent nlohmann/json library don't support the preservation of comments the way we'd like to at all unfortunately.

zadjii-msft

RIP literally every pr

ghost · 2021-09-22T15:59:43Z

Hello @zadjii-msft!

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.

This commit fixes various failing TestHostApp unit tests. Most of these broke as part of 168d28b (#11184). ## PR Checklist * [x] Closes #11339 * [x] I work here * [x] Tests added/passed

lhecker added 2 commits September 9, 2021 02:46

Reduce usage of Json::Value throughout Terminal.Settings.Model

20b91fb

Fix most unit tests

64896e5

DHowett reviewed Sep 9, 2021

View reviewed changes

Fix several issues

1efe485

lhecker force-pushed the dev/lhecker/settings-json-cleanup branch from 7412dfc to 1efe485 Compare September 9, 2021 18:04

DHowett reviewed Sep 9, 2021

View reviewed changes

zadjii-msft self-assigned this Sep 9, 2021

carlos-zamora requested changes Sep 10, 2021

View reviewed changes

ghost added Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something and removed Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something labels Sep 10, 2021

Address Dustin's comments

cdf07bb

carlos-zamora requested changes Sep 10, 2021

View reviewed changes

ghost added Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something and removed Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something labels Sep 10, 2021

Address Carlos' comments

2247297

miniksa reviewed Sep 13, 2021

View reviewed changes

src/cascadia/CascadiaPackage/Package-Dev.appxmanifest Outdated Show resolved Hide resolved

Bug fixes sponsored by Dustin-chan

451d7d5

lhecker added 2 commits September 14, 2021 21:33

Address Carlos' comments & Add code comments

55fb5f3

Fix typo

35b65e6

Allow return value optimizations

f1b20cf

lhecker marked this pull request as ready for review September 14, 2021 20:33

Merge branch 'main' into dev/lhecker/settings-json-cleanup

e2b6e64

DHowett requested changes Sep 14, 2021

View reviewed changes

ghost added Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something and removed Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something labels Sep 14, 2021

ghost added the AutoMerge Marked for automatic merge by the bot when requirements are met label Sep 17, 2021

DHowett removed the AutoMerge Marked for automatic merge by the bot when requirements are met label Sep 17, 2021

DHowett approved these changes Sep 17, 2021

View reviewed changes

carlos-zamora approved these changes Sep 20, 2021

View reviewed changes

lhecker added 2 commits September 21, 2021 01:37

Address Carlos' comments

c4dfb78

Merge remote-tracking branch 'origin/main' into dev/lhecker/settings-…

1aae0ef

…json-cleanup

zadjii-msft reviewed Sep 21, 2021

View reviewed changes

zadjii-msft requested changes Sep 21, 2021

View reviewed changes

ghost added Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something and removed Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something labels Sep 21, 2021

Address Mike's comments

fa9d0d5

This comment has been minimized.

Sign in to view

lhecker added 2 commits September 22, 2021 03:04

German ain't English

2f3d43a

Restore Resources.resw

55c62b4

zadjii-msft approved these changes Sep 22, 2021

View reviewed changes

zadjii-msft removed their assignment Sep 22, 2021

zadjii-msft added the AutoMerge Marked for automatic merge by the bot when requirements are met label Sep 22, 2021

ghost merged commit 168d28b into main Sep 22, 2021

ghost deleted the dev/lhecker/settings-json-cleanup branch September 22, 2021 16:27

zadjii-msft mentioned this pull request Sep 23, 2021

Add pwsh to the pre reqs for building the code #11324

Closed

PankajBhojwani mentioned this pull request Sep 27, 2021

One broken stub in a fragment file causes the whole file to not load #11330

Closed

zadjii-msft mentioned this pull request Sep 28, 2021

[1.12] Setting opacity as profiles.defaults doesn't work #11355

Closed

lhecker mentioned this pull request Oct 1, 2021

Fix failing TestHostApp unit tests #11394

Merged

3 tasks

DHowett mentioned this pull request Oct 25, 2021

Re-registered WSL distro profiles do not get auto generated on terminal restart unless state.json is deleted. #11510

Closed

This pull request was closed.

Conversation

lhecker commented Sep 9, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Checklist

Validation Steps Performed

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DHowett left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

carlos-zamora left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

carlos-zamora left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

DHowett commented Sep 14, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lhecker Sep 15, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

lhecker commented Sep 9, 2021 •

edited

Loading

lhecker Sep 15, 2021 •

edited

Loading

DHowett left a comment •

edited

Loading