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

Fix #697: a crash in the Watermark plugin if screenshot name includes spaces #698

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Photon89
Copy link
Member

@Photon89 Photon89 commented Sep 7, 2024

No description provided.

@Photon89 Photon89 linked an issue Sep 7, 2024 that may be closed by this pull request
@Photon89
Copy link
Member Author

Photon89 commented Sep 7, 2024

Looks like I introduced the issue in #380...

@@ -362,7 +362,7 @@ sub apply_effect {

my $command2 = "composite " . $tmpfilename .
" -gravity " . $gravity_combo->get_active_text .
" " . $filename . " " . $tmpfilename2;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

better not to use backtick at all, but to use some other ways to run subprocess. Or at least escape filesnames and such.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, with a system call it resulted in an error which I couldn't understand (here is my question about it: https://stackoverflow.com/questions/68628648/the-and-comma-operators-in-a-perl-system-call).

What else would you propose instead of backticks?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I just revert your previous change, the system call works fine for me. What was the issue?

Copy link
Member Author

@Photon89 Photon89 Sep 7, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It crashed when clicking on "refresh" or "apply" three years ago, see #379.

I can actually still reproduce it. The plugin window just disappears and the screenshot stays untouched.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you show logs of this?

it works perfectly for me, if I revert your old commit

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please try #702

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Still crashes as in #379, unfortunately. Note for later, perl-ipc-run3 as new dependency required if we want to pursue this variant.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But is the log any better there why it crashes?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nope, it's still the same...

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

well, I don't know how to debug this further for you.

you can merge this PR, of course, but don't be surprised when the next issue opened will be "crash if the name includes the quote character" or something like that

can you try to write a small script (without any shutter or even gtk) which just runs these 2 commands via backticks / via run3, and see what it says?

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.

BUG: error Watermark plugin
2 participants