Refresh screen/current palette automatically on Transaction::commit() (#378)

Removed app_update_current_palette() with this patch.

src/app/app.cpp

@@ -639,26 +639,18 @@ InputChain& App::inputChain()
 }
 #endif
 
-void app_update_current_palette()
-{
-#ifdef ENABLE_UI
-  Context* context = UIContext::instance();
-  ASSERT(context != NULL);
-
-  Site site = context->activeSite();
-
-  if (Palette* pal = site.palette())
-    set_current_palette(pal, false);
-  else
-    set_current_palette(nullptr, false);
-#endif // ENABLE_UI
-}
-
 // Updates palette and redraw the screen.
 void app_refresh_screen()
 {
 #ifdef ENABLE_UI
-  app_update_current_palette();
+  Context* ctx = UIContext::instance();
+  ASSERT(ctx != NULL);
+
+  Site site = ctx->activeSite();
+  if (Palette* pal = site.palette())
+    set_current_palette(pal, false);
+  else
+    set_current_palette(nullptr, false);
 
   // Invalidate the whole screen.
   ui::Manager::getDefault()->invalidate();

src/app/app.h

@@ -147,7 +147,6 @@ namespace app {
 #endif
   };
 
-  void app_update_current_palette();
   void app_refresh_screen();
   void app_rebuild_documents_tabs();
   PixelFormat app_get_current_pixel_format();

src/app/cmd/set_palette.cpp

@@ -1,4 +1,5 @@
 // Aseprite
+// Copyright (C) 2019  Igara Studio S.A.
 // Copyright (C) 2001-2015  David Capello
 //
 // This program is distributed under the terms of
@@ -10,6 +11,7 @@
 
 #include "app/cmd/set_palette.h"
 
+#include "app/doc.h"
 #include "base/serialization.h"
 #include "doc/palette.h"
 #include "doc/sprite.h"
@@ -77,5 +79,12 @@ void SetPalette::onUndo()
   palette->incrementVersion();
 }
 
+void SetPalette::onFireNotifications()
+{
+  doc::Sprite* sprite = this->sprite();
+  Doc* doc = static_cast<Doc*>(sprite->document());
+  doc->notifyPaletteChanged();
+}
+
 } // namespace cmd
 } // namespace app

src/app/cmd/set_palette.h

@@ -1,4 +1,5 @@
 // Aseprite
+// Copyright (C) 2019  Igara Studio S.A.
 // Copyright (C) 2001-2015  David Capello
 //
 // This program is distributed under the terms of
@@ -32,6 +33,7 @@ namespace cmd {
   protected:
     void onExecute() override;
     void onUndo() override;
+    void onFireNotifications() override;
     size_t onMemSize() const override {
       return sizeof(*this) +
         sizeof(doc::color_t) * (m_oldColors.size() +

src/app/commands/cmd_add_color.cpp

@@ -16,14 +16,12 @@
 #include "app/context.h"
 #include "app/context_access.h"
 #include "app/i18n/strings.h"
-#include "app/modules/palettes.h"
 #include "app/tx.h"
 #include "app/ui/color_bar.h"
 #include "app/ui/context_bar.h"
 #include "app/ui/editor/editor.h"
 #include "doc/palette.h"
 #include "fmt/format.h"
-#include "ui/manager.h"
 
 namespace app {
 
@@ -87,8 +85,13 @@ void AddColorCommand::onExecute(Context* ctx)
       break;
   }
 
+  Palette* pal = ctx->activeSite().palette();
+  ASSERT(pal);
+  if (!pal)
+    return;
+
   try {
-    Palette* newPalette = get_current_palette(); // System current pal
+    std::unique_ptr<Palette> newPalette(new Palette(*pal));
     color_t color = doc::rgba(
       appColor.getRed(),
       appColor.getGreen(),
@@ -122,12 +125,9 @@ void AddColorCommand::onExecute(Context* ctx)
       frame_t frame = writer.frame();
 
       Tx tx(writer.context(), friendlyName(), ModifyDocument);
-      tx(new cmd::SetPalette(sprite, frame, newPalette));
+      tx(new cmd::SetPalette(sprite, frame, newPalette.get()));
       tx.commit();
     }
-
-    set_current_palette(newPalette, true);
-    ui::Manager::getDefault()->invalidate();
   }
   catch (base::Exception& e) {
     Console::showException(e);

src/app/commands/cmd_color_quantization.cpp

@@ -16,7 +16,6 @@
 #include "app/context.h"
 #include "app/context_access.h"
 #include "app/job.h"
-#include "app/modules/palettes.h"
 #include "app/pref/preferences.h"
 #include "app/sprite_job.h"
 #include "app/transaction.h"
@@ -25,7 +24,6 @@
 #include "doc/palette.h"
 #include "doc/sprite.h"
 #include "render/quantization.h"
-#include "ui/manager.h"
 
 #include "palette_from_sprite.xml.h"
 
@@ -156,7 +154,7 @@ void ColorQuantizationCommand::onExecute(Context* ctx)
 
     std::unique_ptr<Palette> newPalette(
       new Palette(createPal ? tmpPalette:
-                              *get_current_palette()));
+                              *site.palette()));
 
     if (createPal) {
       entries = PalettePicks(newPalette->size());
@@ -172,13 +170,6 @@ void ColorQuantizationCommand::onExecute(Context* ctx)
 
     if (*curPalette != *newPalette)
       job.tx()(new cmd::SetPalette(sprite, frame, newPalette.get()));
-
-#ifdef ENABLE_UI
-    if (ui) {
-      set_current_palette(newPalette.get(), false);
-      ui::Manager::getDefault()->invalidate();
-    }
-#endif // ENABLE_UI
   }
   catch (const base::Exception& e) {
     Console::showException(e);

src/app/commands/cmd_palette_size.cpp

@@ -13,11 +13,9 @@
 #include "app/commands/command.h"
 #include "app/commands/params.h"
 #include "app/context_access.h"
-#include "app/modules/palettes.h"
 #include "app/tx.h"
 #include "doc/palette.h"
 #include "doc/sprite.h"
-#include "ui/manager.h"
 
 #include "palette_size.xml.h"
 
@@ -51,8 +49,9 @@ void PaletteSizeCommand::onLoadParams(const Params& params)
 void PaletteSizeCommand::onExecute(Context* context)
 {
   ContextReader reader(context);
-  frame_t frame = reader.frame();
-  Palette palette(*reader.sprite()->palette(frame));
+  const frame_t frame = reader.frame();
+  ASSERT(reader.palette());
+  Palette palette(*reader.palette());
   int ncolors = (m_size != 0 ? m_size: palette.size());
 
 #ifdef ENABLE_UI
@@ -76,12 +75,6 @@ void PaletteSizeCommand::onExecute(Context* context)
   Tx tx(context, "Palette Size", ModifyDocument);
   tx(new cmd::SetPalette(writer.sprite(), frame, &palette));
   tx.commit();
-
-  set_current_palette(&palette, false);
-#ifdef ENABLE_UI
-  if (context->isUIAvailable())
-    ui::Manager::getDefault()->invalidate();
-#endif
 }
 
 Command* CommandFactory::createPaletteSizeCommand()

src/app/doc.cpp

@@ -146,6 +146,13 @@ void Doc::notifyColorSpaceChanged()
   notify_observers<DocEvent&>(&DocObserver::onColorSpaceChanged, ev);
 }
 
+void Doc::notifyPaletteChanged()
+{
+  DocEvent ev(this);
+  ev.sprite(sprite());
+  notify_observers<DocEvent&>(&DocObserver::onPaletteChanged, ev);
+}
+
 void Doc::notifySpritePixelsModified(Sprite* sprite, const gfx::Region& region, frame_t frame)
 {
   DocEvent ev(this);
@@ -549,14 +556,6 @@ void Doc::updateOSColorSpace(bool appWideSignal)
       context()->activeDocument() == this) {
     App::instance()->ColorSpaceChange();
   }
-
-  if (ui::is_ui_thread()) {
-    // As the color space has changed, we might need to upate the
-    // current palette (because the color space conversion might be
-    // came from a cmd::ConvertColorProfile, so the palette might be
-    // changed). This might generate a PaletteChange() signal.
-    app_update_current_palette();
-  }
 }
 
 // static

src/app/doc.h

@@ -95,6 +95,7 @@ namespace app {
 
     void notifyGeneralUpdate();
     void notifyColorSpaceChanged();
+    void notifyPaletteChanged();
     void notifySpritePixelsModified(Sprite* sprite, const gfx::Region& region, frame_t frame);
     void notifyExposeSpritePixels(Sprite* sprite, const gfx::Region& region);
     void notifyLayerMergedDown(Layer* srcLayer, Layer* targetLayer);

src/app/doc_observer.h

@@ -24,8 +24,8 @@ namespace app {
     virtual void onGeneralUpdate(DocEvent& ev) { }
 
     virtual void onColorSpaceChanged(DocEvent& ev) { }
-
     virtual void onPixelFormatChanged(DocEvent& ev) { }
+    virtual void onPaletteChanged(DocEvent& ev) { }
 
     virtual void onAddLayer(DocEvent& ev) { }
     virtual void onAddFrame(DocEvent& ev) { }

src/app/transaction.cpp

@@ -15,7 +15,9 @@
 #include "app/context_access.h"
 #include "app/doc.h"
 #include "app/doc_undo.h"
+#include "app/modules/palettes.h"
 #include "doc/sprite.h"
+#include "ui/manager.h"
 #include "ui/system.h"
 
 #define TX_TRACE(...)
@@ -88,6 +90,8 @@ void Transaction::commit()
   TX_TRACE("TX: Commit <%s>\n", m_cmds->label().c_str());
 
   m_cmds->updateSpritePositionAfter();
+  const SpritePosition sprPos = m_cmds->spritePositionAfterExecute();
+
   m_undo->add(m_cmds);
   m_cmds = nullptr;
 
@@ -96,6 +100,23 @@ void Transaction::commit()
     m_doc->resetTransformation();
     m_doc->generateMaskBoundaries();
   }
+
+#ifdef ENABLE_UI
+  if (int(m_changes) & int(Changes::kColorChange)) {
+    ASSERT(m_doc);
+    ASSERT(m_doc->sprite());
+
+    Palette* pal = m_doc->sprite()->palette(sprPos.frame());
+    ASSERT(pal);
+    if (pal)
+      set_current_palette(pal, false);
+    else
+      set_current_palette(nullptr, false);
+
+    if (m_ctx->isUIAvailable())
+      ui::Manager::getDefault()->invalidate();
+  }
+#endif
 }
 
 void Transaction::rollbackAndStartAgain()
@@ -141,4 +162,14 @@ void Transaction::onSelectionChanged(DocEvent& ev)
   m_changes = Changes(int(m_changes) | int(Changes::kSelection));
 }
 
+void Transaction::onColorSpaceChanged(DocEvent& ev)
+{
+  m_changes = Changes(int(m_changes) | int(Changes::kColorChange));
+}
+
+void Transaction::onPaletteChanged(DocEvent& ev)
+{
+  m_changes = Changes(int(m_changes) | int(Changes::kColorChange));
+}
+
 } // namespace app

src/app/transaction.h

@@ -31,6 +31,11 @@ namespace app {
   // whole operation if something fails (e.g. an exceptions is thrown)
   // in the middle of the procedure.
   //
+  // This class is a DocObserver because it listen and accumulates the
+  // changes in the Doc (m_changes), and when the transaction ends, it
+  // processes those changes as UI updates (so widgets are
+  // invalidated/updated correctly to show the new Doc state).
+  //
   // You have to wrap every call to an transaction with a
   // ContextWriter. The preferred usage is as follows:
   //
@@ -79,13 +84,21 @@ namespace app {
 
   private:
     // List of changes during the execution of this transaction
-    enum class Changes { kNone = 0,
-                         kSelection = 1 };
+    enum class Changes {
+      kNone = 0,
+      // The selection has changed so we have to re-generate the
+      // boundary segments.
+      kSelection = 1,
+      // The color palette or color space has changed.
+      kColorChange = 2
+    };
 
     void rollback(CmdTransaction* newCmds);
 
     // DocObserver impl
     void onSelectionChanged(DocEvent& ev) override;
+    void onColorSpaceChanged(DocEvent& ev) override;
+    void onPaletteChanged(DocEvent& ev) override;
 
     Context* m_ctx;
     Doc* m_doc;

src/app/ui/color_bar.cpp

@@ -633,9 +633,6 @@ void ColorBar::setPalette(const doc::Palette* newPalette, const std::string& act
   catch (base::Exception& e) {
     Console::showException(e);
   }
-
-  set_current_palette(newPalette, false);
-  manager()->invalidate();
 }
 
 void ColorBar::setTransparentIndex(int index)

src/app/ui_context.cpp

@@ -125,9 +125,10 @@ void UIContext::setActiveView(DocView* docView)
   m_lastSelectedView = docView;
 
   // TODO all the calls to functions like updateUsingEditor(),
-  // setPixelFormat(), app_refresh_screen(), updateDisplayTitleBar()
-  // Can be replaced with a ContextObserver listening to the
-  // onActiveSiteChange() event.
+  // setPixelFormat(), app_refresh_screen(), updateDisplayTitleBar(),
+  // etc. could be replaced with the Transaction class, which is a
+  // DocObserver and handles updates on the screen processing the
+  // observed changes.
   notifyActiveSiteChanged();
 }