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 signal handler #4

Closed
elfring opened this issue Sep 19, 2015 · 9 comments
Closed

Fix signal handler #4

elfring opened this issue Sep 19, 2015 · 9 comments

Comments

@elfring
Copy link

elfring commented Sep 19, 2015

  1. The data type "sig_atomic_t" should be reused for the variable "stop".
    • How do you think about to assign the value which will be passed to the function "intHandler" to it?
    • Should this variable checked more often so that the program can eventually be aborted quicker?
  2. Can the assignment "loops = loop" be deleted?
@posva
Copy link
Owner

posva commented Sep 19, 2015

I intentionally check it after each loop so an image doesn't get half printed
I'm afraid your knowledge is way higher than mine and that I cannot see what will this bring to catimg. I really appreciate it as I'm learning many interesting things but I cannot see any any benefit from using the sig_atomic_t. To me this seems a bit overkill.
However if you change the code, create a PR and prove the code have been improved in efficiency or in error handling, I'll merge it.

I'm going to add an option for the number of loops so that line will be needed

@posva posva closed this as completed Sep 19, 2015
@elfring
Copy link
Author

elfring commented Sep 19, 2015

… I cannot see any any benefit from using the sig_atomic_t.

The reuse of this data type is one of the preferred ways for the portable data exchange with your signal handler. How do you think about to improve software portability a bit here?

An other software design option would be to delegate signal handling or the image file processing to another thread, wouldn't it?

@posva
Copy link
Owner

posva commented Sep 19, 2015

ok for the portability. Having multiple threads for catimg just to handle the sigterm or any other signal is overkill, don't you think 😅

@elfring
Copy link
Author

elfring commented Sep 20, 2015

Having multiple threads for catimg just to handle the sigterm or any other signal is overkill, …

It depends on user expectations for your software application. In which time frame would you like to see the appropriate response for the event "SIGINT"?

@posva
Copy link
Owner

posva commented Sep 20, 2015

It depends on user expectations for your software application

Well, nobody complained about it yet.

In which time frame would you like to see the appropriate response for the event "SIGINT"?

At the end of an image/frame for a GIF and anytime for a normal image. Although this may vary on the terminal size. I guess I'll etablish a pixel count. I'll run some test and call the handler after that many pixels are displayed

@elfring
Copy link
Author

elfring commented Sep 20, 2015

I guess I'll etablish a pixel count.

Is such an approach really needed?
Will an immediate reaction be usually more appropriate?

@posva
Copy link
Owner

posva commented Sep 20, 2015

nope, as I said in the first comment

@elfring
Copy link
Author

elfring commented Sep 20, 2015

I assume that there are users who will not like delayed signal handling.

Can the portable data type "sig_atomic_t" be applied at least?

@posva
Copy link
Owner

posva commented Sep 21, 2015

I'll see, thanks 😄

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

No branches or pull requests

2 participants