Skip to content

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.

@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! :)

@rqdmap rqdmap requested a review from phisch May 4, 2023 13:14
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