{"action":"created","issue":{"url":"https://api.github.com/repos/Subsurface-divelog/subsurface/issues/1380","repository_url":"https://api.github.com/repos/Subsurface-divelog/subsurface","labels_url":"https://api.github.com/repos/Subsurface-divelog/subsurface/issues/1380/labels{/name}","comments_url":"https://api.github.com/repos/Subsurface-divelog/subsurface/issues/1380/comments","events_url":"https://api.github.com/repos/Subsurface-divelog/subsurface/issues/1380/events","html_url":"https://github.com/Subsurface-divelog/subsurface/pull/1380","id":332879922,"node_id":"MDExOlB1bGxSZXF1ZXN0MTk1MjU2NTAy","number":1380,"title":"Cleanup: Don't clear WA_QuitOnClose attribute on dialogs","user":{"login":"bstoeger","id":32835590,"node_id":"MDQ6VXNlcjMyODM1NTkw","avatar_url":"https://avatars3.githubusercontent.com/u/32835590?v=4","gravatar_id":"","url":"https://api.github.com/users/bstoeger","html_url":"https://github.com/bstoeger","followers_url":"https://api.github.com/users/bstoeger/followers","following_url":"https://api.github.com/users/bstoeger/following{/other_user}","gists_url":"https://api.github.com/users/bstoeger/gists{/gist_id}","starred_url":"https://api.github.com/users/bstoeger/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/bstoeger/subscriptions","organizations_url":"https://api.github.com/users/bstoeger/orgs","repos_url":"https://api.github.com/users/bstoeger/repos","events_url":"https://api.github.com/users/bstoeger/events{/privacy}","received_events_url":"https://api.github.com/users/bstoeger/received_events","type":"User","site_admin":false},"labels":[],"state":"open","locked":false,"assignee":{"login":"neolit123","id":331852,"node_id":"MDQ6VXNlcjMzMTg1Mg==","avatar_url":"https://avatars3.githubusercontent.com/u/331852?v=4","gravatar_id":"","url":"https://api.github.com/users/neolit123","html_url":"https://github.com/neolit123","followers_url":"https://api.github.com/users/neolit123/followers","following_url":"https://api.github.com/users/neolit123/following{/other_user}","gists_url":"https://api.github.com/users/neolit123/gists{/gist_id}","starred_url":"https://api.github.com/users/neolit123/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/neolit123/subscriptions","organizations_url":"https://api.github.com/users/neolit123/orgs","repos_url":"https://api.github.com/users/neolit123/repos","events_url":"https://api.github.com/users/neolit123/events{/privacy}","received_events_url":"https://api.github.com/users/neolit123/received_events","type":"User","site_admin":false},"assignees":[{"login":"neolit123","id":331852,"node_id":"MDQ6VXNlcjMzMTg1Mg==","avatar_url":"https://avatars3.githubusercontent.com/u/331852?v=4","gravatar_id":"","url":"https://api.github.com/users/neolit123","html_url":"https://github.com/neolit123","followers_url":"https://api.github.com/users/neolit123/followers","following_url":"https://api.github.com/users/neolit123/following{/other_user}","gists_url":"https://api.github.com/users/neolit123/gists{/gist_id}","starred_url":"https://api.github.com/users/neolit123/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/neolit123/subscriptions","organizations_url":"https://api.github.com/users/neolit123/orgs","repos_url":"https://api.github.com/users/neolit123/repos","events_url":"https://api.github.com/users/neolit123/events{/privacy}","received_events_url":"https://api.github.com/users/neolit123/received_events","type":"User","site_admin":false}],"milestone":null,"comments":7,"created_at":"2018-06-15T18:52:26Z","updated_at":"2018-06-16T07:48:26Z","closed_at":null,"author_association":"CONTRIBUTOR","pull_request":{"url":"https://api.github.com/repos/Subsurface-divelog/subsurface/pulls/1380","html_url":"https://github.com/Subsurface-divelog/subsurface/pull/1380","diff_url":"https://github.com/Subsurface-divelog/subsurface/pull/1380.diff","patch_url":"https://github.com/Subsurface-divelog/subsurface/pull/1380.patch"},"body":"According to Qt's documentation, the application exits if all windows\r\nwith the WA_QuitOnClose attribute are closed. This attribute was cleared\r\nfor three dialogs. This seems not necessary because:\r\n1) The application can't be closed as long as the modal dialog is shown.\r\n2) The flag only concerns primary windows, which these are not.\r\n\r\nSee: http://doc.qt.io/qt-5/qguiapplication.html#quitOnLastWindowClosed-prop\r\n\r\nSigned-off-by: Berthold Stoeger <bstoeger@mail.tuwien.ac.at>\r\n\r\n<!-- Lines like this one are comments and will not be shown in the final output. -->\r\n<!-- Make sure that you have read the \"Contributing\" section of the README and also the notes in CodingStyle. -->\r\n<!-- If you are a collaborator, please add labels and assign other collaborators for a review. -->\r\n\r\n### Describe the pull request:\r\n<!-- Replace [ ] with [x] to select options. -->\r\n- [ ] Bug fix\r\n- [ ] Functional change\r\n- [ ] New feature\r\n- [x] Code cleanup\r\n- [ ] Build system change\r\n- [ ] Documentation change\r\n- [ ] Language translation\r\n\r\n### Pull request long description:\r\n<!-- Describe your pull request in detail. -->\r\nThis removes the clearing of the `WA_QuitOnClose` flag for three dialogs. To my understanding, this it not necessary. The flag is not relevant, because the dialogs have a parent and you can't close the application anyway while they are shown.\r\n\r\nThis was introduced in 880b8394d2b063c1bbd6597c2cb6c51f0a1d66c1 which has a strange commit message:\r\n> To solve that we mark all custom windows/dialogs with the Qt::WA_QuitOnClose attribute on instance creation.\r\n\r\nBut actually the commit *clears* the flag.\r\n\r\n> Most child windows should be closed with the main application window otherwise if left open and if making specific modifictions could potentially cause a SIGSEGV.\r\n\r\nBut - at least according to the docs - the flag in question does the *opposite*: It closes the application if the marked windows are closed, not vice-versa.\r\n\r\n### Release note:\r\n\r\nNot needed.\r\n\r\n### Documentation change:\r\n\r\nNot needed.\r\n\r\n### Mentions:\r\n<!-- Mention users that you want to review your pull request with @<user-name>. Leave empty if not sure. -->\r\n@neolit123: Original commit is by you. Please check"},"comment":{"url":"https://api.github.com/repos/Subsurface-divelog/subsurface/issues/comments/397795034","html_url":"https://github.com/Subsurface-divelog/subsurface/pull/1380#issuecomment-397795034","issue_url":"https://api.github.com/repos/Subsurface-divelog/subsurface/issues/1380","id":397795034,"node_id":"MDEyOklzc3VlQ29tbWVudDM5Nzc5NTAzNA==","user":{"login":"bstoeger","id":32835590,"node_id":"MDQ6VXNlcjMyODM1NTkw","avatar_url":"https://avatars3.githubusercontent.com/u/32835590?v=4","gravatar_id":"","url":"https://api.github.com/users/bstoeger","html_url":"https://github.com/bstoeger","followers_url":"https://api.github.com/users/bstoeger/followers","following_url":"https://api.github.com/users/bstoeger/following{/other_user}","gists_url":"https://api.github.com/users/bstoeger/gists{/gist_id}","starred_url":"https://api.github.com/users/bstoeger/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/bstoeger/subscriptions","organizations_url":"https://api.github.com/users/bstoeger/orgs","repos_url":"https://api.github.com/users/bstoeger/repos","events_url":"https://api.github.com/users/bstoeger/events{/privacy}","received_events_url":"https://api.github.com/users/bstoeger/received_events","type":"User","site_admin":false},"created_at":"2018-06-16T07:48:26Z","updated_at":"2018-06-16T07:48:26Z","author_association":"CONTRIBUTOR","body":"> i see that your argument is that the fix wasn't a fix?\r\n\r\nThat's not my argument. I can't tell what, if anything, the commit from 2013 fixed. I doubt that it's worth finding out.\r\n\r\nMy argument is that in the current code base this doesn't do anything. Especially, the failure case you describe (dialog calling into destructed `MainWindow`) can never happen, because the dialogs in question are destroyed with `MainWindow`. If this really was the issue, then it has been fixed properly.\r\n\r\nIf the failure case was that the application doesn't close when there is an open dialog - according to my quick glance of Qt's source this shouldn't happen anymore.\r\n\r\n> instead of that why not test your patch on top of the oldest Qt version we support on Linux (5.5)\r\n\r\nI don't have a Qt 5.5 and compiling from source on my laptop is pretty much out of the question. Travis doesn't seem to generate the appropriate binaries. Moreover, I don't even know what I should test, as the described failure mode can not happen.\r\n\r\n> I use Qt 5.10 on my mac, and will give it a spin this weekend.\r\n\r\nThanks, I can imagine this was some Mac issue, as Macs behave quite differently when it comes to window handling."}}
{"actor":{"display_login":"bstoeger"}}