From d20e63d96a4e4e9d5ed4a945ac9dd78dd8d096d6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ale=C5=A1=20Mat=C4=9Bj?= Date: Fri, 6 Dec 2024 10:53:45 +0100 Subject: [PATCH 1/2] Add padding to ProgressBar messages to avoid overlapping Add padding to fully fill the terminal_width, this is because MultiProgressBar overrides its own messages, it doesn't clear the lines. If the message is short some leftover characters could be still present after it. --- .../progressbar/download_progress_bar.cpp | 6 +++ .../test_progressbar_interactive.cpp | 52 ++++++++++++++++--- .../test_progressbar_interactive.hpp | 2 + 3 files changed, 52 insertions(+), 8 deletions(-) diff --git a/libdnf5-cli/progressbar/download_progress_bar.cpp b/libdnf5-cli/progressbar/download_progress_bar.cpp index 03416ee59..b1e7c29b9 100644 --- a/libdnf5-cli/progressbar/download_progress_bar.cpp +++ b/libdnf5-cli/progressbar/download_progress_bar.cpp @@ -205,6 +205,12 @@ void DownloadProgressBar::to_stream(std::ostream & stream) { } } + // Add padding to fully fill the terminal_width, this is because MultiProgressBar + // overrides its own messages, it doesn't clear the lines. + // If the message is short some leftover characters could be still present after it. + if (message.length() < terminal_width - 4) { + message.append(terminal_width - message.length() - 4, ' '); + } // print only part of the message that fits the terminal width // subtracted '4' relates to the '>>> ' prefix stream << message.substr(0, terminal_width - 4); diff --git a/test/libdnf5-cli/test_progressbar_interactive.cpp b/test/libdnf5-cli/test_progressbar_interactive.cpp index 40b52af67..9869e8397 100644 --- a/test/libdnf5-cli/test_progressbar_interactive.cpp +++ b/test/libdnf5-cli/test_progressbar_interactive.cpp @@ -180,8 +180,8 @@ void ProgressbarInteractiveTest::test_download_progress_bar_with_messages() { (*download_progress_bar.*get(to_stream{}))(oss); Pattern expected = "\\[0/0\\] test 40% | ????? ??B\\/s | 4.0 B | ???????\n" - ">>> test message1\n" - ">>> test message2"; + ">>> test message1 \n" + ">>> test message2 "; ASSERT_MATCHES(expected, oss.str()); download_progress_bar->pop_message(); @@ -238,7 +238,7 @@ void ProgressbarInteractiveTest::test_multi_progress_bar_with_messages_with_tota oss << multi_progress_bar; Pattern expected = "\\[1/1\\] test 40% | ????? ??B\\/s | 4.0 B | ???????\n" - ">>> test message1\n" + ">>> test message1 \n" "----------------------------------------------------------------------\n" "\\[0/1\\] Total 40% | ????? ??B\\/s | 4.0 B | ???????"; @@ -318,8 +318,8 @@ void ProgressbarInteractiveTest::test_multi_progress_bars_with_messages_with_tot Pattern expected = "\\[1/2\\] test1 100% | ????? ??B\\/s | 10.0 B | ???????\n" "\\[2/2\\] test2 40% | ????? ??B\\/s | 4.0 B | ???????\n" - ">>> test message1\n" - ">>> test message2\n" + ">>> test message1 \n" + ">>> test message2 \n" "----------------------------------------------------------------------\n" "\\[1/2\\] Total 70% | ????? ??B\\/s | 14.0 B | ???????"; @@ -363,7 +363,7 @@ void ProgressbarInteractiveTest::test_multi_progress_bar_with_messages() { oss << multi_progress_bar; Pattern expected = "\\[1/1\\] test 40% | ????? ??B\\/s | 4.0 B | ???????\n" - ">>> test message1"; + ">>> test message1 "; ASSERT_MATCHES(expected, perform_control_sequences(oss.str())); @@ -417,8 +417,8 @@ void ProgressbarInteractiveTest::test_multi_progress_bars_with_messages() { Pattern expected = "\\[1/2\\] test1 100% | ????? ??B\\/s | 10.0 B | ???????\n" "\\[2/2\\] test2 40% | ????? ??B\\/s | 4.0 B | ???????\n" - ">>> test message1\n" - ">>> test message2"; + ">>> test message1 \n" + ">>> test message2 "; ASSERT_MATCHES(expected, perform_control_sequences(oss.str())); @@ -467,3 +467,39 @@ void ProgressbarInteractiveTest::test_multi_progress_bars_with_messages() { ASSERT_MATCHES(expected, perform_control_sequences(oss.str())); } + +void ProgressbarInteractiveTest::test_multi_progress_bar_with_short_messages() { + // With single bar and Total disabled + // First print long message and remove it (successful scriptlet for package with long name), + // then print short msg (different package that prints something to the log and the message stays). + + auto download_progress_bar = std::make_unique(10, "test"); + + libdnf5::cli::progressbar::MultiProgressBar multi_progress_bar; + multi_progress_bar.set_total_bar_visible_limit(libdnf5::cli::progressbar::MultiProgressBar::NEVER_VISIBLE_LIMIT); + auto download_progress_bar_raw = download_progress_bar.get(); + multi_progress_bar.add_bar(std::move(download_progress_bar)); + + download_progress_bar_raw->set_ticks(4); + download_progress_bar_raw->set_state(libdnf5::cli::progressbar::ProgressBarState::STARTED); + download_progress_bar_raw->add_message( + libdnf5::cli::progressbar::MessageType::INFO, "test loooooooooooooooooooooooooooooooooooooooooong message"); + + std::ostringstream oss; + oss << multi_progress_bar; + Pattern expected = + "\\[1/1\\] test 40% | ????? ??B\\/s | 4.0 B | ???????\n" + ">>> test loooooooooooooooooooooooooooooooooooooooooong message "; + + ASSERT_MATCHES(expected, perform_control_sequences(oss.str())); + + download_progress_bar_raw->pop_message(); + download_progress_bar_raw->add_message(libdnf5::cli::progressbar::MessageType::INFO, "test short message"); + + oss << multi_progress_bar; + expected = + "\\[1/1\\] test 40% | ????? ??B\\/s | 4.0 B | ???????\n" + ">>> test short message "; + + ASSERT_MATCHES(expected, perform_control_sequences(oss.str())); +} diff --git a/test/libdnf5-cli/test_progressbar_interactive.hpp b/test/libdnf5-cli/test_progressbar_interactive.hpp index 2f55338f2..676ac938f 100644 --- a/test/libdnf5-cli/test_progressbar_interactive.hpp +++ b/test/libdnf5-cli/test_progressbar_interactive.hpp @@ -34,6 +34,7 @@ class ProgressbarInteractiveTest : public CppUnit::TestCase { CPPUNIT_TEST(test_multi_progress_bar_with_messages_with_total); CPPUNIT_TEST(test_multi_progress_bars_with_messages_with_total); CPPUNIT_TEST(test_multi_progress_bar_with_messages); + CPPUNIT_TEST(test_multi_progress_bar_with_short_messages); CPPUNIT_TEST(test_multi_progress_bars_with_messages); CPPUNIT_TEST_SUITE_END(); @@ -49,6 +50,7 @@ class ProgressbarInteractiveTest : public CppUnit::TestCase { void test_multi_progress_bar_with_messages_with_total(); void test_multi_progress_bars_with_messages_with_total(); void test_multi_progress_bar_with_messages(); + void test_multi_progress_bar_with_short_messages(); void test_multi_progress_bars_with_messages(); }; From 92ab80c2520e96a9e1dbcc10db536d74aa0a6bdf Mon Sep 17 00:00:00 2001 From: Evan Goode Date: Tue, 10 Dec 2024 20:33:47 +0000 Subject: [PATCH 2/2] Support ProgressBar messages with wide characters --- .../progressbar/download_progress_bar.cpp | 16 ++++++++++----- .../test_progressbar_interactive.cpp | 20 ++++++++++++++++--- 2 files changed, 28 insertions(+), 8 deletions(-) diff --git a/libdnf5-cli/progressbar/download_progress_bar.cpp b/libdnf5-cli/progressbar/download_progress_bar.cpp index b1e7c29b9..12b398ba3 100644 --- a/libdnf5-cli/progressbar/download_progress_bar.cpp +++ b/libdnf5-cli/progressbar/download_progress_bar.cpp @@ -20,6 +20,8 @@ along with libdnf. If not, see . #include "libdnf5-cli/progressbar/download_progress_bar.hpp" +#include "../utils/utf8.hpp" + #include "libdnf5-cli/tty.hpp" #include @@ -181,8 +183,10 @@ void DownloadProgressBar::to_stream(std::ostream & stream) { auto message_type = msg.first; auto message = msg.second; + const auto & prefix = ">>> "; + stream << std::endl; - stream << ">>> "; + stream << prefix; color_used = false; if (tty::is_interactive()) { @@ -208,12 +212,14 @@ void DownloadProgressBar::to_stream(std::ostream & stream) { // Add padding to fully fill the terminal_width, this is because MultiProgressBar // overrides its own messages, it doesn't clear the lines. // If the message is short some leftover characters could be still present after it. - if (message.length() < terminal_width - 4) { - message.append(terminal_width - message.length() - 4, ' '); + const auto prefix_width = libdnf5::cli::utils::utf8::width(prefix); + const auto message_width = libdnf5::cli::utils::utf8::width(message); + if (message_width < terminal_width - prefix_width) { + message.append(terminal_width - message_width - prefix_width, ' '); } + // print only part of the message that fits the terminal width - // subtracted '4' relates to the '>>> ' prefix - stream << message.substr(0, terminal_width - 4); + stream << libdnf5::cli::utils::utf8::substr_width(message, 0, terminal_width - prefix_width); if (color_used) { stream << tty::reset; diff --git a/test/libdnf5-cli/test_progressbar_interactive.cpp b/test/libdnf5-cli/test_progressbar_interactive.cpp index 9869e8397..016a6a3c2 100644 --- a/test/libdnf5-cli/test_progressbar_interactive.cpp +++ b/test/libdnf5-cli/test_progressbar_interactive.cpp @@ -112,6 +112,8 @@ void ProgressbarInteractiveTest::setUp() { setenv("DNF5_FORCE_INTERACTIVE", "1", 1); // Force columns to 70 to make output independ of where it is run setenv("FORCE_COLUMNS", "70", 1); + // Wide characters do not work at all until we set locales in the code + setlocale(LC_ALL, "C.UTF-8"); } void ProgressbarInteractiveTest::tearDown() { @@ -175,15 +177,18 @@ void ProgressbarInteractiveTest::test_download_progress_bar_with_messages() { download_progress_bar->set_state(libdnf5::cli::progressbar::ProgressBarState::STARTED); download_progress_bar->add_message(libdnf5::cli::progressbar::MessageType::INFO, "test message1"); download_progress_bar->add_message(libdnf5::cli::progressbar::MessageType::INFO, "test message2"); + download_progress_bar->add_message(libdnf5::cli::progressbar::MessageType::INFO, "test もで 諤奯ゞ"); std::ostringstream oss; (*download_progress_bar.*get(to_stream{}))(oss); Pattern expected = "\\[0/0\\] test 40% | ????? ??B\\/s | 4.0 B | ???????\n" ">>> test message1 \n" - ">>> test message2 "; + ">>> test message2 \n" + ">>> test もで 諤奯ゞ "; ASSERT_MATCHES(expected, oss.str()); + download_progress_bar->pop_message(); download_progress_bar->pop_message(); download_progress_bar->pop_message(); @@ -270,7 +275,7 @@ void ProgressbarInteractiveTest::test_multi_progress_bar_with_messages_with_tota download_progress_bar_raw->add_message(libdnf5::cli::progressbar::MessageType::INFO, "test message1"); download_progress_bar_raw->add_message(libdnf5::cli::progressbar::MessageType::INFO, "test message2"); - download_progress_bar_raw->add_message(libdnf5::cli::progressbar::MessageType::INFO, "test message3"); + download_progress_bar_raw->add_message(libdnf5::cli::progressbar::MessageType::INFO, "test もで 諤奯ゞ"); oss << multi_progress_bar; download_progress_bar_raw->pop_message(); download_progress_bar_raw->pop_message(); @@ -312,6 +317,7 @@ void ProgressbarInteractiveTest::test_multi_progress_bars_with_messages_with_tot download_progress_bar2_raw->set_state(libdnf5::cli::progressbar::ProgressBarState::STARTED); download_progress_bar2_raw->add_message(libdnf5::cli::progressbar::MessageType::INFO, "test message1"); download_progress_bar2_raw->add_message(libdnf5::cli::progressbar::MessageType::INFO, "test message2"); + download_progress_bar2_raw->add_message(libdnf5::cli::progressbar::MessageType::INFO, "test もで 諤奯ゞ"); std::ostringstream oss; oss << multi_progress_bar; @@ -320,11 +326,13 @@ void ProgressbarInteractiveTest::test_multi_progress_bars_with_messages_with_tot "\\[2/2\\] test2 40% | ????? ??B\\/s | 4.0 B | ???????\n" ">>> test message1 \n" ">>> test message2 \n" + ">>> test もで 諤奯ゞ \n" "----------------------------------------------------------------------\n" "\\[1/2\\] Total 70% | ????? ??B\\/s | 14.0 B | ???????"; ASSERT_MATCHES(expected, perform_control_sequences(oss.str())); + download_progress_bar2_raw->pop_message(); download_progress_bar2_raw->pop_message(); download_progress_bar2_raw->pop_message(); oss << multi_progress_bar; @@ -333,6 +341,7 @@ void ProgressbarInteractiveTest::test_multi_progress_bars_with_messages_with_tot "\\[2/2\\] test2 40% | ????? ??B\\/s | 4.0 B | ???????\n" "----------------------------------------------------------------------\n" "\\[1/2\\] Total 70% | ????? ??B\\/s | 14.0 B | ???????\n" + "\n" "\n"; ASSERT_MATCHES(expected, perform_control_sequences(oss.str())); @@ -411,6 +420,7 @@ void ProgressbarInteractiveTest::test_multi_progress_bars_with_messages() { download_progress_bar2_raw->set_state(libdnf5::cli::progressbar::ProgressBarState::STARTED); download_progress_bar2_raw->add_message(libdnf5::cli::progressbar::MessageType::INFO, "test message1"); download_progress_bar2_raw->add_message(libdnf5::cli::progressbar::MessageType::INFO, "test message2"); + download_progress_bar2_raw->add_message(libdnf5::cli::progressbar::MessageType::INFO, "test こんにちは世界！"); std::ostringstream oss; oss << multi_progress_bar; @@ -418,10 +428,12 @@ void ProgressbarInteractiveTest::test_multi_progress_bars_with_messages() { "\\[1/2\\] test1 100% | ????? ??B\\/s | 10.0 B | ???????\n" "\\[2/2\\] test2 40% | ????? ??B\\/s | 4.0 B | ???????\n" ">>> test message1 \n" - ">>> test message2 "; + ">>> test message2 \n" + ">>> test こんにちは世界！ "; ASSERT_MATCHES(expected, perform_control_sequences(oss.str())); + download_progress_bar2_raw->pop_message(); download_progress_bar2_raw->pop_message(); download_progress_bar2_raw->pop_message(); oss << multi_progress_bar; @@ -429,6 +441,7 @@ void ProgressbarInteractiveTest::test_multi_progress_bars_with_messages() { expected = "\\[1/2\\] test1 100% | ????? ??B\\/s | 10.0 B | ???????\n" "\\[2/2\\] test2 40% | ????? ??B\\/s | 4.0 B | ???????\n" + "\n" "\n"; ASSERT_MATCHES(expected, perform_control_sequences(oss.str())); @@ -441,6 +454,7 @@ void ProgressbarInteractiveTest::test_multi_progress_bars_with_messages() { expected = "\\[1/2\\] test1 100% | ????? ??B\\/s | 10.0 B | ???????\n" "\\[2/2\\] test2 40% | ????? ??B\\/s | 4.0 B | ???????\n" + "\n" "\n"; ASSERT_MATCHES(expected, perform_control_sequences(oss.str()));