Implement progress indicator

[Baryonics]

Adding p-ranav/indicators library to this project, to indicate the reader status. (and any other range object)

[codecov]

Codecov Report

❌ Patch coverage is 90.12346% with 8 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
source/centipede/util/progress_indicator.hpp	87.69%	8 Missing ⚠️

📢 Thoughts on this report? Let us know!

[Baryonics]

The indicator feature is functional! I'll need to do some error handling, not quite sure how to do that to be honest... Probably some static assert, since it's almost all template stuff. plus I'm unsure of how to do the testing part. I added the indicator in the reader integration test and it's all covered, but I think it might be necessary to implement unit tests as well.
Whats your opinion @YanzhaoW ?

[YanzhaoW]

Thanks for the PR. I will check the code in detail later.

The indicator feature is functional! I'll need to do some error handling, not quite sure how to do that to be honest... Probably some static assert, since it's almost all template stuff.

Yeah, static checking should always be preferred.

plus I'm unsure of how to do the testing part. I added the indicator in the reader integration test and it's all covered, but I think it might be necessary to implement unit tests as well.

I would say an integration test should suffice.

[YanzhaoW]

It looks good. I made some suggestions.

[YanzhaoW]

I feel this API is a bit too open. These options come from Indicator library, which is an internal implementation. Normally it's not a good idea to expose the internal implementation to the API because if Internal implementation is changed, the public interface also needs to be changed, which is undesirable.

Second, many configs aren't necessary to let users to set them and probably better to hard-code it in the class. For example, these configs can be hard-coded:

        centipede::progress::config::Start{ "[" },
        centipede::progress::config::Fill{ "=" },
        centipede::progress::config::Lead{ ">" },
        centipede::progress::config::Remainder{ " " },
        centipede::progress::config::End{ "]" },
        centipede::progress::config::ForegroundColor{ centipede::progress::ProgressColor::green },
        centipede::progress::config::FontStyles{
            std::vector<centipede::progress::ProgressFontStyle>{ centipede::progress::ProgressFontStyle::bold } }

It's also better to have a closed Config class with some fixed options users can tweak. Something like:

struct Config{
   bool enable_percentage = true;
   std::size_t bar_width = DEFAULT_INDICATOR_BAR_WIDTH;
   std::size_t file_size = 0;
   std::string label_text;
   /* maybe something else? */
};

Then when user needs to use this adaptor, they can just check the config struct and tweak some values over there, instead of having to go to another library to check what options they have.

Third, would it be possible to make it one line? Something like:

for ([[maybe_unused]] const auto& entry :
         reader | 
         ProgressAdaptor{
                {.file_size = reader.get_file_size(), 
                 .label_text = "Reading binary data...", 
                 . bar_width = 50}, 
                [&reader]() { return reader.get_last_entry_bytes(); }
         }
       )

[Baryonics]

The problem was the variadic template constructor of indicators::ProgressBar. It required to move every member of a config struct, which resulted in a huge struct, and a huge constructor with 20 move calls.
But you are right, it's probably better to expose a minimal config struct to the user. In that case, everything should be a bit more clear.

This

for ([[maybe_unused]] const auto& entry :
         reader | 
         ProgressAdaptor{
                {.file_size = reader.get_file_size(), 
                 .label_text = "Reading binary data...", 
                 . bar_width = 50}, 
                [&reader]() { return reader.get_last_entry_bytes(); }
         }
       )

is indeed possible! That's why I introduced the shared_ptr. The problem was, that ProgressClosure owned state_ and bar_, while ProgressView stored raw pointers to those objects.
this expression

for (auto elem : range | ProgressAdaptor{/** any constructor **/})
{
}

results in a temporary r-value ProgressAdaptor{}, which, until C++ 20 would not survive the initial call in the for loop head. This would result in dangling pointers inside ProgressView.
But luckily, since C++ 23, those ClosureObjects survive until the loop ends, so thats good.
Unfortunately, this expression auto progress_view = range | ProgressView{/** any constructor **/}; would still result in a dangling pointer. So I wanted to create a way, that guarantees the survival of a ProgressAdaptor object, that is connected to a ProgressView, so I introduced the shared_ptr for that task.

TL;DR: ProgressView should have been the owner of bar_ and status_ in the first place while ProgressAdaptor only stores the pointers... Well, I think outsmarted myself there a bit. But hey, at least I learned a lot.

[YanzhaoW]

I see. Ok, I was confused by the name "Adaptor". 😆

ProgressView should have been the owner of bar_ and status_ in the first place while ProgressAdaptor only stores the pointers... Well, I think outsmarted myself there a bit. But hey, at least I learned a lot.

I'm not sure this is a good idea because conventionally a view or an adaptor should just be a thin layer, cheap to allocate and cheap to delete and shouldn't own anything by itself.

Maybe you can still use your original design, but change the name "Adaptor" to something like "ProgressBar" or "ProgressIndicator", which owns everything and create an adaptor or a view:

 for ([[maybe_unused]] const auto& entry : reader | progress_indicator.adaptor())

[YanzhaoW]

Is it really necessary to use std::function instead of just lambda?

std::function has a noticeable performance overhead and calling it is not cheap compared to lambda or plain function pointer.

[Baryonics]

I wasn't sure because I know that std::function requires allocations, but also didn't want to use a raw function pointer.
Is it even possible to store a lambda as a non static member? I think I'll switch to function pointer instead, this would require checks for nullptr but the overhead is minimal compared to either std::function or weird lamda structures.

[YanzhaoW]

Is it even possible to store a lambda as a non static member?

Yes, absolutely. This is also a very common practice, called "Dependency injection (DI)", where you accept a callback or a lambda as an input argument and invoke it inside the class:

template <typename UnitaryFn>
class MyClass{
  public:
    MyClass(UnitaryFn unitary_fn) : unitary_fn_ {unitary_fn} {}

    void call(int val) {unitary_fn_(val);}
  private:
    UnitaryFn unitary_fn_;
};

You can also restrict the type UnitaryFn with some concepts for nice compile error messages.

Lambda is just callable object with defined operator()() overloading and surely you can store an object as a non static member. The drawback is that you have to make your class a class template. In case you can't make the class as a template, std::function can be an alternative with some perf overhead.

Plain function pointer is ugly to deal with and has a limitation that you can't convert a captured lambda to a function pointer. Non-captured lambda can be converted to function pointer as it has no internal state (captured stuffs).

So I would still suggest to store a Lambda if your class is allowed to become a template.

[YanzhaoW]

Suggested change

	std::size_t total_size_n;
	std::size_t total_size_n {};

[YanzhaoW]

I'm not sure std::shared_ptr here is a good idea.

You don't need to use any std::shared_ptr if your code is running in a single thread. std::unique_ptr should be enough. So question would be "who owns these?". The owner owns it by std::unique_ptr and any other classes that use these objects should store their raw pointer. Also in this case, owner should outlive those user classes.

Mostly, shared ptr is used in a multi-threading scenario, where ownership is not clear and you are not sure which thread first finishes and which thread last finishes and, thus, should clean up the memory.

[YanzhaoW]

Better to chain the overloading from one to another:

Suggested change

	auto operator()(std::size_t total_size_n)
	{
	return ProgressClosure{ {}, total_size_n, []() -> std::size_t { return 1UZ; }, bar_ptr_, status_ptr_ };
	}
	auto operator()(std::size_t total_size_n)
	{
	return (*this)(total_size_n, []() -> std::size_t { return 1UZ; });
	}

So if you change one overloading, the change will propagate to all overloading (Similar to multi-constructor overloading).

[YanzhaoW]

Suggested change

	requires std::ranges::range<RangeT>
	requires std::ranges::forward_range<RangeT>

Maybe narrow the concept to be an forward_range?

[YanzhaoW]

Name is a bit redundant. So total identifier is srs::progress::ProgressAdaptor. So maybe either remove the progress namespace or change the class name to Adaptor, such that the final identifier is either srs::ProgressAdaptor or srs::progress::Adaptor.

[YanzhaoW]

Same problem with name. The full identifier is srs::ProgressAdaptor::ProgressView. srs::progress::View would be better.

[YanzhaoW]

I see. Ok, I was confused by the name "Adaptor". 😆

ProgressView should have been the owner of bar_ and status_ in the first place while ProgressAdaptor only stores the pointers... Well, I think outsmarted myself there a bit. But hey, at least I learned a lot.

I'm not sure this is a good idea because conventionally a view or an adaptor should just be a thin layer, cheap to allocate and cheap to delete and shouldn't own anything by itself.

Maybe you can still use your original design, but change the name "Adaptor" to something like "ProgressBar" or "ProgressIndicator", which owns everything and create an adaptor or a view:

 for ([[maybe_unused]] const auto& entry : reader | progress_indicator.adaptor())

[YanzhaoW]

The error from the Coverage CI should be fixed by the latest master branch.

[Baryonics]

In this new implementation, ProgressIndicator owns status_ and bar_. Because of that, we have to make sure that the ProgressIndicator lives longer than the ProgressAdaptor or ProgressView which uses it.

If we want to allow something like

for (auto elem : range | ProgressIndicator{}.get_adaptor())
{
}

we would have a problem, because the adaptor/view could store a pointer to a temporary ProgressIndicator.

One solution would be to store bar_ and status_ inside some shared state, for example with a shared_ptr. But I think this would make the implementation more complicated than necessary.

Because of that, I would prefer to forbid getting an adaptor from a temporary ProgressIndicator.

This can be done with ref-qualified getter functions. The getter only works for non-temporary objects, while the overload for temporary objects is deleted.

The API would then look like this:

auto progress = ProgressIndicator{};

for (auto elem : range | progress.get_adaptor(/* any supported arguments */))
{
}

This way we can keep the ownership simple and also make sure that the ProgressIndicator is still alive while the adaptor/view is using it.