MultiProgressBar fixes and tests

include/libdnf5-cli/progressbar/multi_progress_bar.hpp

@@ -43,6 +43,9 @@ class LIBDNF_CLI_API MultiProgressBar {
     ~MultiProgressBar();
 
     void add_bar(std::unique_ptr<ProgressBar> && bar);
+
+    // In interactive mode MultiProgressBar doesn't print a newline after unfinished progressbars.
+    // Finished progressbars always end with a newline.
     void print() {
         std::cerr << *this;
         std::cerr << std::flush;
@@ -69,6 +72,7 @@ class LIBDNF_CLI_API MultiProgressBar {
     std::vector<ProgressBar *> bars_todo;
     std::vector<ProgressBar *> bars_done;
     DownloadProgressBar total;
+    // Whether the last line was printed without a new line ending (such as an in progress bar)
     bool line_printed{false};
     std::size_t num_of_lines_to_clear{0};
 };

include/libdnf5-cli/tty.hpp

@@ -46,6 +46,7 @@ LIBDNF_CLI_API std::ostream & white(std::ostream & stream);
 
 LIBDNF_CLI_API std::ostream & clear_line(std::ostream & stream);
 LIBDNF_CLI_API std::ostream & cursor_up(std::ostream & stream);
+LIBDNF_CLI_API std::ostream & cursor_down(std::ostream & stream);
 
 LIBDNF_CLI_API std::ostream & cursor_hide(std::ostream & stream);
 LIBDNF_CLI_API std::ostream & cursor_show(std::ostream & stream);

libdnf5-cli/progressbar/multi_progress_bar.cpp

@@ -106,14 +106,15 @@ std::ostream & operator<<(std::ostream & stream, MultiProgressBar & mbar) {
     text_buffer.str("");
     text_buffer.clear();
 
+    std::size_t last_num_of_lines_to_clear = mbar.num_of_lines_to_clear;
+    std::size_t num_of_lines_permanent = 0;
+
     if (is_interactive && mbar.num_of_lines_to_clear > 0) {
         if (mbar.num_of_lines_to_clear > 1) {
             // Move the cursor up by the number of lines we want to write over
             text_buffer << "\033[" << (mbar.num_of_lines_to_clear - 1) << "A";
         }
         text_buffer << "\r";
-    } else if (mbar.line_printed) {
-        text_buffer << std::endl;
     }
 
     mbar.num_of_lines_to_clear = 0;
@@ -135,6 +136,8 @@ std::ostream & operator<<(std::ostream & stream, MultiProgressBar & mbar) {
         numbers.pop_back();
         text_buffer << *bar;
         text_buffer << std::endl;
+        num_of_lines_permanent++;
+        num_of_lines_permanent += bar->get_messages().size();
         mbar.bars_done.push_back(bar);
         // TODO(dmach): use iterator
         mbar.bars_todo.erase(mbar.bars_todo.begin() + static_cast<int>(i));
@@ -206,8 +209,32 @@ std::ostream & operator<<(std::ostream & stream, MultiProgressBar & mbar) {
         }
 
         text_buffer << mbar.total;
-        text_buffer << std::endl;
-        mbar.num_of_lines_to_clear += 3;
+        mbar.num_of_lines_to_clear += 2;
+        if (mbar.total.is_finished()) {
+            text_buffer << std::endl;
+        } else {
+            mbar.line_printed = true;
+        }
+    }
+
+    // If we have written less lines than last time we need to clear the rest otherwise
+    // there would be garbage under the updated progressbar. This is because normally
+    // we don't actually clear the lines we just write over the old output to ensure smooth
+    // output updating.
+    // TODO(amatej): It would be sufficient to do this only once after all progressbars have
+    // finished but unfortunaly MultiProgressBar doesn't have a pImpl so we cannot
+    // store the highest line count it had. We could fix this when breaking ABI.
+    size_t all_written = num_of_lines_permanent + mbar.num_of_lines_to_clear;
+    if (last_num_of_lines_to_clear > all_written) {
+        auto delta = last_num_of_lines_to_clear - all_written;
+        if (!mbar.line_printed) {
+            text_buffer << tty::clear_line;
+        }
+        for (std::size_t i = 0; i < delta; i++) {
+            text_buffer << tty::cursor_down << tty::clear_line;
+        }
+        // Move cursor back up after clearing lines leftover from previous print
+        text_buffer << "\033[" << delta << "A";
     }
 
     stream << text_buffer.str();  // Single syscall to output all commands

libdnf5-cli/tty.cpp

@@ -52,6 +52,18 @@ int get_width() {
 
 
 bool is_interactive() {
+    // Use a custom "DNF5_FORCE_INTERACTIVE" variable for testing purposes.
+    // "interactivity" depends on stdout configuration which is hard to control sometimes
+    char * force_interactive = std::getenv("DNF5_FORCE_INTERACTIVE");
+    if (force_interactive != nullptr) {
+        try {
+            // Convert to an int which is then converted to bool,
+            // so when defined accept 0 as FALSE and non 0 as TRUE
+            return std::stoi(force_interactive);
+        } catch (std::invalid_argument & ex) {
+        } catch (std::out_of_range & ex) {
+        }
+    }
     return isatty(fileno(stdout)) == 1;
 }
 
@@ -107,6 +119,9 @@ TTY_COMMAND(clear_line, "\033[2K")
 // tty::cursor_up
 TTY_COMMAND(cursor_up, "\x1b[A")
 
+// tty::cursor_down
+TTY_COMMAND(cursor_down, "\x1b[B")
+
 // tty::cursor_hide
 TTY_COMMAND(cursor_hide, "\x1b[?25l")
 

test/libdnf5-cli/test_progressbar.cpp

@@ -21,9 +21,8 @@ along with libdnf.  If not, see <https://www.gnu.org/licenses/>.
 #include "test_progressbar.hpp"
 
 #include "../shared/private_accessor.hpp"
+#include "../shared/utils.hpp"
 
-#include <fmt/format.h>
-#include <fnmatch.h>
 #include <libdnf5-cli/progressbar/download_progress_bar.hpp>
 #include <libdnf5-cli/progressbar/multi_progress_bar.hpp>
 
@@ -38,23 +37,42 @@ create_getter(to_stream, &libdnf5::cli::progressbar::DownloadProgressBar::to_str
 
 }  //namespace
 
+void ProgressbarTest::setUp() {
+    // MultiProgressBar behaves differently depending on interactivity
+    setenv("DNF5_FORCE_INTERACTIVE", "0", 1);
+    // Force columns to 70 to make output independ of where it is run
+    setenv("FORCE_COLUMNS", "70", 1);
+}
+
+void ProgressbarTest::tearDown() {
+    unsetenv("DNF5_FORCE_INTERACTIVE");
+    unsetenv("FORCE_COLUMNS");
+}
+
 void ProgressbarTest::test_download_progress_bar() {
+    // In non interactive mode download progress bar is printed only when finished.
+
     auto download_progress_bar = std::make_unique<libdnf5::cli::progressbar::DownloadProgressBar>(10, "test");
-    download_progress_bar->set_ticks(10);
-    download_progress_bar->set_state(libdnf5::cli::progressbar::ProgressBarState::SUCCESS);
+    download_progress_bar->set_ticks(4);
+    download_progress_bar->set_state(libdnf5::cli::progressbar::ProgressBarState::STARTED);
+    auto download_progress_bar_raw = download_progress_bar.get();
 
     std::ostringstream oss;
     (*download_progress_bar.*get(to_stream{}))(oss);
-    // When running the tests binary directly (run_tests_cli) it uses terminal size to determine white space count.
-    // To ensure the tests works match any number of white space.
-    std::string expected = "\\[0/0\\] test      [ ]*      100% |   0.0   B\\/s |  10.0   B |  ?     ";
-    CPPUNIT_ASSERT_EQUAL_MESSAGE(
-        fmt::format("Expression: \"{}\" doesn't match output: \"{}\"", expected, oss.str()),
-        fnmatch(expected.c_str(), oss.str().c_str(), FNM_EXTMATCH),
-        0);
+    Pattern expected = "";
+    ASSERT_MATCHES(expected, oss.str());
+
+    download_progress_bar_raw->set_ticks(10);
+    download_progress_bar_raw->set_state(libdnf5::cli::progressbar::ProgressBarState::SUCCESS);
+    (*download_progress_bar.*get(to_stream{}))(oss);
+
+    expected = "\\[0/0\\] test                    100% | ????? ??B\\/s |  10.0   B | ???????";
+    ASSERT_MATCHES(expected, oss.str());
 }
 
 void ProgressbarTest::test_multi_progress_bar() {
+    // In non interactive mode finished multiline progressbar ends with a new line.
+
     auto download_progress_bar1 = std::make_unique<libdnf5::cli::progressbar::DownloadProgressBar>(10, "test1");
     download_progress_bar1->set_ticks(10);
     download_progress_bar1->set_state(libdnf5::cli::progressbar::ProgressBarState::SUCCESS);
@@ -68,18 +86,54 @@ void ProgressbarTest::test_multi_progress_bar() {
     multi_progress_bar.add_bar(std::move(download_progress_bar2));
     std::ostringstream oss;
     oss << multi_progress_bar;
-    auto output = oss.str();
-
-    // When running the tests binary directly (run_tests_cli) it uses terminal size to determine white space and dash count.
-    // To ensure the tests works match any number of white space and dashes.
-    std::string expected =
-        "\\[1/2\\] test1     [ ]*      100% |   0.0   B\\/s |  10.0   B |  ?     \n"
-        "\\[2/2\\] test2     [ ]*      100% |   0.0   B\\/s |  10.0   B |  ?     \n"
-        "--------------------[-]*------------------------------------------------\n"
-        "\\[2/2\\] Total     [ ]*      100% | ????? ??B\\/s |  20.0   B |  ??m??s\n";
-
-    CPPUNIT_ASSERT_EQUAL_MESSAGE(
-        fmt::format("Expression: \"{}\" doesn't match output: \"{}\"", expected, oss.str()),
-        fnmatch(expected.c_str(), oss.str().c_str(), FNM_EXTMATCH),
-        0);
+
+    Pattern expected =
+        "\\[1/2\\] test1                   100% | ????? ??B\\/s |  10.0   B | ???????\n"
+        "\\[2/2\\] test2                   100% | ????? ??B\\/s |  10.0   B | ???????\n"
+        "----------------------------------------------------------------------\n"
+        "\\[2/2\\] Total                   100% | ????? ??B\\/s |  20.0   B | ???????\n";
+    ASSERT_MATCHES(expected, oss.str());
+}
+
+void ProgressbarTest::test_multi_progress_bar_unfinished() {
+    // In non-interacitve mode:
+    // - unfinished progressbars are not printed
+    // - total is not printed until all progressbars are finished
+
+    auto download_progress_bar1 = std::make_unique<libdnf5::cli::progressbar::DownloadProgressBar>(10, "test1");
+    download_progress_bar1->set_ticks(10);
+    download_progress_bar1->set_state(libdnf5::cli::progressbar::ProgressBarState::SUCCESS);
+
+    auto download_progress_bar2 = std::make_unique<libdnf5::cli::progressbar::DownloadProgressBar>(10, "test2");
+    download_progress_bar2->set_ticks(4);
+    download_progress_bar2->set_state(libdnf5::cli::progressbar::ProgressBarState::STARTED);
+    auto download_progress_bar2_raw = download_progress_bar2.get();
+
+    libdnf5::cli::progressbar::MultiProgressBar multi_progress_bar;
+    multi_progress_bar.add_bar(std::move(download_progress_bar1));
+    multi_progress_bar.add_bar(std::move(download_progress_bar2));
+    std::ostringstream oss;
+
+    oss << multi_progress_bar;
+    Pattern expected = "\\[1/2\\] test1                   100% | ????? ??B\\/s |  10.0   B | ???????\n";
+    ASSERT_MATCHES(expected, oss.str());
+
+    // More iterations
+    download_progress_bar2_raw->set_ticks(5);
+    oss << multi_progress_bar;
+    download_progress_bar2_raw->set_ticks(6);
+    oss << multi_progress_bar;
+    expected = "\\[1/2\\] test1                   100% | ????? ??B\\/s |  10.0   B | ???????\n";
+    ASSERT_MATCHES(expected, oss.str());
+
+    // Next iteration
+    download_progress_bar2_raw->set_ticks(10);
+    download_progress_bar2_raw->set_state(libdnf5::cli::progressbar::ProgressBarState::SUCCESS);
+    oss << multi_progress_bar;
+    expected =
+        "\\[1/2\\] test1                   100% | ????? ??B\\/s |  10.0   B | ???????\n"
+        "\\[2/2\\] test2                   100% | ????? ??B\\/s |  10.0   B | ???????\n"
+        "----------------------------------------------------------------------\n"
+        "\\[2/2\\] Total                   100% | ????? ??B\\/s |  20.0   B | ???????\n";
+    ASSERT_MATCHES(expected, oss.str());
 }

test/libdnf5-cli/test_progressbar.hpp

@@ -29,12 +29,17 @@ class ProgressbarTest : public CppUnit::TestCase {
 
     CPPUNIT_TEST(test_download_progress_bar);
     CPPUNIT_TEST(test_multi_progress_bar);
+    CPPUNIT_TEST(test_multi_progress_bar_unfinished);
 
     CPPUNIT_TEST_SUITE_END();
 
 public:
+    void setUp() override;
+    void tearDown() override;
+
     void test_download_progress_bar();
     void test_multi_progress_bar();
+    void test_multi_progress_bar_unfinished();
 };