Fix scripting app.command.Cut() and app.command.Paste() does not work in --batch mode (fix #4354) #4446

Gasparoken · 2024-05-02T21:29:40Z

It fixes the original problem, but I need advice on this approach.

aseprite-bot

clang-tidy made some suggestions

aseprite-bot · 2024-05-02T21:33:06Z

src/app/util/clipboard.cpp

@@ -290,7 +290,7 @@ bool Clipboard::copyFromDocument(const Site& site, bool merged)
    (mask ? new Mask(*mask): nullptr),
    (pal ? new Palette(*pal): nullptr),
    nullptr,
-    true,                       // set native clipboard
+    App::instance()->isGui() ? true : false, // set native clipboard


warning: redundant boolean literal in ternary expression result [readability-simplify-boolean-expr]

Suggested change

App::instance()->isGui() ? true : false, // set native clipboard

App::instance()->isGui(), // set native clipboard

I think we can just leave the true as it was (probably some batch scripts will want to copy some content to the native clipboard).

aseprite-bot · 2024-05-02T21:33:07Z

src/app/util/clipboard.cpp

@@ -487,7 +504,9 @@

    case ClipboardFormat::Image: {
      // Get the image from the native clipboard.
-      if (!getImage(nullptr))
+      if (editor && !getImage(nullptr))


warning: implicit conversion 'Editor *' -> bool [readability-implicit-bool-conversion]

Suggested change

if (editor && !getImage(nullptr))

if ((editor != nullptr) && !getImage(nullptr))

aseprite-bot · 2024-05-02T21:33:07Z

src/app/util/clipboard.cpp

@@ -487,7 +504,9 @@

    case ClipboardFormat::Image: {
      // Get the image from the native clipboard.
-      if (!getImage(nullptr))
+      if (editor && !getImage(nullptr))
+        return;


warning: repeated branch in conditional chain [bugprone-branch-clone]

return; ^

Additional context

src/app/util/clipboard.cpp:507: end of the original

return; ^

src/app/util/clipboard.cpp:509: clone 1 starts here

return; ^

aseprite-bot · 2024-05-02T21:33:07Z

src/app/util/clipboard.cpp

+      else if (!isImageAvailable())
        return;


warning: do not use 'else' after 'return' [readability-else-after-return]

Suggested change

else if (!isImageAvailable())

return;

if (!isImageAvailable())

return;

aseprite-bot

clang-tidy made some suggestions

aseprite-bot · 2024-05-03T16:35:23Z

src/app/util/clipboard.cpp

@@ -487,7 +504,9 @@ void Clipboard::paste(Context* ctx,

    case ClipboardFormat::Image: {
      // Get the image from the native clipboard.
-      if (!getImage(nullptr))
+      if (Editor::activeEditor() && !getImage(nullptr))


warning: implicit conversion 'Editor *' -> bool [readability-implicit-bool-conversion]

Suggested change

if (Editor::activeEditor() && !getImage(nullptr))

if ((Editor::activeEditor() != nullptr) && !getImage(nullptr))

… in --batch mode (fix aseprite#4354)

Gasparoken · 2024-05-03T17:42:08Z

Ready for advice.

dacap

Some comments to complete the implementation.

dacap · 2024-05-28T17:30:45Z

src/app/commands/cmd_cut.cpp

+class Clipboard;
+


This is not needed because we are already including "app/util/clipboard.h".

dacap · 2024-05-28T17:32:17Z

src/app/commands/cmd_cut.cpp

@@ -30,12 +37,18 @@ CutCommand::CutCommand()

 bool CutCommand::onEnabled(Context* ctx)
 {
-  return App::instance()->inputChain().canCut(ctx);
+  return App::instance()->isGui() ?


Use ctx->isUIAvailable() when possible:

Suggested change

return App::instance()->isGui() ?

return ctx->isUIAvailable() ?

dacap · 2024-05-28T17:33:06Z

src/app/commands/cmd_cut.cpp

@@ -30,12 +37,18 @@ CutCommand::CutCommand()

 bool CutCommand::onEnabled(Context* ctx)
 {
-  return App::instance()->inputChain().canCut(ctx);
+  return App::instance()->isGui() ?
+    App::instance()->inputChain().canCut(ctx) : true;


Is this true? Or should we check that ctx->clipboard()? Should Context offer a default input chain?

dacap · 2024-05-28T17:33:50Z

src/app/commands/cmd_paste.cpp

+    App::instance()->inputChain().canPaste(ctx) :
+    ctx->clipboard()->isImageAvailable();


Same case as in cut, probably we can implement a default input chain for batch processing.

dacap · 2024-05-28T17:36:54Z

src/app/util/clipboard.cpp

@@ -290,7 +290,7 @@ bool Clipboard::copyFromDocument(const Site& site, bool merged)
    (mask ? new Mask(*mask): nullptr),
    (pal ? new Palette(*pal): nullptr),
    nullptr,
-    true,                       // set native clipboard
+    App::instance()->isGui() ? true : false, // set native clipboard


I think we can just leave the true as it was (probably some batch scripts will want to copy some content to the native clipboard).

dacap · 2024-05-28T17:37:54Z

src/app/util/clipboard.cpp

+      if (App::instance()->isGui() && !getImage(nullptr))
+        return;


I think this change is not needed (?). Again a script could paste the native clipboard content if possible.

dacap · 2024-05-28T17:38:56Z

src/app/util/clipboard.cpp

+bool Clipboard::isImageAvailable() const
+{
+  return m_data->image &&
+    m_data->image->width() > 0 &&
+    m_data->image->height() > 0;
+}
+


Probably this should go to some CliClipboardManager (which could implement an InputChain and be the default one) to handle all the cut/paste/copy logic for scripts (when UI and editors are not available). Not sure but I think it might be a possible design.

dacap · 2024-05-28T17:40:34Z

src/app/util/clipboard.cpp

+    if (!App::instance()->isGui())
+      return;
 #ifdef ENABLE_UI
    update_screen_for_document(writer.document());
 #endif


Suggested change

if (!App::instance()->isGui())

return;

#ifdef ENABLE_UI

update_screen_for_document(writer.document());

#endif

#ifdef ENABLE_UI

if (writer.context()->isUIAvailable())

update_screen_for_document(writer.document());

#endif

dacap · 2024-05-28T17:45:10Z

src/app/commands/cmd_paste.cpp

-  App::instance()->inputChain().paste(ctx);
+  if (ctx->isUIAvailable())
+    App::instance()->inputChain().paste(ctx);
+  else if (ctx->clipboard())
+    ctx->clipboard()->paste(ctx, false);


Probably we required a origin argument for the paste (at least initially) to paste the image in any position:

struct PasteParams : public NewParams { Param<gfx::Point> origin { this, gfx::Point(), "origin" }; }; class PasteCommand : public CommandWithNewParams<PasteParams> { public: PasteCommand(); ...

dacap · 2024-05-28T17:46:41Z

src/app/util/clipboard.cpp

+      if (!App::instance()->isGui()) {
+        if (const Doc* doc = static_cast<const Doc*>(site.document())) {
+          if (doc->sprite()) {
+            const Mask* mask = doc->mask();
+            const Palette* pal = doc->sprite()->palette(site.frame());
+            doc::Image* image(new_image_from_mask(site, true));
+            setData(image,
+                    (mask ? new Mask(*mask): nullptr),
+                    (pal ? new Palette(*pal): nullptr),
+                    nullptr,
+                    false,
+                    site.layer() && !site.layer()->isBackground());
+          }
+        }
+      }


Why is this required? Didn't copyFromDocument() make the copy?

Gasparoken assigned dacap May 2, 2024

Gasparoken requested a review from dacap as a code owner May 2, 2024 21:29

aseprite-bot reviewed May 2, 2024

View reviewed changes

Gasparoken assigned Gasparoken and unassigned dacap May 2, 2024

Gasparoken force-pushed the fix-cut-paste-script branch from 4d1c93f to 10c388f Compare May 3, 2024 16:31

aseprite-bot reviewed May 3, 2024

View reviewed changes

Gasparoken force-pushed the fix-cut-paste-script branch from 10c388f to ec043ec Compare May 3, 2024 16:48

Fix scripting app.command.Cut() and app.command.Paste() does not work…

a3b0c26

… in --batch mode (fix aseprite#4354)

Gasparoken force-pushed the fix-cut-paste-script branch from ec043ec to a3b0c26 Compare May 3, 2024 17:17

Gasparoken assigned dacap and unassigned Gasparoken May 3, 2024

aseprite deleted a comment from aseprite-bot May 28, 2024

dacap requested changes May 28, 2024

View reviewed changes

dacap assigned Gasparoken and unassigned dacap May 28, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix scripting app.command.Cut() and app.command.Paste() does not work in --batch mode (fix #4354) #4446

Fix scripting app.command.Cut() and app.command.Paste() does not work in --batch mode (fix #4354) #4446

Gasparoken commented May 2, 2024

aseprite-bot left a comment

aseprite-bot May 2, 2024

dacap May 28, 2024

aseprite-bot May 2, 2024

aseprite-bot May 2, 2024

aseprite-bot May 2, 2024

aseprite-bot left a comment

aseprite-bot May 3, 2024

Gasparoken commented May 3, 2024

dacap left a comment

dacap May 28, 2024

dacap May 28, 2024

dacap May 28, 2024

dacap May 28, 2024

dacap May 28, 2024

dacap May 28, 2024

dacap May 28, 2024

dacap May 28, 2024

dacap May 28, 2024

dacap May 28, 2024

	App::instance()->isGui() ? true : false, // set native clipboard
	App::instance()->isGui(), // set native clipboard

	if (editor && !getImage(nullptr))
	if ((editor != nullptr) && !getImage(nullptr))

	if (Editor::activeEditor() && !getImage(nullptr))
	if ((Editor::activeEditor() != nullptr) && !getImage(nullptr))

	return App::instance()->isGui() ?
	return ctx->isUIAvailable() ?

		App::instance()->inputChain().canPaste(ctx) :
		ctx->clipboard()->isImageAvailable();

Fix scripting app.command.Cut() and app.command.Paste() does not work in --batch mode (fix #4354) #4446

Are you sure you want to change the base?

Fix scripting app.command.Cut() and app.command.Paste() does not work in --batch mode (fix #4354) #4446

Conversation

Gasparoken commented May 2, 2024

aseprite-bot left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aseprite-bot left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Gasparoken commented May 3, 2024

dacap left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment