diff --git a/src/client/cli/cmd/animated_spinner.cpp b/src/client/cli/cmd/animated_spinner.cpp index 2373ecd45ef..2320e21af83 100644 --- a/src/client/cli/cmd/animated_spinner.cpp +++ b/src/client/cli/cmd/animated_spinner.cpp @@ -16,6 +16,7 @@ #include "animated_spinner.h" +#include #include namespace mp = multipass; @@ -29,8 +30,8 @@ void clear_line(std::ostream& out) } } // namespace -mp::AnimatedSpinner::AnimatedSpinner(std::ostream& cout) - : spinner{'|', '/', '-', '\\'}, cout{cout}, running{false} +mp::AnimatedSpinner::AnimatedSpinner(std::ostream& cout, bool is_live) + : spinner{'|', '/', '-', '\\'}, cout{cout}, running{false}, is_live(is_live) { } @@ -42,19 +43,26 @@ mp::AnimatedSpinner::~AnimatedSpinner() void mp::AnimatedSpinner::start(const std::string& start_message) { std::unique_lock lock{mutex}; + if (!running) { current_message = start_message; running = true; - clear_line(cout); - cout << start_message << " " << std::flush; - t = std::thread(&AnimatedSpinner::draw, this); + + if (is_live) + { + clear_line(cout); + cout << start_message << " " << std::flush; + t = std::thread(&AnimatedSpinner::draw, this); + } + else + cout << start_message << std::endl; } } void mp::AnimatedSpinner::start() { - if (!current_message.empty()) + if (is_live && !current_message.empty()) start(current_message); } @@ -64,12 +72,17 @@ void mp::AnimatedSpinner::stop() if (running) { running = false; - cv.notify_one(); - lock.unlock(); - if (t.joinable()) - t.join(); + if (is_live) + { + cv.notify_one(); + lock.unlock(); + if (t.joinable()) + t.join(); + } } - clear_line(cout); + + if (is_live) + clear_line(cout); } void mp::AnimatedSpinner::print(std::ostream& stream, const std::string& message) @@ -84,6 +97,9 @@ void mp::AnimatedSpinner::print(std::ostream& stream, const std::string& message void mp::AnimatedSpinner::draw() { std::unique_lock lock{mutex}; + + assert(is_live && "should only draw when running live"); + auto it = spinner.begin(); while (running) { @@ -92,6 +108,5 @@ void mp::AnimatedSpinner::draw() cout << "\b" << *it << std::flush; cv.wait_for(lock, std::chrono::milliseconds(100)); } - cout << "\b" - << " " << std::flush; + cout << "\b" << " " << std::flush; } diff --git a/src/client/cli/cmd/animated_spinner.h b/src/client/cli/cmd/animated_spinner.h index 58c18eba9ff..d1b7f86035d 100644 --- a/src/client/cli/cmd/animated_spinner.h +++ b/src/client/cli/cmd/animated_spinner.h @@ -27,7 +27,7 @@ namespace multipass class AnimatedSpinner { public: - explicit AnimatedSpinner(std::ostream& cout); + explicit AnimatedSpinner(std::ostream& cout, bool is_live = true); ~AnimatedSpinner(); void start(const std::string& message); @@ -44,5 +44,6 @@ class AnimatedSpinner std::mutex mutex; std::condition_variable cv; std::thread t; + const bool is_live; }; } // namespace multipass diff --git a/src/client/cli/cmd/clone.cpp b/src/client/cli/cmd/clone.cpp index 8bde7000d09..4fde19a3e06 100644 --- a/src/client/cli/cmd/clone.cpp +++ b/src/client/cli/cmd/clone.cpp @@ -33,7 +33,7 @@ mp::ReturnCodeVariant cmd::Clone::run(ArgParser* parser) return parser->returnCodeFrom(parscode); } - AnimatedSpinner spinner{cout}; + AnimatedSpinner spinner{cout, term->cout_is_live()}; auto action_on_success = [this, &spinner](CloneReply& reply) -> ReturnCodeVariant { spinner.stop(); cout << reply.reply_message(); diff --git a/src/client/cli/cmd/launch.cpp b/src/client/cli/cmd/launch.cpp index 94f486c760c..65048ae59f8 100644 --- a/src/client/cli/cmd/launch.cpp +++ b/src/client/cli/cmd/launch.cpp @@ -468,7 +468,7 @@ mp::ReturnCodeVariant cmd::Launch::request_launch(const ArgParser* parser) { if (!spinner) spinner = std::make_unique( - cout); // Creating just in time to work around canonical/multipass#2075 + cout, term->cout_is_live()); // Creating just in time to work around canonical/multipass#2075 if (timer) timer->resume(); diff --git a/src/client/cli/cmd/mount.cpp b/src/client/cli/cmd/mount.cpp index 2030ddb990b..7638b507f27 100644 --- a/src/client/cli/cmd/mount.cpp +++ b/src/client/cli/cmd/mount.cpp @@ -78,7 +78,7 @@ mp::ReturnCodeVariant cmd::Mount::run(mp::ArgParser* parser) return parser->returnCodeFrom(ret); } - mp::AnimatedSpinner spinner{cout}; + mp::AnimatedSpinner spinner{cout, term->cout_is_live()}; auto on_success = [&spinner](mp::MountReply& reply) -> ReturnCodeVariant { spinner.stop(); diff --git a/src/client/cli/cmd/restart.cpp b/src/client/cli/cmd/restart.cpp index 6a97424ea2d..e296fce3587 100644 --- a/src/client/cli/cmd/restart.cpp +++ b/src/client/cli/cmd/restart.cpp @@ -38,7 +38,7 @@ mp::ReturnCodeVariant cmd::Restart::run(mp::ArgParser* parser) if (ret != ParseCode::Ok) return parser->returnCodeFrom(ret); - AnimatedSpinner spinner{cout}; + AnimatedSpinner spinner{cout, term->cout_is_live()}; auto on_success = [this, &spinner](mp::RestartReply& reply) -> ReturnCodeVariant { spinner.stop(); if (term->is_live() && update_available(reply.update_info())) diff --git a/src/client/cli/cmd/restore.cpp b/src/client/cli/cmd/restore.cpp index fbb4feba93e..6b9fecb6b0e 100644 --- a/src/client/cli/cmd/restore.cpp +++ b/src/client/cli/cmd/restore.cpp @@ -31,7 +31,7 @@ mp::ReturnCodeVariant cmd::Restore::run(mp::ArgParser* parser) if (auto ret = parse_args(parser); ret != ParseCode::Ok) return parser->returnCodeFrom(ret); - AnimatedSpinner spinner{cout}; + AnimatedSpinner spinner{cout, term->cout_is_live()}; auto on_success = [this, &spinner](mp::RestoreReply& reply) -> ReturnCodeVariant { spinner.stop(); diff --git a/src/client/cli/cmd/snapshot.cpp b/src/client/cli/cmd/snapshot.cpp index 7b2ba492dc3..a3965c47a90 100644 --- a/src/client/cli/cmd/snapshot.cpp +++ b/src/client/cli/cmd/snapshot.cpp @@ -31,7 +31,7 @@ mp::ReturnCodeVariant cmd::Snapshot::run(mp::ArgParser* parser) if (auto ret = parse_args(parser); ret != ParseCode::Ok) return parser->returnCodeFrom(ret); - AnimatedSpinner spinner{cout}; + AnimatedSpinner spinner{cout, term->cout_is_live()}; auto on_success = [this, &spinner](mp::SnapshotReply& reply) -> ReturnCodeVariant { spinner.stop(); diff --git a/src/client/cli/cmd/start.cpp b/src/client/cli/cmd/start.cpp index e1bd276e32f..7c8b931b3a5 100644 --- a/src/client/cli/cmd/start.cpp +++ b/src/client/cli/cmd/start.cpp @@ -55,7 +55,7 @@ mp::ReturnCodeVariant cmd::Start::run(mp::ArgParser* parser) return parser->returnCodeFrom(ret); } - AnimatedSpinner spinner{cout}; + AnimatedSpinner spinner{cout, term->cout_is_live()}; auto on_success = [&spinner, this](mp::StartReply& reply) -> ReturnCodeVariant { spinner.stop(); diff --git a/src/client/cli/cmd/stop.cpp b/src/client/cli/cmd/stop.cpp index a94101a2042..5eb4e42fdd3 100644 --- a/src/client/cli/cmd/stop.cpp +++ b/src/client/cli/cmd/stop.cpp @@ -39,7 +39,7 @@ mp::ReturnCodeVariant cmd::Stop::run(mp::ArgParser* parser) auto on_success = [](mp::StopReply& reply) -> ReturnCodeVariant { return ReturnCode::Ok; }; - AnimatedSpinner spinner{cout}; + AnimatedSpinner spinner{cout, term->cout_is_live()}; auto on_failure = [this, &spinner](grpc::Status& status) -> ReturnCodeVariant { spinner.stop(); diff --git a/src/client/cli/cmd/suspend.cpp b/src/client/cli/cmd/suspend.cpp index 67413acd620..24d3f41ed03 100644 --- a/src/client/cli/cmd/suspend.cpp +++ b/src/client/cli/cmd/suspend.cpp @@ -38,7 +38,7 @@ mp::ReturnCodeVariant cmd::Suspend::run(mp::ArgParser* parser) auto on_success = [](mp::SuspendReply& reply) -> ReturnCodeVariant { return ReturnCode::Ok; }; - AnimatedSpinner spinner{cout}; + AnimatedSpinner spinner{cout, term->cout_is_live()}; auto on_failure = [this, &spinner](grpc::Status& status) -> ReturnCodeVariant { spinner.stop(); return standard_failure_handler_for(name(), cerr, status); diff --git a/src/client/cli/cmd/wait_ready.cpp b/src/client/cli/cmd/wait_ready.cpp index db7c8bcef65..5b695cd1ee6 100644 --- a/src/client/cli/cmd/wait_ready.cpp +++ b/src/client/cli/cmd/wait_ready.cpp @@ -36,7 +36,7 @@ mp::ReturnCodeVariant cmd::WaitReady::run(mp::ArgParser* parser) return parser->returnCodeFrom(ret); } - mp::AnimatedSpinner spinner{cout}; + mp::AnimatedSpinner spinner{cout, term->cout_is_live()}; spinner.start("Waiting for the Multipass daemon to be ready"); std::unique_ptr timer; diff --git a/tests/unit/test_common_callbacks.cpp b/tests/unit/test_common_callbacks.cpp index 53421d3b298..17fb29a6e60 100644 --- a/tests/unit/test_common_callbacks.cpp +++ b/tests/unit/test_common_callbacks.cpp @@ -17,6 +17,7 @@ #include "common.h" #include "mock_client_platform.h" #include "mock_client_rpc.h" +#include "multipass/utils.h" #include "stub_terminal.h" #include @@ -26,6 +27,7 @@ #include namespace mp = multipass; +namespace mpu = mp::utils; namespace mpt = mp::test; using namespace testing; @@ -199,3 +201,55 @@ TEST_F(TestSpinnerCallbacks, confirmationCallbackAnswers) EXPECT_THAT(err.str(), IsEmpty()); EXPECT_THAT(out.str(), clearStreamMatcher()); } + +TEST_F(TestSpinnerCallbacks, nonLiveLoggingSpinnerCallbackLogsWithoutSpinnerArtifacts) +{ + constexpr auto log = "message in a bottle"; + mp::AnimatedSpinner non_live_spinner{out, false}; + + mp::MountReply reply; + reply.set_log_line(log); + + auto cb = + mp::make_logging_spinner_callback(non_live_spinner, err); + cb(reply, nullptr); + + EXPECT_THAT(err.str(), StrEq(log)); + EXPECT_THAT(out.str(), IsEmpty()); +} + +TEST_F(TestSpinnerCallbacks, nonLiveReplySpinnerCallbackPrintsAllMessagesWithoutSpinnerArtifacts) +{ + constexpr auto log = "message in a bottle"; + constexpr auto msg = "answer"; + mp::AnimatedSpinner non_live_spinner{out, false}; + + mp::MountReply reply; + reply.set_log_line(log); + reply.set_reply_message(msg); + + auto cb = + mp::make_reply_spinner_callback(non_live_spinner, err); + cb(reply, nullptr); + + EXPECT_THAT(err.str(), StrEq(log)); + EXPECT_THAT(mpu::trim_end(out.str()), StrEq(msg)); +} + +TEST_F(TestSpinnerCallbacks, nonLiveIterativeSpinnerCallbackPrintsAllMessagesWithoutSpinnerArtifacts) +{ + constexpr auto log = "message in a bottle"; + constexpr auto msg = "answer"; + mp::AnimatedSpinner non_live_spinner{out, false}; + + mp::MountReply reply; + reply.set_log_line(log); + reply.set_reply_message(msg); + + auto cb = mp::make_iterative_spinner_callback( + non_live_spinner, term); + cb(reply, nullptr); + + EXPECT_THAT(err.str(), StrEq(log)); + EXPECT_THAT(mpu::trim_end(out.str()), StrEq(msg)); +}