From 395be62b03b2a50d0ec4b9061478ceeba3f3ca54 Mon Sep 17 00:00:00 2001 From: David Capello Date: Sat, 29 Mar 2014 16:31:39 -0300 Subject: [PATCH] Fix issue #310 - crash on export sprite sheet The timeline wasn't being added as a Document observer, so it wasn't getting notifications about layers modifications. In this way the timeline wasn't able to update its m_layers[] internal field and was accessing to removed layers/invalid memory. --- src/app/document_api.cpp | 5 +- src/app/document_observer.h | 3 +- src/app/ui/document_view.cpp | 2 +- src/app/ui/document_view.h | 2 +- src/app/ui/timeline.cpp | 128 +++++++++++++++++++---------------- src/app/ui/timeline.h | 2 +- src/base/observers.h | 4 ++ 7 files changed, 83 insertions(+), 63 deletions(-) diff --git a/src/app/document_api.cpp b/src/app/document_api.cpp index e9e045ef6..79bc611f9 100644 --- a/src/app/document_api.cpp +++ b/src/app/document_api.cpp @@ -632,7 +632,7 @@ void DocumentApi::removeLayer(Layer* layer) DocumentEvent ev(m_document); ev.sprite(layer->getSprite()); ev.layer(layer); - m_document->notifyObservers(&DocumentObserver::onRemoveLayer, ev); + m_document->notifyObservers(&DocumentObserver::onBeforeRemoveLayer, ev); // Add undoers. if (undoEnabled()) @@ -640,6 +640,9 @@ void DocumentApi::removeLayer(Layer* layer) // Do the action. layer->getParent()->removeLayer(layer); + + m_document->notifyObservers(&DocumentObserver::onAfterRemoveLayer, ev); + delete layer; } diff --git a/src/app/document_observer.h b/src/app/document_observer.h index a4f00c0fd..0d6e9646b 100644 --- a/src/app/document_observer.h +++ b/src/app/document_observer.h @@ -41,7 +41,8 @@ namespace app { virtual void onRemoveSprite(DocumentEvent& ev) { } - virtual void onRemoveLayer(DocumentEvent& ev) { } + virtual void onBeforeRemoveLayer(DocumentEvent& ev) { } + virtual void onAfterRemoveLayer(DocumentEvent& ev) { } // Called when a frame is removed. It's called after the frame was // removed, and the sprite's total number of frames is modified. diff --git a/src/app/ui/document_view.cpp b/src/app/ui/document_view.cpp index d3cf0788a..fcdcb9649 100644 --- a/src/app/ui/document_view.cpp +++ b/src/app/ui/document_view.cpp @@ -218,7 +218,7 @@ void DocumentView::onAddLayer(DocumentEvent& ev) } } -void DocumentView::onRemoveLayer(DocumentEvent& ev) +void DocumentView::onBeforeRemoveLayer(DocumentEvent& ev) { Sprite* sprite = ev.sprite(); Layer* layer = ev.layer(); diff --git a/src/app/ui/document_view.h b/src/app/ui/document_view.h index 3ed84be9a..9c85671dd 100644 --- a/src/app/ui/document_view.h +++ b/src/app/ui/document_view.h @@ -64,7 +64,7 @@ namespace app { void onSpritePixelsModified(DocumentEvent& ev) OVERRIDE; void onLayerMergedDown(DocumentEvent& ev) OVERRIDE; void onAddLayer(DocumentEvent& ev) OVERRIDE; - void onRemoveLayer(DocumentEvent& ev) OVERRIDE; + void onBeforeRemoveLayer(DocumentEvent& ev) OVERRIDE; void onAddFrame(DocumentEvent& ev) OVERRIDE; void onRemoveFrame(DocumentEvent& ev) OVERRIDE; void onTotalFramesChanged(DocumentEvent& ev) OVERRIDE; diff --git a/src/app/ui/timeline.cpp b/src/app/ui/timeline.cpp index 3c817d030..62e5ae5a3 100644 --- a/src/app/ui/timeline.cpp +++ b/src/app/ui/timeline.cpp @@ -180,6 +180,8 @@ void Timeline::updateUsingEditor(Editor* editor) DocumentView* view = m_editor->getDocumentView(); view->getDocumentLocation(&location); + location.document()->addObserver(this); + // If we are already in the same position as the "editor", we don't // need to update the at all timeline. if (m_document == location.document() && @@ -859,72 +861,82 @@ void Timeline::onPreferredSize(PreferredSizeEvent& ev) void Timeline::onPaint(ui::PaintEvent& ev) { Graphics* g = ev.getGraphics(); + bool noDoc = (m_document == NULL); + if (noDoc) + goto paintNoDoc; - if (!m_document) { - drawPart(g, getClientBounds(), NULL, m_timelinePaddingStyle); - return; - } + try { + // Lock the sprite to read/render it. + const DocumentReader documentReader(m_document); + + int layer, first_layer, last_layer; + FrameNumber frame, first_frame, last_frame; - int layer, first_layer, last_layer; - FrameNumber frame, first_frame, last_frame; + getDrawableLayers(g, &first_layer, &last_layer); + getDrawableFrames(g, &first_frame, &last_frame); - getDrawableLayers(g, &first_layer, &last_layer); - getDrawableFrames(g, &first_frame, &last_frame); + // Draw the header for layers. + drawHeader(g); - // Draw the header for layers. - drawHeader(g); + // Draw the header for each visible frame. + { + IntersectClip clip(g, getFrameHeadersBounds()); + if (clip) { + for (frame=first_frame; frame<=last_frame; ++frame) + drawHeaderFrame(g, frame); - // Draw the header for each visible frame. - { - IntersectClip clip(g, getFrameHeadersBounds()); - if (clip) { - for (frame=first_frame; frame<=last_frame; ++frame) - drawHeaderFrame(g, frame); - - // Draw onionskin indicators. - gfx::Rect bounds = getOnionskinFramesBounds(); - if (!bounds.isEmpty()) { - drawPart(g, bounds, - NULL, m_timelineOnionskinRangeStyle, - false, false, false); + // Draw onionskin indicators. + gfx::Rect bounds = getOnionskinFramesBounds(); + if (!bounds.isEmpty()) { + drawPart(g, bounds, + NULL, m_timelineOnionskinRangeStyle, + false, false, false); + } } } + + // Draw each visible layer. + for (layer=first_layer; layer<=last_layer; layer++) { + { + IntersectClip clip(g, getLayerHeadersBounds()); + if (clip) + drawLayer(g, layer); + } + + // Get the first CelIterator to be drawn (it is the first cel with cel->frame >= first_frame) + CelIterator it, end; + Layer* layerPtr = m_layers[layer]; + if (layerPtr->isImage()) { + it = static_cast(layerPtr)->getCelBegin(); + end = static_cast(layerPtr)->getCelEnd(); + for (; it != end && (*it)->getFrame() < first_frame; ++it) + ; + } + + IntersectClip clip(g, getCelsBounds()); + if (!clip) + continue; + + // Draw every visible cel for each layer. + for (frame=first_frame; frame<=last_frame; ++frame) { + Cel* cel = (layerPtr->isImage() && it != end && (*it)->getFrame() == frame ? *it: NULL); + + drawCel(g, layer, frame, cel); + + if (cel) + ++it; // Go to next cel + } + } + + drawPaddings(g); + } + catch (const LockedDocumentException&) { + noDoc = true; } - // Draw each visible layer. - for (layer=first_layer; layer<=last_layer; layer++) { - { - IntersectClip clip(g, getLayerHeadersBounds()); - if (clip) - drawLayer(g, layer); - } - - // Get the first CelIterator to be drawn (it is the first cel with cel->frame >= first_frame) - CelIterator it, end; - Layer* layerPtr = m_layers[layer]; - if (layerPtr->isImage()) { - it = static_cast(layerPtr)->getCelBegin(); - end = static_cast(layerPtr)->getCelEnd(); - for (; it != end && (*it)->getFrame() < first_frame; ++it) - ; - } - - IntersectClip clip(g, getCelsBounds()); - if (!clip) - continue; - - // Draw every visible cel for each layer. - for (frame=first_frame; frame<=last_frame; ++frame) { - Cel* cel = (layerPtr->isImage() && it != end && (*it)->getFrame() == frame ? *it: NULL); - - drawCel(g, layer, frame, cel); - - if (cel) - ++it; // Go to next cel - } - } - - drawPaddings(g); +paintNoDoc:; + if (noDoc) + drawPart(g, getClientBounds(), NULL, m_timelinePaddingStyle); } void Timeline::onCommandAfterExecution(Context* context) @@ -954,7 +966,7 @@ void Timeline::onAddLayer(DocumentEvent& ev) invalidate(); } -void Timeline::onRemoveLayer(DocumentEvent& ev) +void Timeline::onAfterRemoveLayer(DocumentEvent& ev) { Sprite* sprite = ev.sprite(); Layer* layer = ev.layer(); diff --git a/src/app/ui/timeline.h b/src/app/ui/timeline.h index 3e6e3a0b3..263bb464d 100644 --- a/src/app/ui/timeline.h +++ b/src/app/ui/timeline.h @@ -114,7 +114,7 @@ namespace app { // DocumentObserver impl. void onAddLayer(DocumentEvent& ev) OVERRIDE; - void onRemoveLayer(DocumentEvent& ev) OVERRIDE; + void onAfterRemoveLayer(DocumentEvent& ev) OVERRIDE; void onAddFrame(DocumentEvent& ev) OVERRIDE; void onRemoveFrame(DocumentEvent& ev) OVERRIDE; diff --git a/src/base/observers.h b/src/base/observers.h index 51388fceb..c0b7d884b 100644 --- a/src/base/observers.h +++ b/src/base/observers.h @@ -39,6 +39,7 @@ public: // collection and will be destroyed calling the T::dispose() member // function. void addObserver(observer_type* observer) { + ASSERT(std::find(m_observers.begin(), m_observers.end(), observer) == m_observers.end() && "You've tried to add an observer that already is in the collection"); m_observers.push_back(observer); } @@ -48,6 +49,9 @@ public: iterator it = std::find(m_observers.begin(), m_observers.end(), observer); if (it != end()) m_observers.erase(it); + else { + ASSERT(false && "You've tried to remove an observer that isn't in the collection"); + } } void notifyObservers(void (observer_type::*method)()) {