-
-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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
Fix scripting app.command.Cut() and app.command.Paste() does not work in --batch mode (fix #4354) #4446
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
clang-tidy made some suggestions
@@ -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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
warning: redundant boolean literal in ternary expression result [readability-simplify-boolean-expr]
App::instance()->isGui() ? true : false, // set native clipboard | |
App::instance()->isGui(), // set native clipboard |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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).
src/app/util/clipboard.cpp
Outdated
@@ -487,7 +504,9 @@ | |||
|
|||
case ClipboardFormat::Image: { | |||
// Get the image from the native clipboard. | |||
if (!getImage(nullptr)) | |||
if (editor && !getImage(nullptr)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
warning: implicit conversion 'Editor *' -> bool [readability-implicit-bool-conversion]
if (editor && !getImage(nullptr)) | |
if ((editor != nullptr) && !getImage(nullptr)) |
@@ -487,7 +504,9 @@ | |||
|
|||
case ClipboardFormat::Image: { | |||
// Get the image from the native clipboard. | |||
if (!getImage(nullptr)) | |||
if (editor && !getImage(nullptr)) | |||
return; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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;
^
else if (!isImageAvailable()) | ||
return; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
warning: do not use 'else' after 'return' [readability-else-after-return]
else if (!isImageAvailable()) | |
return; | |
if (!isImageAvailable()) | |
return; |
4d1c93f
to
10c388f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
clang-tidy made some suggestions
src/app/util/clipboard.cpp
Outdated
@@ -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)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
warning: implicit conversion 'Editor *' -> bool [readability-implicit-bool-conversion]
if (Editor::activeEditor() && !getImage(nullptr)) | |
if ((Editor::activeEditor() != nullptr) && !getImage(nullptr)) |
10c388f
to
ec043ec
Compare
… in --batch mode (fix aseprite#4354)
ec043ec
to
a3b0c26
Compare
Ready for advice. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some comments to complete the implementation.
class Clipboard; | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not needed because we are already including "app/util/clipboard.h"
.
@@ -30,12 +37,18 @@ CutCommand::CutCommand() | |||
|
|||
bool CutCommand::onEnabled(Context* ctx) | |||
{ | |||
return App::instance()->inputChain().canCut(ctx); | |||
return App::instance()->isGui() ? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use ctx->isUIAvailable()
when possible:
return App::instance()->isGui() ? | |
return ctx->isUIAvailable() ? |
@@ -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; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this true? Or should we check that ctx->clipboard()
? Should Context
offer a default input chain?
App::instance()->inputChain().canPaste(ctx) : | ||
ctx->clipboard()->isImageAvailable(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same case as in cut, probably we can implement a default input chain for batch processing.
@@ -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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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).
if (App::instance()->isGui() && !getImage(nullptr)) | ||
return; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this change is not needed (?). Again a script could paste the native clipboard content if possible.
bool Clipboard::isImageAvailable() const | ||
{ | ||
return m_data->image && | ||
m_data->image->width() > 0 && | ||
m_data->image->height() > 0; | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
if (!App::instance()->isGui()) | ||
return; | ||
#ifdef ENABLE_UI | ||
update_screen_for_document(writer.document()); | ||
#endif |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 |
App::instance()->inputChain().paste(ctx); | ||
if (ctx->isUIAvailable()) | ||
App::instance()->inputChain().paste(ctx); | ||
else if (ctx->clipboard()) | ||
ctx->clipboard()->paste(ctx, false); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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();
...
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()); | ||
} | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this required? Didn't copyFromDocument()
make the copy?
It fixes the original problem, but I need advice on this approach.
fix #4354