worked on thread safety with tasks

app/common/CMakeLists.txt

@@ -26,6 +26,8 @@ set(OLIVE_SOURCES
   common/qobjectlistcast.h
   common/qtversionabstraction.h
   common/qtversionabstraction.cpp
+  common/threadedobject.h
+  common/threadedobject.cpp
   common/timecodefunctions.h
   common/timecodefunctions.cpp
   PARENT_SCOPE

app/common/threadedobject.cpp

@@ -0,0 +1,37 @@
+#include "threadedobject.h"
+
+ThreadedObject::ThreadedObject()
+{
+}
+
+void ThreadedObject::LockDeletes()
+{
+  threadobj_delete_lock_++;
+}
+
+void ThreadedObject::UnlockDeletes()
+{
+  Q_ASSERT(AreDeletesLocked());
+
+  threadobj_delete_lock_--;
+}
+
+bool ThreadedObject::AreDeletesLocked()
+{
+  return (threadobj_delete_lock_ > 0);
+}
+
+void ThreadedObject::LockMutex()
+{
+  threadobj_main_lock_.lock();
+}
+
+void ThreadedObject::UnlockMutex()
+{
+  threadobj_main_lock_.unlock();
+}
+
+bool ThreadedObject::TryLockMutex(int timeout)
+{
+  return threadobj_main_lock_.tryLock(timeout);
+}

app/common/threadedobject.h

@@ -0,0 +1,25 @@
+#ifndef THREADEDOBJECT_H
+#define THREADEDOBJECT_H
+
+#include <QMutex>
+
+class ThreadedObject
+{
+public:
+  ThreadedObject();
+
+  void LockMutex();
+  void UnlockMutex();
+  bool TryLockMutex(int timeout = 0);
+
+  void LockDeletes();
+  void UnlockDeletes();
+  bool AreDeletesLocked();
+
+private:
+  QMutex threadobj_main_lock_;
+
+  QAtomicInt threadobj_delete_lock_;
+};
+
+#endif // THREADEDOBJECT_H

app/project/item/item.cpp

@@ -157,13 +157,3 @@ bool Item::ChildExistsWithNameInternal(const QString &name, Item *folder)
 
   return false;
 }
-
-void Item::Lock()
-{
-  mutex_.lock();
-}
-
-void Item::Unlock()
-{
-  mutex_.unlock();
-}

app/project/item/item.h

@@ -27,6 +27,8 @@
 #include <QMutex>
 #include <QString>
 
+#include "common/threadedobject.h"
+
 class Item;
 using ItemPtr = std::shared_ptr<Item>;
 
@@ -36,7 +38,7 @@ using ItemPtr = std::shared_ptr<Item>;
  * Project objects implement a parent-child hierarchy of Items that can be used throughout the Project. The Item class
  * itself is abstract and will need to be subclassed to be used in a Project.
  */
-class Item
+class Item : public ThreadedObject
 {
 public:
   enum Type {
@@ -100,9 +102,6 @@ public:
 
   bool ChildExistsWithName(const QString& name);
 
-  void Lock();
-  void Unlock();
-
 private:
   bool ChildExistsWithNameInternal(const QString& name, Item* folder);
 
@@ -116,8 +115,6 @@ private:
 
   QString tooltip_;
 
-  QMutex mutex_;
-
 };
 
 #endif // ITEM_H

app/task/import/import.cpp

@@ -41,11 +41,13 @@ ImportTask::ImportTask(ProjectViewModel *model, Folder *parent, const QStringLis
   urls_(urls),
   parent_(parent)
 {
-  set_text(tr("Importing %1 files").arg (urls.size()));
+  set_text(tr("Importing %1 files").arg(urls.size()));
 }
 
 bool ImportTask::Action()
 {
+  parent_->LockDeletes();
+
   QUndoCommand* command = new QUndoCommand();
 
   Import(urls_, parent_, command);
@@ -54,12 +56,11 @@ bool ImportTask::Action()
   // the undo command executes the final import anyway)
   if (cancelled()) {
     delete command;
-
-    return true;
+  } else {
+    olive::undo_stack.push(command);
   }
 
-  // FIXME: This should be run in the main thread somehow
-  olive::undo_stack.push(command);
+  parent_->UnlockDeletes();
 
   return true;
 }

app/task/probe/probe.cpp

@@ -34,11 +34,11 @@ ProbeTask::ProbeTask(FootagePtr footage) :
 
 bool ProbeTask::Action()
 {
-  footage_->Lock();
+  footage_->LockDeletes();
 
   Decoder::ProbeMedia(footage_.get());
 
-  footage_->Unlock();
+  footage_->UnlockDeletes();
 
   return true;
 }

app/task/task.cpp

@@ -58,6 +58,12 @@ bool Task::Start()
 
   cancelled_ = false;
 
+  // Run Prologue() function
+  if (!Prologue()) {
+    set_status(kError);
+    return false;
+  }
+
   set_status(kWorking);
 
   thread_.start();
@@ -65,11 +71,21 @@ bool Task::Start()
   return true;
 }
 
+bool Task::Prologue()
+{
+  return true;
+}
+
 bool Task::Action()
 {
   return true;
 }
 
+bool Task::Epilogue()
+{
+  return true;
+}
+
 const Task::Status &Task::status()
 {
   return status_;
@@ -151,8 +167,16 @@ void Task::set_status(const Task::Status &status)
 void Task::ThreadComplete()
 {
   // thread_.result() will be set to the return value of Action()
+  bool succeeded = thread_.result();
 
-  if (thread_.result()) {
+  // Run the Prologue() function for any final tasks
+  // User cancelling is not considered an error, so we need to check it too
+  if (succeeded && !cancelled()) {
+    succeeded = Epilogue();
+  }
+
+  // If everything succeeded, we set the status accordingly
+  if (succeeded) {
     set_status(kFinished);
   } else {
     set_status(kError);

app/task/task.h

@@ -89,10 +89,26 @@ public:
   bool Start();
 
   /**
-   * @brief The main Task function
+   * @brief Perform opening tasks before main Task thread begins
    *
-   * Action() is the function that gets called once another thread has been created. This function should be overridden
-   * in subclasses.
+   * If a Task needs to perform any actions in the main thread before starting the Task's thread, (e.g. copying or
+   * altering information) this function should be overridden and those actions should be performed here. It's
+   * guaranteed that Prologue() will run in the main thread, and as such, functions here should remain as minimal as
+   * possible as to not block the main thread for a noticeable amount of time. If your Task does not need any such
+   * actions, you don't need to override this.
+   *
+   * @return
+   *
+   * TRUE if the prologue was successful and we can start the Task now. If Prologue returns FALSE, the thread is never
+   * created and Action()/Epilogue() are never run.
+   */
+  virtual bool Prologue();
+
+  /**
+   * @brief The main Task function which is run in a separate thread
+   *
+   * Action() is the function that gets called once the separate thread has been created. This function should be
+   * overridden in subclasses.
    *
    * It's also recommended to emit ProgressChanged() throughout your Action() so that any attached ProgressBars can
    * show accurate progress information.
@@ -105,6 +121,24 @@ public:
    */
   virtual bool Action();
 
+  /**
+   * @brief Perform any closing Tasks in the main thread after the Task thread finishes
+   *
+   * It's likely your Task modifies data used throughout the program in some way, and to prevent race conditions, it's
+   * recommended to work with "copies" of that data in Action() (which is run in separate thread) and never
+   * access/modify any data used in other threads. Then, after the Action() thread is complete, that data can be used to
+   * "apply" that data in the main thread here.
+   *
+   * As this runs in the main thread, these functions shouldn't be kept fairly minimal to prevent blocking the main
+   * thread.
+   *
+   * @return
+   *
+   * TRUE if the Epilogue completed successfully. FALSE if not. A FALSE result here is considered a complete failure
+   * of the Task, even though the bulk of the processing has been performed in Action().
+   */
+  virtual bool Epilogue();
+
   /**
    * @brief Current status of the Task
    *