Fix crashes using certain function w/Undo History open

Some commands were calling transaction.commit() in the non-main/UI
thread (e.g. SpriteSizeCommand). That commit() called
DocumentUndo::add() which generated a
DocumentUndoObserver::onAddUndoState() and it updated the Undo History
window UI. This generated a lot of racing conditions in the UI module
and possibilities of crashes if the Undo History window was visible.

src/app/CMakeLists.txt

@@ -404,7 +404,6 @@ add_library(app-lib
   pref/preferences.cpp
   project.cpp
   recent_files.cpp
-  render_task_job.cpp
   res/http_loader.cpp
   res/palettes_loader_delegate.cpp
   res/resources_loader.cpp
@@ -414,7 +413,9 @@ add_library(app-lib
   send_crash.cpp
   shade.cpp
   snap_to_grid.cpp
+  sprite_job.cpp
   thumbnail_generator.cpp
+  thumbnails.cpp
   tools/active_tool.cpp
   tools/ink_type.cpp
   tools/intertwine.cpp
@@ -508,7 +509,6 @@ add_library(app-lib
   ui/workspace_tabs.cpp
   ui/zoom_entry.cpp
   ui_context.cpp
-  thumbnails.cpp
   util/autocrop.cpp
   util/clipboard.cpp
   util/clipboard_native.cpp

src/app/commands/cmd_change_pixel_format.cpp

@@ -19,7 +19,7 @@
 #include "app/modules/editors.h"
 #include "app/modules/gui.h"
 #include "app/modules/palettes.h"
-#include "app/render_task_job.h"
+#include "app/sprite_job.h"
 #include "app/transaction.h"
 #include "app/ui/dithering_selector.h"
 #include "app/ui/editor/editor.h"
@@ -462,23 +462,21 @@ void ChangePixelFormatCommand::onExecute(Context* context)
     return;
 
   {
-    RenderTaskJob job("Converting Color Mode");
+    const ContextReader reader(context);
-    job.startJob(
+    SpriteJob job(reader, "Color Mode Change");
-      [this, &job, context, flatten]{
+    job.startJobWithCallback(
-        ContextWriter writer(context);
+      [this, &job, flatten] {
-        Transaction transaction(writer.context(), "Color Mode Change");
+        Sprite* sprite(job.sprite());
-        Sprite* sprite(writer.sprite());
 
         if (flatten)
-          transaction.execute(new cmd::FlattenLayers(sprite));
+          job.transaction().execute(new cmd::FlattenLayers(sprite));
 
-        transaction.execute(
+        job.transaction().execute(
           new cmd::SetPixelFormat(
             sprite, m_format,
             m_ditheringAlgorithm,
-            m_ditheringMatrix, &job));
+            m_ditheringMatrix,
-        if (!job.isCanceled())
+            &job));             // SpriteJob is a render::TaskDelegate
-          transaction.commit();
       });
     job.waitJob();
   }

src/app/commands/cmd_color_quantization.cpp

@@ -17,7 +17,7 @@
 #include "app/job.h"
 #include "app/modules/palettes.h"
 #include "app/pref/preferences.h"
-#include "app/render_task_job.h"
+#include "app/sprite_job.h"
 #include "app/transaction.h"
 #include "app/ui/color_bar.h"
 #include "app/ui_context.h"
@@ -114,12 +114,14 @@ void ColorQuantizationCommand::onExecute(Context* context)
 
     Palette tmpPalette(frame, entries.picks());
 
-    RenderTaskJob job("Creating Palette");
+    ContextReader reader(context);
-    job.startJob(
+    SpriteJob job(reader, "Color Quantization");
+    job.startJobWithCallback(
       [sprite, withAlpha, &tmpPalette, &job]{
         render::create_palette_from_sprite(
           sprite, 0, sprite->lastFrame(),
-          withAlpha, &tmpPalette, &job);
+          withAlpha, &tmpPalette,
+          &job);              // SpriteJob is a render::TaskDelegate
       });
     job.waitJob();
     if (job.isCanceled())
@@ -141,17 +143,13 @@ void ColorQuantizationCommand::onExecute(Context* context)
       ++i;
     }
 
-    if (*curPalette != *newPalette) {
+    if (*curPalette != *newPalette)
-      ContextWriter writer(UIContext::instance(), 500);
+      job.transaction().execute(new cmd::SetPalette(sprite, frame, newPalette.get()));
-      Transaction transaction(writer.context(), "Color Quantization", ModifyDocument);
-      transaction.execute(new cmd::SetPalette(sprite, frame, newPalette.get()));
-      transaction.commit();
 
-      set_current_palette(newPalette.get(), false);
+    set_current_palette(newPalette.get(), false);
-      ui::Manager::getDefault()->invalidate();
+    ui::Manager::getDefault()->invalidate();
-    }
   }
-  catch (base::Exception& e) {
+  catch (const base::Exception& e) {
     Console::showException(e);
   }
 }

src/app/commands/cmd_rotate.cpp

@@ -15,9 +15,9 @@
 #include "app/context_access.h"
 #include "app/document_api.h"
 #include "app/document_range.h"
-#include "app/job.h"
 #include "app/modules/editors.h"
 #include "app/modules/gui.h"
+#include "app/sprite_job.h"
 #include "app/tools/tool_box.h"
 #include "app/transaction.h"
 #include "app/ui/color_bar.h"
@@ -35,10 +35,7 @@
 
 namespace app {
 
-class RotateJob : public Job {
+class RotateJob : public SpriteJob {
-  ContextWriter m_writer;
-  Document* m_document;
-  Sprite* m_sprite;
   int m_angle;
   CelList m_cels;
   bool m_rotateSprite;
@@ -46,13 +43,9 @@ class RotateJob : public Job {
 public:
 
   RotateJob(const ContextReader& reader, int angle, const CelList& cels, bool rotateSprite)
-    : Job("Rotate Canvas")
+    : SpriteJob(reader, "Rotate Canvas")
-    , m_writer(reader)
-    , m_document(m_writer.document())
-    , m_sprite(m_writer.sprite())
     , m_cels(cels)
-    , m_rotateSprite(rotateSprite)
+    , m_rotateSprite(rotateSprite) {
-  {
     m_angle = angle;
   }
 
@@ -63,18 +56,18 @@ protected:
     const gfx::RectT<T> bounds = newBounds;
     switch (m_angle) {
       case 180:
-        newBounds.x = m_sprite->width() - bounds.x - bounds.w;
+        newBounds.x = sprite()->width() - bounds.x - bounds.w;
-        newBounds.y = m_sprite->height() - bounds.y - bounds.h;
+        newBounds.y = sprite()->height() - bounds.y - bounds.h;
         break;
       case 90:
-        newBounds.x = m_sprite->height() - bounds.y - bounds.h;
+        newBounds.x = sprite()->height() - bounds.y - bounds.h;
         newBounds.y = bounds.x;
         newBounds.w = bounds.h;
         newBounds.h = bounds.w;
         break;
       case -90:
         newBounds.x = bounds.y;
-        newBounds.y = m_sprite->width() - bounds.x - bounds.w;
+        newBounds.y = sprite()->width() - bounds.x - bounds.w;
         newBounds.w = bounds.h;
         newBounds.h = bounds.w;
         break;
@@ -82,10 +75,8 @@ protected:
   }
 
   // [working thread]
-  virtual void onJob()
+  void onJob() override {
-  {
+    DocumentApi api = document()->getApi(transaction());
-    Transaction transaction(m_writer.context(), "Rotate Canvas");
-    DocumentApi api = m_document->getApi(transaction);
 
     // 1) Rotate cel positions
     for (Cel* cel : m_cels) {
@@ -97,13 +88,13 @@ protected:
         gfx::RectF bounds = cel->boundsF();
         rotate_rect(bounds);
         if (cel->boundsF() != bounds)
-          transaction.execute(new cmd::SetCelBoundsF(cel, bounds));
+          transaction().execute(new cmd::SetCelBoundsF(cel, bounds));
       }
       else {
         gfx::Rect bounds = cel->bounds();
         rotate_rect(bounds);
         if (bounds.origin() != cel->bounds().origin())
-          api.setCelPosition(m_sprite, cel, bounds.x, bounds.y);
+          api.setCelPosition(sprite(), cel, bounds.x, bounds.y);
       }
     }
 
@@ -118,7 +109,7 @@ protected:
         new_image->setMaskColor(image->maskColor());
 
         doc::rotate_image(image, new_image.get(), m_angle);
-        api.replaceImage(m_sprite, cel->imageRef(), new_image);
+        api.replaceImage(sprite(), cel->imageRef(), new_image);
       }
 
       jobProgress((float)i / m_cels.size());
@@ -130,24 +121,24 @@ protected:
     }
 
     // rotate mask
-    if (m_document->isMaskVisible()) {
+    if (document()->isMaskVisible()) {
-      Mask* origMask = m_document->mask();
+      Mask* origMask = document()->mask();
       base::UniquePtr<Mask> new_mask(new Mask());
       const gfx::Rect& origBounds = origMask->bounds();
       int x = 0, y = 0;
 
       switch (m_angle) {
         case 180:
-          x = m_sprite->width() - origBounds.x - origBounds.w;
+          x = sprite()->width() - origBounds.x - origBounds.w;
-          y = m_sprite->height() - origBounds.y - origBounds.h;
+          y = sprite()->height() - origBounds.y - origBounds.h;
           break;
         case 90:
-          x = m_sprite->height() - origBounds.y - origBounds.h;
+          x = sprite()->height() - origBounds.y - origBounds.h;
           y = origBounds.x;
           break;
         case -90:
           x = origBounds.y;
-          y = m_sprite->width() - origBounds.x - origBounds.w;
+          y = sprite()->width() - origBounds.x - origBounds.w;
           break;
       }
 
@@ -162,16 +153,13 @@ protected:
       api.copyToCurrentMask(new_mask);
 
       // Regenerate mask
-      m_document->resetTransformation();
+      document()->resetTransformation();
-      m_document->generateMaskBoundaries();
+      document()->generateMaskBoundaries();
     }
 
     // change the sprite's size
     if (m_rotateSprite && m_angle != 180)
-      api.setSpriteSize(m_sprite, m_sprite->height(), m_sprite->width());
+      api.setSpriteSize(sprite(), sprite()->height(), sprite()->width());
-
-    // commit changes
-    transaction.commit();
   }
 
 };

src/app/commands/cmd_sprite_size.cpp

@@ -13,14 +13,11 @@
 #include "app/commands/cmd_sprite_size.h"
 #include "app/commands/command.h"
 #include "app/commands/params.h"
-#include "app/context.h"
-#include "app/context_access.h"
 #include "app/document_api.h"
 #include "app/ini_file.h"
-#include "app/job.h"
 #include "app/modules/gui.h"
 #include "app/modules/palettes.h"
-#include "app/transaction.h"
+#include "app/sprite_job.h"
 #include "base/bind.h"
 #include "base/unique_ptr.h"
 #include "doc/algorithm/resize_image.h"
@@ -43,19 +40,16 @@ namespace app {
 using namespace ui;
 using doc::algorithm::ResizeMethod;
 
-class SpriteSizeJob : public Job {
+class SpriteSizeJob : public SpriteJob {
-  ContextWriter m_writer;
-  Document* m_document;
-  Sprite* m_sprite;
   int m_new_width;
   int m_new_height;
   ResizeMethod m_resize_method;
 
   template<typename T>
-  T scale_x(T x) const { return x * T(m_new_width) / T(m_sprite->width()); }
+  T scale_x(T x) const { return x * T(m_new_width) / T(sprite()->width()); }
 
   template<typename T>
-  T scale_y(T y) const { return y * T(m_new_height) / T(m_sprite->height()); }
+  T scale_y(T y) const { return y * T(m_new_height) / T(sprite()->height()); }
 
   template<typename T>
   gfx::RectT<T> scale_rect(const gfx::RectT<T>& rc) const {
@@ -69,11 +63,7 @@ class SpriteSizeJob : public Job {
 public:
 
   SpriteSizeJob(const ContextReader& reader, int new_width, int new_height, ResizeMethod resize_method)
-    : Job("Sprite Size")
+    : SpriteJob(reader, "Sprite Size") {
-    , m_writer(reader)
-    , m_document(m_writer.document())
-    , m_sprite(m_writer.sprite())
-  {
     m_new_width = new_width;
     m_new_height = new_height;
     m_resize_method = resize_method;
@@ -81,34 +71,30 @@ public:
 
 protected:
 
-  /**
+  // [working thread]
-   * [working thread]
+  void onJob() override {
-   */
+    DocumentApi api = writer().document()->getApi(transaction());
-  virtual void onJob()
-  {
-    Transaction transaction(m_writer.context(), "Sprite Size");
-    DocumentApi api = m_writer.document()->getApi(transaction);
 
     int cels_count = 0;
-    for (Cel* cel : m_sprite->uniqueCels()) { // TODO add size() member function to CelsRange
+    for (Cel* cel : sprite()->uniqueCels()) { // TODO add size() member function to CelsRange
       (void)cel;
       ++cels_count;
     }
 
     // For each cel...
     int progress = 0;
-    for (Cel* cel : m_sprite->uniqueCels()) {
+    for (Cel* cel : sprite()->uniqueCels()) {
       // Get cel's image
       Image* image = cel->image();
       if (image && !cel->link()) {
         // Resize the cel bounds only if it's from a reference layer
         if (cel->layer()->isReference()) {
           gfx::RectF newBounds = scale_rect<double>(cel->boundsF());
-          transaction.execute(new cmd::SetCelBoundsF(cel, newBounds));
+          transaction().execute(new cmd::SetCelBoundsF(cel, newBounds));
         }
         else {
           // Change its location
-          api.setCelPosition(m_sprite, cel, scale_x(cel->x()), scale_y(cel->y()));
+          api.setCelPosition(sprite(), cel, scale_x(cel->x()), scale_y(cel->y()));
 
           // Resize the image
           int w = scale_x(image->width());
@@ -120,11 +106,11 @@ protected:
           doc::algorithm::resize_image(
             image, new_image.get(),
             m_resize_method,
-            m_sprite->palette(cel->frame()),
+            sprite()->palette(cel->frame()),
-            m_sprite->rgbMap(cel->frame()),
+            sprite()->rgbMap(cel->frame()),
-            (cel->layer()->isBackground() ? -1: m_sprite->transparentColor()));
+            (cel->layer()->isBackground() ? -1: sprite()->transparentColor()));
 
-          api.replaceImage(m_sprite, cel->imageRef(), new_image);
+          api.replaceImage(sprite(), cel->imageRef(), new_image);
         }
       }
 
@@ -137,24 +123,24 @@ protected:
     }
 
     // Resize mask
-    if (m_document->isMaskVisible()) {
+    if (document()->isMaskVisible()) {
       ImageRef old_bitmap
-        (crop_image(m_document->mask()->bitmap(), -1, -1,
+        (crop_image(document()->mask()->bitmap(), -1, -1,
-                    m_document->mask()->bitmap()->width()+2,
+                    document()->mask()->bitmap()->width()+2,
-                    m_document->mask()->bitmap()->height()+2, 0));
+                    document()->mask()->bitmap()->height()+2, 0));
 
       int w = scale_x(old_bitmap->width());
       int h = scale_y(old_bitmap->height());
       base::UniquePtr<Mask> new_mask(new Mask);
       new_mask->replace(
         gfx::Rect(
-          scale_x(m_document->mask()->bounds().x-1),
+          scale_x(document()->mask()->bounds().x-1),
-          scale_y(m_document->mask()->bounds().y-1), MAX(1, w), MAX(1, h)));
+          scale_y(document()->mask()->bounds().y-1), MAX(1, w), MAX(1, h)));
       algorithm::resize_image(
         old_bitmap.get(), new_mask->bitmap(),
         m_resize_method,
-        m_sprite->palette(0), // Ignored
+        sprite()->palette(0), // Ignored
-        m_sprite->rgbMap(0),  // Ignored
+        sprite()->rgbMap(0),  // Ignored
         -1);                  // Ignored
 
       // Reshrink
@@ -164,12 +150,12 @@ protected:
       api.copyToCurrentMask(new_mask);
 
       // Regenerate mask
-      m_document->resetTransformation();
+      document()->resetTransformation();
-      m_document->generateMaskBoundaries();
+      document()->generateMaskBoundaries();
     }
 
     // Resize slices
-    for (auto& slice : m_document->sprite()->slices()) {
+    for (auto& slice : sprite()->slices()) {
       for (auto& k : *slice) {
         const SliceKey& key = *k.value();
         if (key.isEmpty())
@@ -185,16 +171,13 @@ protected:
           newKey.setPivot(gfx::Point(scale_x(newKey.pivot().x),
                                      scale_y(newKey.pivot().y)));
 
-        transaction.execute(
+        transaction().execute(
           new cmd::SetSliceKey(slice, k.frame(), newKey));
       }
     }
 
     // Resize Sprite
-    api.setSpriteSize(m_sprite, m_new_width, m_new_height);
+    api.setSpriteSize(sprite(), m_new_width, m_new_height);
-
-    // Commit changes
-    transaction.commit();
   }
 
 };

src/app/render_task_job.cpp

@@ -1,35 +0,0 @@
-// Aseprite
-// Copyright (C) 2017  David Capello
-//
-// This program is distributed under the terms of
-// the End-User License Agreement for Aseprite.
-
-#ifdef HAVE_CONFIG_H
-#include "config.h"
-#endif
-
-#include "app/render_task_job.h"
-
-namespace app {
-
-void RenderTaskJob::onJob()
-{
-  try {
-    m_func();
-  }
-  catch (const std::exception&) {
-    // TODO show the exception
-  }
-}
-
-bool RenderTaskJob::continueTask()
-{
-  return !isCanceled();
-}
-
-void RenderTaskJob::notifyTaskProgress(double progress)
-{
-  jobProgress(progress);
-}
-
-} // namespace app

src/app/render_task_job.h

@@ -1,43 +0,0 @@
-// Aseprite
-// Copyright (C) 2017  David Capello
-//
-// This program is distributed under the terms of
-// the End-User License Agreement for Aseprite.
-
-#ifndef APP_RENDER_TASK_JOB_H_INCLUDED
-#define APP_RENDER_TASK_JOB_H_INCLUDED
-#pragma once
-
-#include "app/job.h"
-#include "render/task_delegate.h"
-
-#include <functional>
-
-namespace app {
-
-class RenderTaskJob : public Job,
-                      public render::TaskDelegate {
-public:
-  RenderTaskJob(const char* jobName)
-    : Job(jobName) {
-  }
-
-  template<typename T>
-  void startJob(T&& func) {
-    m_func = std::move(func);
-    Job::startJob();
-  }
-
-private:
-  void onJob() override;
-
-  // render::TaskDelegate impl
-  bool continueTask() override;
-  void notifyTaskProgress(double progress) override;
-
-  std::function<void()> m_func;
-};
-
-} // namespace app
-
-#endif

src/app/sprite_job.cpp

@@ -0,0 +1,45 @@
+// Aseprite
+// Copyright (C) 2017  David Capello
+//
+// This program is distributed under the terms of
+// the End-User License Agreement for Aseprite.
+
+#ifdef HAVE_CONFIG_H
+#include "config.h"
+#endif
+
+#include "app/sprite_job.h"
+
+namespace app {
+
+SpriteJob::SpriteJob(const ContextReader& reader, const char* jobName)
+  : Job(jobName)
+  , m_writer(reader, 500)
+  , m_document(m_writer.document())
+  , m_sprite(m_writer.sprite())
+  , m_transaction(m_writer.context(), jobName, ModifyDocument)
+{
+}
+
+SpriteJob::~SpriteJob()
+{
+  if (!isCanceled())
+    m_transaction.commit();
+}
+
+void SpriteJob::onJob()
+{
+  m_callback();
+}
+
+bool SpriteJob::continueTask()
+{
+  return !isCanceled();
+}
+
+void SpriteJob::notifyTaskProgress(double progress)
+{
+  jobProgress(progress);
+}
+
+} // namespace app

src/app/sprite_job.h

@@ -0,0 +1,60 @@
+// Aseprite
+// Copyright (C) 2017  David Capello
+//
+// This program is distributed under the terms of
+// the End-User License Agreement for Aseprite.
+
+#ifndef APP_SPRITE_JOB_H_INCLUDED
+#define APP_SPRITE_JOB_H_INCLUDED
+#pragma once
+
+#include "app/context.h"
+#include "app/context_access.h"
+#include "app/job.h"
+#include "app/transaction.h"
+#include "render/task_delegate.h"
+
+#include <functional>
+
+namespace app {
+
+class SpriteJob : public Job,
+                  public render::TaskDelegate {
+public:
+  SpriteJob(const ContextReader& reader, const char* jobName);
+  ~SpriteJob();
+
+  ContextWriter& writer() { return m_writer; }
+  Document* document() const { return m_document; }
+  Sprite* sprite() const { return m_sprite; }
+  Transaction& transaction() { return m_transaction; }
+
+  template<typename T>
+  void startJobWithCallback(T&& callback) {
+    m_callback = std::move(callback);
+    Job::startJob();
+  }
+
+private:
+  // Job impl
+  void onJob() override;
+
+  // render::TaskDelegate impl just in case you need to use this
+  // Job as a delegate in render::create_palette_from_sprite()
+  bool continueTask() override;
+  void notifyTaskProgress(double progress) override;
+
+  ContextWriter m_writer;
+  Document* m_document;
+  Sprite* m_sprite;
+  Transaction m_transaction;
+
+  // Default implementation calls the given function in
+  // startJob(). Anyway you can just extended the SpriteJob and
+  // override onJob().
+  std::function<void()> m_callback;
+};
+
+} // namespace app
+
+#endif // APP_SPRITE_JOB_H_INCLUDED