[vlc-devel] [PATCH] qt: medialib: add async task handle

[Pierre Lamot]

Hi, OK for the patch,

some nits bellow.

On 2021-01-06 15:55, Romain Vimont wrote:
> Use a smart pointer to "abandon" an AsyncTask using RAII. This avoids 
> to
> forget to call abandon() on client destruction.
> ---
>  modules/gui/qt/util/asynctask.hpp | 22 +++++++++++++++++++
>  modules/gui/qt/util/listcache.hpp | 36 +++++++++----------------------
>  2 files changed, 32 insertions(+), 26 deletions(-)
> 
> diff --git a/modules/gui/qt/util/asynctask.hpp
> b/modules/gui/qt/util/asynctask.hpp
> index fa4a6d3b2c..81fcc54022 100644
> --- a/modules/gui/qt/util/asynctask.hpp
> +++ b/modules/gui/qt/util/asynctask.hpp
> @@ -128,6 +128,9 @@
>   * to declare that it won't use it anymore (and don't want the 
> result). When
>   * both conditions are met, the task is deleted via 
> `QObject::deleteLater()`.
>   *
> + * For convenience, a `TaskHandle` smart pointer is provided to call
> + * `abandon()` using RAII.
> + *
>   * The task result is provided via a separate method `takeResult()` 
> instead of
>   * a slot parameter, because otherwise, Qt would require its type to 
> be:
>   *  - non-template (like `std::vector<T>`),
> @@ -243,4 +246,23 @@ private:
>      AsyncTask<T> *m_task;
>  };
> 
> +template <typename T>
> +struct TaskDeleter
> +{
> +    void operator()(T *task) { task->abandon(); }
> +};
> +
> +/**
> + * Task handle to abandon() a task using RAII
> + *
> + * It behaves like a unique_ptr, but its deleter calls 
> AsyncTask<T>::abandon(),
> + * which indicates that the client cancels the task and requests 
> deletion when
> + * possible.
> + *
> + * The actual deletion may happen later (when the runnable also 
> finished its
> + * work).
> + */
> +template <typename T>
> +using TaskHandle = std::unique_ptr<T, TaskDeleter<T>>;
> +
>  #endif
> diff --git a/modules/gui/qt/util/listcache.hpp
> b/modules/gui/qt/util/listcache.hpp
> index e92b7d84df..20fa510832 100644
> --- a/modules/gui/qt/util/listcache.hpp
> +++ b/modules/gui/qt/util/listcache.hpp
> @@ -115,7 +115,6 @@ public:
>          : m_threadPool(threadPool)
>          , m_loader(loader)
>          , m_chunkSize(chunkSize) {}
> -    ~ListCache();
> 
>      /**
>       * Return the item at specified index
> @@ -167,19 +166,10 @@ private:
>      bool m_countRequested = false;
>      MLRange m_lastRangeRequested;
> 
> -    LoadTask<T> *m_loadTask = nullptr;
> -    CountTask<T> *m_countTask = nullptr;
> +    TaskHandle<LoadTask<T>> m_loadTask;
> +    TaskHandle<CountTask<T>> m_countTask;
>  };
> 
> -template <typename T>
> -ListCache<T>::~ListCache()
> -{
> -    if (m_countTask)
> -        m_countTask->abandon();
> -    if (m_loadTask)
> -        m_loadTask->abandon();
> -}
> -
>  template <typename T>
>  const T *ListCache<T>::get(size_t index) const
>  {
> @@ -248,8 +238,8 @@ void ListCache<T>::asyncCount()
>  {
>      assert(!m_countTask);
> 
> -    m_countTask = new CountTask<T>(m_loader);
> -    connect(m_countTask, &BaseAsyncTask::result,
> +    m_countTask.reset(new CountTask<T>(m_loader));

maybe use std::make_unique

> +    connect(&*m_countTask, &BaseAsyncTask::result,
>              this, &ListCache<T>::onCountResult);

m_countTask.get() directly gives you the underlying address.

>      m_countRequested = true;
>      m_countTask->start(m_threadPool);
> @@ -259,15 +249,14 @@ template <typename T>
>  void ListCache<T>::onCountResult()
>  {
>      CountTask<T> *task = static_cast<CountTask<T> *>(sender());
> -    assert(task == m_countTask);
> +    assert(task == &*m_countTask);
> 
>      m_offset = 0;
>      m_list.clear();
>      m_total_count = static_cast<ssize_t>(task->takeResult());
>      emit localSizeChanged(m_total_count);
> 
> -    task->abandon();
> -    m_countTask = nullptr;
> +    m_countTask.reset();
>  }
> 
>  template <typename T>
> @@ -298,12 +287,8 @@ private:
>  template <typename T>
>  void ListCache<T>::asyncLoad(size_t offset, size_t count)
>  {
> -    if (m_loadTask)
> -        /* Cancel any current pending task */
> -        m_loadTask->abandon();
> -
> -    m_loadTask = new LoadTask<T>(m_loader, offset, count);
> -    connect(m_loadTask, &BaseAsyncTask::result,
> +    m_loadTask.reset(new LoadTask<T>(m_loader, offset, count));
> +    connect(&*m_loadTask, &BaseAsyncTask::result,
>              this, &ListCache<T>::onLoadResult);
>      m_lastRangeRequested = { offset, count };
>      m_loadTask->start(m_threadPool);
> @@ -313,15 +298,14 @@ template <typename T>
>  void ListCache<T>::onLoadResult()
>  {
>      LoadTask<T> *task = static_cast<LoadTask<T> *>(sender());
> -    assert(task == m_loadTask);
> +    assert(task == &*m_loadTask);
> 
>      m_offset = task->m_offset;
>      m_list = task->takeResult();
>      if (m_list.size())
>          emit localDataChanged(m_offset, m_list.size());
> 
> -    task->abandon();
> -    m_loadTask = nullptr;
> +    m_loadTask.reset();
>  }
> 
>  #endif