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

Make giph stop all previous recordings properly #26

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

rqdmap
Copy link

@rqdmap rqdmap commented Apr 18, 2023

I'm new to giph and I love the tiny and elegant program very much! But sometimes I find giph --stop not working properly. After investigating, I find two main problems:

  1. "$(pgrep -f "bash.+giph")" will return all pids as a single string. So after starting multiple giph records, giph --stop can only stop at most one previous record.
  2. Currently when executing giph --stop, It will send SIGINT to all running giph processes as well as their child processes. This has caused me some trouble because I'm using keyboard shortcut and when I accidently executed this command twice, the encode() bash function will be interrupted. So I will lose all the screen records and only get an empty gif file. I modified stop_all_recordings in order to kill giph's record child process accurately.

@phisch
Copy link
Owner

phisch commented Apr 26, 2023

Hey @rqdmap!

Stopping all recordings reliable has been an issue in many different setups, every time someone thinks they have the solution, someone else comes up reporting it doesn't work on their system.

That out of the way, I'm currently not using an x11 environment, which makes this impossible for me to test, so I have to gain confidence in this change another way, but I don't quite understand the whole change. So let me ask some questions in a review to fully understand it.

Copy link
Owner

@phisch phisch left a comment

Choose a reason for hiding this comment

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

Please use your implementation to replace the current --stop feature instead of introducing a new flag. If you want to somewhat keep the old behavior, put it behind a --kill flag that would just hard kill all running recordings.

src/giph Outdated Show resolved Hide resolved
src/giph Outdated Show resolved Hide resolved
src/giph Outdated Show resolved Hide resolved
@phisch
Copy link
Owner

phisch commented Apr 26, 2023

Please take your time with the changes, no rush. I hope to create a new release when this and #23 are both merged, and I'm not sure if I'll get an answer on #23 any time soon as I haven't given feedback on it in a very long time.

@rqdmap
Copy link
Author

rqdmap commented Apr 27, 2023

That's great. Looking forward to the new release! :)

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.

None yet

2 participants