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

Improve configure user experience #3158

Merged
merged 4 commits into from
Apr 17, 2023
Merged
Changes from 1 commit
Commits
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
Next Next commit
config UX improvements (check file exists first, log file contents, p…
…rogress info)
JohnMcPMS committed Apr 16, 2023

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
commit 0d8e86122bcc30d5d8c3288118763d30a2a66807
1 change: 0 additions & 1 deletion src/AppInstallerCLICore/ChannelStreams.cpp
Original file line number Diff line number Diff line change
@@ -6,7 +6,6 @@

namespace AppInstaller::CLI::Execution
{
using namespace Settings;
using namespace VirtualTerminal;

size_t GetConsoleWidth()
2 changes: 2 additions & 0 deletions src/AppInstallerCLICore/Command.cpp
Original file line number Diff line number Diff line change
@@ -4,6 +4,8 @@
#include "Command.h"
#include "Resources.h"
#include <winget/UserSettings.h>
#include <AppInstallerRuntime.h>
#include <winget/Locale.h>

using namespace std::string_view_literals;
using namespace AppInstaller::Utility::literals;
1 change: 1 addition & 0 deletions src/AppInstallerCLICore/Commands/ConfigureCommand.cpp
Original file line number Diff line number Diff line change
@@ -54,6 +54,7 @@ namespace AppInstaller::CLI
void ConfigureCommand::ExecuteInternal(Execution::Context& context) const
{
context <<
EnsureConfigurationFileExists <<
CreateConfigurationProcessor <<
OpenConfigurationSet <<
ShowConfigurationSet <<
Original file line number Diff line number Diff line change
@@ -35,6 +35,7 @@ namespace AppInstaller::CLI
void ConfigureShowCommand::ExecuteInternal(Execution::Context& context) const
{
context <<
EnsureConfigurationFileExists <<
CreateConfigurationProcessor <<
OpenConfigurationSet <<
ShowConfigurationSet;
1 change: 1 addition & 0 deletions src/AppInstallerCLICore/Commands/RootCommand.cpp
Original file line number Diff line number Diff line change
@@ -2,6 +2,7 @@
// Licensed under the MIT License.
#include "pch.h"
#include "RootCommand.h"
#include <AppInstallerRuntime.h>

#include "InstallCommand.h"
#include "ShowCommand.h"
1 change: 1 addition & 0 deletions src/AppInstallerCLICore/Commands/ValidateCommand.cpp
Original file line number Diff line number Diff line change
@@ -5,6 +5,7 @@
#include "Workflows/WorkflowBase.h"
#include "Workflows/DependenciesFlow.h"
#include "Resources.h"
#include <winget/ManifestYamlParser.h>

namespace AppInstaller::CLI
{
3 changes: 2 additions & 1 deletion src/AppInstallerCLICore/CompletionData.cpp
Original file line number Diff line number Diff line change
@@ -3,12 +3,13 @@
#include "pch.h"
#include "CompletionData.h"
#include "Resources.h"
#include <AppInstallerLogging.h>
#include <AppInstallerErrors.h>

namespace AppInstaller::CLI
{
using namespace std::string_view_literals;
using namespace Utility::literals;
using namespace Settings;

// Completion takes in the following values:
// Word :: The token from the command line that is being targeted for completion.
Original file line number Diff line number Diff line change
@@ -2,6 +2,8 @@
// Licensed under the MIT License.
#include "pch.h"
#include "ConfigurationSetProcessorFactoryRemoting.h"
#include <AppInstallerLogging.h>
#include <AppInstallerRuntime.h>

using namespace winrt::Windows::Foundation;
using namespace winrt::Microsoft::Management::Configuration;
10 changes: 8 additions & 2 deletions src/AppInstallerCLICore/ExecutionProgress.cpp
Original file line number Diff line number Diff line change
@@ -166,7 +166,12 @@ namespace AppInstaller::CLI::Execution

void ProgressVisualizerBase::SetMessage(std::string_view message)
{
m_message = message;
std::atomic_store(&m_message, std::make_shared<std::string>(message));
}

std::shared_ptr<std::string> ProgressVisualizerBase::GetMessage()
{
return std::atomic_load(&m_message);
}
}

@@ -213,7 +218,8 @@ namespace AppInstaller::CLI::Execution
ApplyStyle(i % repetitionCount, repetitionCount, true);
m_out << '\r' << indent << spinnerChars[i % ARRAYSIZE(spinnerChars)];
m_out.RestoreDefault();
m_out << ' ' << m_message << std::flush;
std::shared_ptr<std::string> message = this->GetMessage();
m_out << ' ' << (message ? *message : std::string{}) << std::flush;
Sleep(250);
}

4 changes: 3 additions & 1 deletion src/AppInstallerCLICore/ExecutionProgress.h
Original file line number Diff line number Diff line change
@@ -11,6 +11,7 @@
#include <atomic>
#include <future>
#include <istream>
#include <memory>
#include <ostream>
#include <string>
#include <vector>
@@ -28,11 +29,11 @@ namespace AppInstaller::CLI::Execution
void SetStyle(AppInstaller::Settings::VisualStyle style) { m_style = style; }

void SetMessage(std::string_view message);
std::shared_ptr<std::string> GetMessage();

protected:
BaseStream& m_out;
Settings::VisualStyle m_style = AppInstaller::Settings::VisualStyle::Accent;
std::string m_message;

bool UseVT() const { return m_enableVT && m_style != AppInstaller::Settings::VisualStyle::NoVT; }

@@ -43,6 +44,7 @@ namespace AppInstaller::CLI::Execution

private:
bool m_enableVT = false;
std::shared_ptr<std::string> m_message;
};
}

42 changes: 42 additions & 0 deletions src/AppInstallerCLICore/ExecutionReporter.cpp
Original file line number Diff line number Diff line change
@@ -2,6 +2,7 @@
// Licensed under the MIT License.
#include "pch.h"
#include "ExecutionReporter.h"
#include <AppInstallerErrors.h>


namespace AppInstaller::CLI::Execution
@@ -246,9 +247,50 @@ namespace AppInstaller::CLI::Execution
GetBasicOutputStream() << VirtualTerminal::Cursor::Visibility::EnableShow;
};

Reporter::AsyncProgressScope::AsyncProgressScope(Reporter& reporter, IProgressSink* sink, bool hideProgressWhenDone) :
m_reporter(reporter), m_callback(sink)
{
reporter.SetProgressCallback(&m_callback);
sink->BeginProgress();
m_hideProgressWhenDone = hideProgressWhenDone;
}

Reporter::AsyncProgressScope::~AsyncProgressScope()
{
m_reporter.get().SetProgressCallback(nullptr);
m_callback.GetSink()->EndProgress(m_hideProgressWhenDone);
}

ProgressCallback& Reporter::AsyncProgressScope::Callback()
{
return m_callback;
}

IProgressCallback* Reporter::AsyncProgressScope::operator->()
{
return &m_callback;
}

bool Reporter::AsyncProgressScope::HideProgressWhenDone() const
{
return m_hideProgressWhenDone;
}

void Reporter::AsyncProgressScope::HideProgressWhenDone(bool value)
{
m_hideProgressWhenDone.store(value);
}

std::unique_ptr<Reporter::AsyncProgressScope> Reporter::BeginAsyncProgress(bool hideProgressWhenDone)
{
return std::make_unique<AsyncProgressScope>(*this, m_progressSink.load(), hideProgressWhenDone);
}

void Reporter::SetProgressCallback(ProgressCallback* callback)
{
auto lock = m_progressCallbackLock.lock_exclusive();
// Attempting two progress operations at the same time; not supported.
THROW_HR_IF(HRESULT_FROM_WIN32(ERROR_INVALID_STATE), m_progressCallback != nullptr && callback != nullptr);
m_progressCallback = callback;
}

41 changes: 30 additions & 11 deletions src/AppInstallerCLICore/ExecutionReporter.h
Original file line number Diff line number Diff line change
@@ -13,6 +13,7 @@
#include <atomic>
#include <iomanip>
#include <istream>
#include <memory>
#include <optional>
#include <ostream>
#include <string>
@@ -116,23 +117,41 @@ namespace AppInstaller::CLI::Execution
void SetProgressMessage(std::string_view message) override;
void EndProgress(bool hideProgressWhenDone) override;

// Contains the objects used for async progress and the lifetime of said progress.
struct AsyncProgressScope
{
AsyncProgressScope(Reporter& reporter, IProgressSink* sink, bool hideProgressWhenDone);
~AsyncProgressScope();

AsyncProgressScope(const AsyncProgressScope&) = delete;
AsyncProgressScope& operator=(const AsyncProgressScope&) = delete;

AsyncProgressScope(AsyncProgressScope&&) = delete;
AsyncProgressScope& operator=(AsyncProgressScope&&) = delete;

ProgressCallback& Callback();
IProgressCallback* operator->();

bool HideProgressWhenDone() const;
void HideProgressWhenDone(bool value);

private:
std::reference_wrapper<Reporter> m_reporter;
ProgressCallback m_callback;
std::atomic_bool m_hideProgressWhenDone;
};

// Runs the given callable of type: auto(IProgressCallback&)
template <typename F>
auto ExecuteWithProgress(F&& f, bool hideProgressWhenDone = false)
{
IProgressSink* sink = m_progressSink.load();
ProgressCallback callback(sink);
SetProgressCallback(&callback);
sink->BeginProgress();

auto hideProgress = wil::scope_exit([this, hideProgressWhenDone]()
{
SetProgressCallback(nullptr);
m_progressSink.load()->EndProgress(hideProgressWhenDone);
});
return f(callback);
auto progressScope = BeginAsyncProgress(hideProgressWhenDone);
return f(progressScope->Callback());
}

// Begins an asynchronous progress operation.
std::unique_ptr<AsyncProgressScope> BeginAsyncProgress(bool hideProgressWhenDone = false);

// Sets the in progress callback.
void SetProgressCallback(ProgressCallback* callback);

6 changes: 3 additions & 3 deletions src/AppInstallerCLICore/PackageCollection.cpp
Original file line number Diff line number Diff line change
@@ -1,11 +1,11 @@
// Copyright (c) Microsoft Corporation.
// Licensed under the MIT License.
#include "pch.h"

#include "PackageCollection.h"

#include "AppInstallerRuntime.h"
#include "winget/JsonSchemaValidation.h"
#include <AppInstallerLogging.h>
#include <AppInstallerRuntime.h>
#include <winget/JsonSchemaValidation.h>

#include "PackagesSchema.h"

1 change: 1 addition & 0 deletions src/AppInstallerCLICore/PortableInstaller.cpp
Original file line number Diff line number Diff line change
@@ -10,6 +10,7 @@
#include "Microsoft/PortableIndex.h"
#include "Microsoft/Schema/IPortableIndex.h"
#include <AppInstallerErrors.h>
#include <AppInstallerRuntime.h>

using namespace AppInstaller::Utility;
using namespace AppInstaller::Registry;
3 changes: 3 additions & 0 deletions src/AppInstallerCLICore/Resources.h
Original file line number Diff line number Diff line change
@@ -58,10 +58,13 @@ namespace AppInstaller::CLI::Resource
WINGET_DEFINE_RESOURCE_STRINGID(ConfigurationFileEmpty);
WINGET_DEFINE_RESOURCE_STRINGID(ConfigurationFileInvalid);
WINGET_DEFINE_RESOURCE_STRINGID(ConfigurationFileVersionUnknown);
WINGET_DEFINE_RESOURCE_STRINGID(ConfigurationGettingDetails);
WINGET_DEFINE_RESOURCE_STRINGID(ConfigurationInform);
WINGET_DEFINE_RESOURCE_STRINGID(ConfigurationInitializing);
WINGET_DEFINE_RESOURCE_STRINGID(ConfigurationLocal);
WINGET_DEFINE_RESOURCE_STRINGID(ConfigurationModuleNameOnly);
WINGET_DEFINE_RESOURCE_STRINGID(ConfigurationModuleWithDetails);
WINGET_DEFINE_RESOURCE_STRINGID(ConfigurationReadingConfigFile);
WINGET_DEFINE_RESOURCE_STRINGID(ConfigurationSettings);
WINGET_DEFINE_RESOURCE_STRINGID(ConfigurationSuccessfullyApplied);
WINGET_DEFINE_RESOURCE_STRINGID(ConfigurationUnitFailed);
1 change: 1 addition & 0 deletions src/AppInstallerCLICore/VTSupport.cpp
Original file line number Diff line number Diff line change
@@ -2,6 +2,7 @@
// Licensed under the MIT License.
#include "pch.h"
#include "VTSupport.h"
#include <AppInstallerLogging.h>


namespace AppInstaller::CLI::VirtualTerminal
43 changes: 36 additions & 7 deletions src/AppInstallerCLICore/Workflows/ConfigurationFlow.cpp
Original file line number Diff line number Diff line change
@@ -479,10 +479,29 @@ namespace AppInstaller::CLI::Workflow
std::set<winrt::guid> m_unitsSeen;
bool m_isFirstProgress = true;
};

std::filesystem::path GetConfigurationFilePath(Execution::Context& context)
{
std::filesystem::path argPath = Utility::ConvertToUTF16(context.Args.GetArg(Args::Type::ConfigurationFile));
return std::filesystem::weakly_canonical(argPath);
}
}

void EnsureConfigurationFileExists(Execution::Context& context)
{
std::filesystem::path absolutePath = GetConfigurationFilePath(context);
if (!std::filesystem::exists(absolutePath))
{
context.Reporter.Error() << Resource::String::FileNotFound(Utility::LocIndView{ context.Args.GetArg(Args::Type::ConfigurationFile) }) << std::endl;
AICLI_TERMINATE_CONTEXT(HRESULT_FROM_WIN32(ERROR_FILE_NOT_FOUND));
}
}

void CreateConfigurationProcessor(Context& context)
{
auto progressScope = context.Reporter.BeginAsyncProgress(true);
progressScope->Callback().SetProgressMessage(Resource::String::ConfigurationInitializing());

ConfigurationProcessor processor{ CreateConfigurationSetProcessorFactory() };

// Set the processor to the current level of the logging.
@@ -507,13 +526,10 @@ namespace AppInstaller::CLI::Workflow

void OpenConfigurationSet(Context& context)
{
std::filesystem::path argPath = Utility::ConvertToUTF16(context.Args.GetArg(Args::Type::ConfigurationFile));
std::filesystem::path absolutePath = std::filesystem::weakly_canonical(argPath);
if (!std::filesystem::exists(absolutePath))
{
context.Reporter.Error() << Resource::String::FileNotFound << std::endl;
AICLI_TERMINATE_CONTEXT(HRESULT_FROM_WIN32(ERROR_FILE_NOT_FOUND));
}
auto progressScope = context.Reporter.BeginAsyncProgress(true);
progressScope->Callback().SetProgressMessage(Resource::String::ConfigurationReadingConfigFile());

std::filesystem::path absolutePath = GetConfigurationFilePath(context);

Streams::IInputStream inputStream = nullptr;
inputStream = Streams::FileRandomAccessStream::OpenAsync(absolutePath.wstring(), FileAccessMode::Read).get();
@@ -561,6 +577,10 @@ namespace AppInstaller::CLI::Workflow
AICLI_TERMINATE_CONTEXT(S_FALSE);
}

auto gettingDetailString = Resource::String::ConfigurationGettingDetails();
auto progressScope = context.Reporter.BeginAsyncProgress(true);
progressScope->Callback().SetProgressMessage(gettingDetailString);

auto getDetailsOperation = configContext.Processor().GetSetDetailsAsync(configContext.Set(), ConfigurationUnitDetailLevel::Catalog);

OutputStream out = context.Reporter.Info();
@@ -570,19 +590,26 @@ namespace AppInstaller::CLI::Workflow
{
auto threadContext = context.SetForCurrentThread();

progressScope.reset();

auto unitResults = operation.GetResults().UnitResults();
for (unitsShown; unitsShown < unitResults.Size(); ++unitsShown)
{
GetConfigurationUnitDetailsResult unitResult = unitResults.GetAt(unitsShown);
LogFailedGetConfigurationUnitDetails(unitResult.Unit(), unitResult.ResultInformation());
OutputConfigurationUnitInformation(out, unitResult.Unit());
}

progressScope = context.Reporter.BeginAsyncProgress(true);
progressScope->Callback().SetProgressMessage(gettingDetailString);
});

try
{
GetConfigurationSetDetailsResult result = getDetailsOperation.get();

progressScope.reset();

// Handle any missing progress callbacks
auto unitResults = result.UnitResults();
for (unitsShown; unitsShown < unitResults.Size(); ++unitsShown)
@@ -594,6 +621,8 @@ namespace AppInstaller::CLI::Workflow
}
CATCH_LOG();

progressScope.reset();

// In the event of an exception from GetSetDetailsAsync, show the data we do have
if (!unitsShown)
{
6 changes: 6 additions & 0 deletions src/AppInstallerCLICore/Workflows/ConfigurationFlow.h
Original file line number Diff line number Diff line change
@@ -5,6 +5,12 @@

namespace AppInstaller::CLI::Workflow
{
// Fails if the configuration file path does not exist.
// Required Args: ConfigurationFile
// Inputs: None
// Outputs: None
void EnsureConfigurationFileExists(Execution::Context& context);
Copy link
Contributor

Choose a reason for hiding this comment

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

This name of this almost makes me think that this could be abstracted into a generic check for files existing where an enum could provide the logic of which arg to grab the path from . winget import is one that comes to mind, install/uninstall/show with --manifest, winget hash, etc. I'm surprised this isn’t genericised already. . .

Copy link
Member Author

Choose a reason for hiding this comment

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

It actually is, and I wrote it. I just didn't look hard enough to rediscover VerifyFile.


// Composite flow that chooses what to do based on the installer type.
// Required Args: None
// Inputs: None
Loading