Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Invalidate Preview Window when pasting Frame (fix #2215) #3434

Closed
wants to merge 1 commit into from

Conversation

Joefish
Copy link
Contributor

@Joefish Joefish commented Jul 20, 2022

I agree that my contributions are licensed under the Individual Contributor License Agreement V3.0 ("CLA") as stated in https://github.com/aseprite/sourcecode/blob/main/cla.md

I have signed the CLA following the steps given in https://github.com/aseprite/sourcecode/blob/main/sign-cla.md#sign-the-cla

@Joefish Joefish requested a review from dacap as a code owner July 20, 2022 03:24
@Joefish
Copy link
Contributor Author

Joefish commented Jul 20, 2022

While the original Issue #2215 that the frame editor was not updated when pasting a cel, was resolved, the preview window still did not update correctly. I know throwing another widget invalidate in there is not the best solution. I am still new to the code base so it took me a bit to understand how the messaging and observer system work in this case.
PreviewEditorWindow inherits from EditorObserver, and although it has onBefore/AfterFrameChanged, it didn't fire when the Cel was pasted and rendered in the editor. The Document observer interface has a onCelCopied function that could be used by adding a DocObserver to the PreviewEditor Class would be needed or extending the Editor Class to forward the notify.
A working example would be like this,

diff --git a/src/app/ui/preview_editor.cpp b/src/app/ui/preview_editor.cpp
index 9c89a6fa5..4c63d96cd 100644
--- a/src/app/ui/preview_editor.cpp
+++ b/src/app/ui/preview_editor.cpp
@@ -379,6 +379,7 @@ void PreviewEditorWindow::updateUsingEditor(Editor* editor)
     miniEditor->setState(EditorStatePtr(new NavigateState));
     miniEditor->setAnimationSpeedMultiplier(m_aniSpeed);
     miniEditor->add_observer(this);
+    document->add_observer(this);
     layout();
·
     if (!autoScroll)
@@ -513,4 +514,9 @@ void PreviewEditorWindow::adjustPlayingTag()
                    Preferences::instance().preview.playAll());
 }
·
+void PreviewEditorWindow::onCelCopied(DocEvent &ev)
+{
+  invalidate();
+}
+
 } // namespace app
diff --git a/src/app/ui/preview_editor.h b/src/app/ui/preview_editor.h
index ec7d20c7e..ea0c6fdf3 100644
--- a/src/app/ui/preview_editor.h
+++ b/src/app/ui/preview_editor.h
@@ -21,6 +21,7 @@ namespace app {
·
   class PreviewEditorWindow : public ui::Window
                             , public EditorObserver
+                            , public DocObserver
                             , public DocViewPreviewDelegate {
   public:
     PreviewEditorWindow();
@@ -51,6 +52,8 @@ namespace app {
     void onClose(ui::CloseEvent& ev) override;
     void onWindowResize() override;
·
+    void onCelCopied(DocEvent& ev) override;
+
   private:
     void uncheckCenterButton();
     bool hasDocument() const;

But as I said I wanted to keep the changes to a minimum and imo another observer class just for this would be overkill.

@dacap dacap self-assigned this Nov 17, 2022
@dacap dacap added the wip label Nov 17, 2022
@dacap
Copy link
Member

dacap commented Nov 17, 2022

Thanks for your PR @Joefish 👍 I've just tested and while it fixes the problem with the preview editor, I saw that it doesn't fix the problem if we have multiple editors, e.g. (in second 0:05 the cel is pasted):

Screencast.2022-11-17.11.17.18.mp4

As you saw, an onCelCopied solution might be better, but I've found other bigger problems when frames are copied (whole frames). I'll review the behavior of cels, frames, and layers, and fix them.

@dacap
Copy link
Member

dacap commented Nov 17, 2022

I've used Doc::notifyGeneralUpdate to update all visible Editor/DocView: d8c1b19

Also there were a missing redraw when copying/pasting frames (not cels) in the same doc (the DocRange::kFrames case). This was fixed too in the commit. Thanks for your help @Joefish to detect and fix this issue 👍

@dacap dacap closed this Nov 17, 2022
@dacap dacap removed the wip label Nov 17, 2022
@Joefish Joefish deleted the issue-2215 branch December 4, 2022 02:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants