Killing window does not exit openambit

[paddy-hack]

To reproduce start openambit with ./run.sh and click the window's x icon (assuming the window manager shows one). No need to connect your clock. The ./run.sh job will happily continue. You need to Ctrl-C to get the command prompt back.

Looks like there is at least one window manager event that isn't handled.

FWIW, I'm running an Xfce4 desktop.

[mbernasocchi]

I think it might be a feature, there is a dock notification icon that appears in unity. from there you can exit the program.

[mbernasocchi]

also if you do menu exit all closes.

[benedetto]

The same here on KDE. So it might be that your xfce desktop does provide a different method to close programs to the dock and that this method is not supported by openambit (just guessing).

[paddy-hack]

I guess I was not being clear. When I click the x icon that the window manager provides (in the upper right corner of the screenshot), the window closes. This is the same behaviour as when you select Exit from the File menu.

What is not the same is the job finishing. I start the application from the command-line, via ./run.sh. When I select Exit, I get my command prompt back. When I click x, I don't. After I hit Ctrl-C I get my command-prompt back. When I then execute ./run.sh again I get

olaf@hack:~/code/openambit$ ./run.sh 
------running openambit------
"QLocalSocket::connectToServer: Connection refused" 

These kind of window manager events are delivered to the application, no matter what GUI toolkit you use. The application just has to bind a callback to the relevant event so that it shuts down cleanly, just like what happens when it gets a "clicked Exit on the File menu" event.

[marguslt]

In case your DM/WM supports systray (or QT believes it does), ignoring close event and instead hiding the window is expected behavior - https://github.com/openambitproject/openambit/blob/master/src/openambit/mainwindow.cpp#L172

[paddy-hack]

The command-line prompt not coming back is most certainly not expected behaviour from my point of view. The application disappearing from view is fine, but I want my command-line prompt back without having to Ctrl-C (which sends a SIGINT to the application that results in a non-clean exit).

[paddy-hack]

FWIW, to get an application to "minimize" (either iconify in a systray or a button on a task bar) I'd click the _ icon. The x icon is supposed to terminate the application.

[svenstorp]

The behavior is per design (good or bad, right or wrong can/should of course still be debated). When implementing it I made openambit behave the same way a few of my other day to day applications behave, for example amarok and skype. Just as @marguslt points out this "minimize to systray"-behavior should only be enabled if a systray is available (Qt-code), but we should probably add an override setting since we obviously have users with and without systray-preferences :)

[battila]

The x is close the window, not the application! By design. If you don't like it, then edit the source you downloaded from the github and modify it. It is easy to add the quit there. On Sep 19, 2014 2:05 PM, "Olaf Meeuwissen" notifications@github.com wrote:

FWIW, to get an application to "minimize" (either iconify in a systray or a button on a task bar) I'd click the _ icon. The x icon is supposed to terminate the application.

— Reply to this email directly or view it on GitHub https://github.com/openambitproject/openambit/issues/60#issuecomment-56168608 .

[mbernasocchi]

@battila although I agree with the fact that it is by design, I'd have a look at the commit history of openambit, before teaching @paddy-hack how to use github...

[paddy-hack]

@svenstorp I've just checked iceweasel (firefox), libreoffice, gimp, evince and wireshark. When starting these from the command-line and clicking the x icon, I get my command prompt back. The icon is just that, an icon. AFAIK, the intended window manager action for that icon is terminate. There is another icon, _ that minimizes the application (see screenshot, third icon from the right - second from right maximizes, fourth one "rolls up" the window so that only the title remains).

@battila It's easy to fix if you know where to look. I haven't done any Qt since 1.3x ;-P @mbernasocchi ;-)

[battila]

The chrome has an option to keep running in the background and it puts an icon to the toolbar.

I will create a patch today.

On 21 Sep 2014, at 02:56, Olaf Meeuwissen notifications@github.com wrote:

@svenstorp I've just checked iceweasel (firefox), libreoffice, gimp, evince and wireshark. When starting these from the command-line and clicking the x icon, I get my command prompt back. The icon is just that, an icon. AFAIK, the intended window manager action for that icon is terminate. There is another icon, _ that minimizes the application (see screenshot, third icon from the right - second from right maximizes, fourth one "rolls up" the window so that only the title remains).

@battila It's easy to fix if you know where to look. I haven't done any Qt since 1.3x ;-P @mbernasocchi ;-)

— Reply to this email directly or view it on GitHub.

[bubulle]

I agree that a setting allowing to choose between the current behavior (staying in the taskbar when there's one) and a "standard app" behavior, with the default being the current behavior, seems the most sensible thing to do

[svenstorp]

@paddy-hack None of the applications you give as an example provide any "docking"/"keep running in the background" functionality, so it is not really a relevant comparison. Also, for at least firefox, the behavior is rather "close the window, and if it was the last open window (not necessary the first window that was opened), also terminate the process".

Anyhow, I have added 2 new issue (#61 and #62) to implement the user option, and also to add proper handling of SIGINT.

[battila]

Hey,

new checkbox in the General settings:

"Continue running in background when OpenAmbit main window is closed"

If set, the windowmanager close button hide the application (just like before) if not set the windowmanager close button will close the application.

Best regards, Attila

On Sun, Sep 21, 2014 at 1:19 PM, Emil Ljungdahl notifications@github.com wrote:

@paddy-hack https://github.com/paddy-hack None of the applications you give as an example provide any "docking"/"keep running in the background" functionality, so it is not really a relevant comparison. Also, for at least firefox, the behavior is rather "close the window, and if it was the last open window (not necessary the first window that was opened), also terminate the process".

Anyhow, I have added 2 new issue (#61 https://github.com/openambitproject/openambit/issues/61 and #62 https://github.com/openambitproject/openambit/issues/62) to implement the user option, and also to add proper handling of SIGINT.

— Reply to this email directly or view it on GitHub https://github.com/openambitproject/openambit/issues/60#issuecomment-56295890 .

[paddy-hack]

@svenstorp Thanks for submitting the new issues, especially #61 @battila Thanks for the quick addition of the option.

I've just tested things and everything seems to work as intended now. I also finally noticed that there was indeed an OpenAmbit icon in my system tray. Sorry for not noticing earlier. One thing that might be useful if a system tray is present, is putting the openambit process in the background when started from a command-line. IIRC, OpenOffice.org did something like that in the past. That'd just be icing on the cake though.

[battila]

You are right, i forget to put it to bg at startup. I will see if i have time today to fix it. On Sep 22, 2014 1:47 AM, "Olaf Meeuwissen" notifications@github.com wrote:

@svenstorp https://github.com/svenstorp Thanks for submitting the new issues, especially #61 https://github.com/openambitproject/openambit/issues/61 @battila https://github.com/battila Thanks for the quick addition of the option.

I've just tested things and everything seems to work as intended now. I also finally noticed that there was indeed an OpenAmbit icon in my system tray. Sorry for not noticing earlier. One thing that might be useful if a system tray is present, is putting the openambit process in the background when started from a command-line. IIRC, OpenOffice.org did something like that in the past. That'd just be icing on the cake though.

— Reply to this email directly or view it on GitHub https://github.com/openambitproject/openambit/issues/60#issuecomment-56317214 .

[battila]

Hey,

yesterday i uploaded the modification to the git. Now the you will get back the prompt immediately during start.

Best regards, Attila On Sep 22, 2014 7:41 AM, attila.bardi@gmail.com wrote:

You are right, i forget to put it to bg at startup. I will see if i have time today to fix it. On Sep 22, 2014 1:47 AM, "Olaf Meeuwissen" notifications@github.com wrote:

@svenstorp https://github.com/svenstorp Thanks for submitting the new issues, especially #61 https://github.com/openambitproject/openambit/issues/61 @battila https://github.com/battila Thanks for the quick addition of the option.

I've just tested things and everything seems to work as intended now. I also finally noticed that there was indeed an OpenAmbit icon in my system tray. Sorry for not noticing earlier. One thing that might be useful if a system tray is present, is putting the openambit process in the background when started from a command-line. IIRC, OpenOffice.org did something like that in the past. That'd just be icing on the cake though.

— Reply to this email directly or view it on GitHub https://github.com/openambitproject/openambit/issues/60#issuecomment-56317214 .

[paddy-hack]

@battila Thanks! Works like a charm ... but I'm not overly happy with the complete lack of any error checking in the code.

[battila]

Hey Olaf,

the code

// Fork for background running
if ( fork() > 0 ) {
    // Exit the parent process
    return 0;
}
// Set the child to the new process group leader
setsid();

We can check two errors here. The first is the fork(). If it returns -1 then the only thing we can do is to inform the user, the app can run without any problem. The second one is the setsid() which goes to failed if the process is already the process group leader, so nothing to do with it.

If you want I can add a warning if the fork is failed.

By the way, fork() is not available on windows, i have the fix for that.

Best regards, Attila

On Tue, Sep 23, 2014 at 1:16 PM, Olaf Meeuwissen notifications@github.com wrote:

@battila https://github.com/battila Thanks! Works like a charm ... but I'm not overly happy with the complete lack of any error checking in the code.

— Reply to this email directly or view it on GitHub https://github.com/openambitproject/openambit/issues/60#issuecomment-56505437 .

[battila]

Preprocessor fix for missing fork() on Windows uploaded.

On Tue, Sep 23, 2014 at 2:14 PM, Attila Bárdi attila.bardi@gmail.com wrote:

Hey Olaf,

the code

// Fork for background running
if ( fork() > 0 ) {
    // Exit the parent process
    return 0;
}
// Set the child to the new process group leader
setsid();

We can check two errors here. The first is the fork(). If it returns -1 then the only thing we can do is to inform the user, the app can run without any problem. The second one is the setsid() which goes to failed if the process is already the process group leader, so nothing to do with it.

If you want I can add a warning if the fork is failed.

By the way, fork() is not available on windows, i have the fix for that.

Best regards, Attila

On Tue, Sep 23, 2014 at 1:16 PM, Olaf Meeuwissen <notifications@github.com

wrote:

@battila https://github.com/battila Thanks! Works like a charm ... but I'm not overly happy with the complete lack of any error checking in the code.

— Reply to this email directly or view it on GitHub https://github.com/openambitproject/openambit/issues/60#issuecomment-56505437 .

[paddy-hack]

I don't care too much for the Windows preprocessor check ;-) There's probably more bits that don't quite port to that platform. Unless we have a buildbot for it, it'd be a major pain to find out what parts of what standards (anyone say POSIX.1-2001?, which has fork() and setsid() BTW) are supported or not.

Back to the error handling, yes, please log the fork() failure as a warning so people can find out why things don't background. As for setsid(), any errors are probably only of interest to developer types, so maybe an informational log message will do. I don't think we have any guidelines on logging but personally I prefer to stay out of the debugger (more so for multi-threaded, child-spawning programs ;-)

BTW, shouldn't you _exit(0) the parent rather than return 0 as per man 2 setsid?

[battila]

It was not my idea to talk about windows and mac porting. I think it is pointless, they have Suunto supported Moveslink. I think we should stay at linux and bsd side.

The BSD man page mention nothing about _exit(0). However in the main() the return 0, exit(0) and in our case the _exit(0) are the bloody same.

Do we have any guideline at all?

On 25 Sep 2014, at 15:01, Olaf Meeuwissen notifications@github.com wrote:

I don't care too much for the Windows preprocessor check ;-) There's probably more bits that don't quite port to that platform. Unless we have a buildbot for it, it'd be a major pain to find out what parts of what standards (anyone say POSIX.1-2001?, which has fork() and setsid() BTW) are supported or not.

Back to the error handling, yes, please log the fork() failure as a warning so people can find out why things don't background. As for setsid(), any errors are probably only of interest to developer types, so maybe an informational log message will do. I don't think we have any guidelines on logging but personally I prefer to stay out of the debugger (more so for multi-threaded, child-spawning programs ;-)

BTW, shouldn't you _exit(0) the parent rather than return 0 as per man 2 setsid?

— Reply to this email directly or view it on GitHub.

[svenstorp]

@battila No it was mine :) Maybe it is just hubris, but at least when we get to implement #25, there is a fair reason to use Openambit on Windows as well. Especially if you are not that interested in movescount itself.

With that said, we should not work our ass off for something that might be, but I think it is a good "direction" to have in mind when implementing new stuff. The good thing, for the UI-part it is not harder than trying to use Qt-libs for as much as possible.

The fork() part is a bit tricky, it has no corresponding functionality on Windows, I think the closest you get with Qt is QProcess. The current preprocessor-checks are just fine, I think we can live with a no-backgrounding Windows-version !

Nope, no guidelines, but maybe it is time to write a few lines! It is just so damn hard to prioritize boring stuff on your spare time :)

[svenstorp]

Start getting out of topic here, but I almost forgot one important motivation for multi-platform support! For all of us complaining about Suunto not having any official Linux-support; The least we can do is to prove them it is not that hard to fix! :)

[svenstorp]

Solved by fork-ing into background on start