Added option to kill the activity

[ezequielpereira]

It is now possible to kill an activity from its handler on the frame.

This is a GCI 2015 task: https://codein.withgoogle.com/tasks/4832285018816512/

[FGrose]

Here is a direct link to the bug report, https://bugs.sugarlabs.org/ticket/1646

[tchx84]

Nice! I think it would be better to add an alert though, informing the user the risks of closing the activity in this way, with an option to cancel.

Please add it :)

[nemesiscodex]

I don't know if it is something to be worry about but I tried this with Simcity, and it killed my sugar instance, although It works with normal activities
(the last one is simcity)

[ezequielpereira]

It seems that when it isn't an activity, the kill option tries to kill with PID 0 ("All processes in the current process group are signaled").

[ezequielpereira]

Fixed, at least part of it, it doesn't show the option to kill in non-activities.

[quozl]

Thanks for the patch. I've reviewed it and have comments;

• a design question; does kill sound like erase? It may be misunderstood? Should the term be "Force Stop" instead?
• don't like "9", please use signal.SIGKILL.
• don't like SIGKILL, as it gives no chance for the activity to release resources external to the process. Please instead send a signal.SIGTERM, add a GObject timeout for five seconds, and in the timeout callback send signal.SIGKILL, ignore any exception, and remember to return False.
• don't like how the PID is not validated before asking user to confirm. In __kill_activate_cb please instead send signal 0 to verify the process still exists, and if it does not then handle the situation gracefully.

Please force push once you're ready.

[ezequielpereira]

• Changed "Kill" to "Force Stop"
• Changed "9" to "signal.SIGKILL"
• Changed instant kill to "_home_activity.stop()" with a 5 seconds timeout before killing it (I didn't use SIGTERM because it abruptly closed the activity)
• Validate PID before asking user to confirm, so it doesn't show the alert in the case the process do not exists (Maybe the activity stopped when the user was clicking "Force Stop")

[quozl]

Thanks.

• your use of signal.SIG_DFL should be 0 (zero); while SIG_DFL happens to be zero, SIG_DFL is intended for use in signal() or getsignal(); so your code elicits cognitive dissonance when viewed by an experienced programmer of signals, 😁
• you are still sending SIGKILL without first sending SIGTERM; external resources not released; while your change to use .stop() is good, the next step should be a delay, then SIGTERM, then a delay, and finally SIGKILL.
• the delays are a matter of discussion, and while I had suggested five seconds, your new two step sequence may need different delays; and there is no feedback; so perhaps two seconds each delay.

[ezequielpereira]

• Changed "signal.SIG_DFL" to 0
• First timeout (2 seconds) callback sends SIGTERM, second timeout (4 seconds) sends SIGKILL

[quozl]

Sorry I didn't notice it before, but

• if .stop() throws an exception the signals won't be sent; is that what you wanted?

[ezequielpereira]

No, that is not something I want. I'll fix it.

[ezequielpereira]

Fixed ;).

[quozl]

Thanks. Good work. I'm finished with my review. I'll let others review now.

[i5o]

I don't like the Kill option in the menu, I feel like there is a lot of things there already..
I don't know if it's possible to do, but I would suggest a submenu in Stop, like the 'resume with' in the journal

[samdroid-apps]

Please no more submenues. Submenues use the Gtk.Menu based code, so you
don't get correct borders, etc rendered.

On Mon, Dec 14, 2015, 2:49 PM Ignacio Rodríguez notifications@github.com
wrote:

I don't like the Kill option in the menu, I feel like there is a lot of
things there already..
I don't know if it's possible to do, but I would suggest a submenu in
Stop, like the 'resume with' in the journal
[image: 2015-12-14-004724_1280x1024_scrot]
https://cloud.githubusercontent.com/assets/2650479/11772567/6b4d876c-a1fc-11e5-8f87-0571af16f81a.png

—
Reply to this email directly or view it on GitHub
#626 (comment).

[i5o]

Well, It was just a proposal.

I think showing stop, and force stop at same time is weird to user.. Or
idk..

Maybe show it if stop fails..

Just ideas
El 14/12/2015 2:00, "Sam" notifications@github.com escribió:

Please no more submenues. Submenues use the Gtk.Menu based code, so you
don't get correct borders, etc rendered.

On Mon, Dec 14, 2015, 2:49 PM Ignacio Rodríguez notifications@github.com
wrote:

I don't like the Kill option in the menu, I feel like there is a lot of
things there already..
I don't know if it's possible to do, but I would suggest a submenu in
Stop, like the 'resume with' in the journal
[image: 2015-12-14-004724_1280x1024_scrot]
<
https://cloud.githubusercontent.com/assets/2650479/11772567/6b4d876c-a1fc-11e5-8f87-0571af16f81a.png

—
Reply to this email directly or view it on GitHub
#626 (comment).

—
Reply to this email directly or view it on GitHub
#626 (comment).

[tchx84]

Hmm, regarding the design part, there doesn't seem to be a consensus...

What about something like this? (similar to what @i5o mentioned)

1. we don't add another button.
2. once the (already existing) stop button is pressed on the frame menu, we launch a timer (with some time threshold we decide)
3. IF the activity is closed before the threshold, then we remove the timer and will be as nothing happened.
4. BUT if the activity hasn't stop by then, we can we show that same alert with the option to kill?

This way we don't add more sub-menues and we still provide a way force stop?

@quozl @nemesiscodex @i5o @samdroid-apps opinions?

[quozl]

@tchx84, yes, that would also be acceptable to me. @samdroid-apps, @i5go?

(@163gal, since you're main contributor of this design change, can you please tell me if you are willing to do what our maintainer @tchx84 has suggested once consensus is reached? I'd like a force stop feature in my OLPC fork, and have already merged and tested 440883f, and am happy to revert.)

[ezequielpereira]

@quozl, of course.

[i5o]

The design idea sounds great for me.

+1

Ignacio Rodríguez
SugarLabs at Facebook
https://www.facebook.com/pages/SugarLabs/187845102582

2015-12-22 20:04 GMT-03:00 Ezequiel Pereira notifications@github.com:

@quozl https://github.com/quozl, of course.

—
Reply to this email directly or view it on GitHub
#626 (comment).

[godiard]

Show a alert is complicated. Where should be displayed? Maybe add a notification?

[samdroid-apps]

Since adding an alert is hard, maybe we use @tchx84's design inside a palette:

1. User clicks stop
2. Changes to stopping...
3. palette popsdown if the activity responds (eg. keep error) or if it quits
4. if 3 secs has elapsed, it is replaced with "force stop" in bold, and with a warning icon

[godiard]

@samdroid-apps, when the user click stop, the pallete will be closed (that is how palettes work). I am not sure in this case, but in general, in sugar, the palettes are created every time that are displayed. After N seconds, if the activity don't close, you can set a flag, and the palette can add the option "force stop" the next time that is opened. Probably will not be very intuitive to the user, but is a solution to a corner case. Other than that, I think 3 seconds is too short for hardware like the XO-1

[samdroid-apps]

+1 @godiard sounds good

[samdroid-apps]

I am closing old pull requests today to take the out of the review queue. Please feel free to re-open it and update the patch as per the discussion.