https://bugs.kde.org/show_bug.cgi?id=346768
--- Comment #60 from John Stanley <[email protected]> --- (In reply to Alexey Chernov from comment #56) > I've just applied and quite intensively tested two patches John attached. > > At first for patches: I've actually looked through them before applying, and > there're a couple of questions: > > 1. I've modified kxmlgui patch a little — at first I've replaces close event > stuff for calling queryClose() method. Following XSM protocol one is not > allowed to close windows on the saving state as a) this is not necessary > shutdown that save-yourself is triggered , it is explicitly stated in the > documentation, and b) shutdown, if any, could normally be cancelled and > session would continue, and closing some windows would just spoil this > user's session. Agreed. But, the close event here is actually "fake", no windows are actually closed. In tracing queryClose(), it seemed appropriate to hook KMainWindow::closeEvent() in KMainWindow::commitData() to pick up the "auto save" functionality in KMainWindow::closeEvent(). KMainWindow::closeEvent() potentially does some KMainWindow-specific auto saving, and then simply calls queryClose(). Anyway, I'm happy to go with your call on this, but have a look at KMainWindow::closeEvent() if you haven't already. > > Another thing I removed is restore() method which is obviously the same as > before, but always returns false. Is there any reason for it? > The KMainWindow::restore() currently always returns false, which is not useful for apps that choose to iterate over windows for multi-window restorations (using a false return code terminate iteration). My change is to return true on successful restoration, and false otherwise. > 2. KSMServer patch is what I first have tried not to apply at all. Except > the first edit, which seems to be right as it's just follows the comment, > other stuff seems to be redundant. Could you please explain you ideas behind > these changes? Anyway, everything works even without it (see below). > Yup, the first edit is the only important one. The others, well, I was tinkering a bit, and thinking of doing more, but its a bit time-consuming; I should have removed them before posting. The final two edits are merely code restructuring, no functional changes, should've removed 'em. The remaining edits, associated with "Phase 2 for non-WM clients", are an attempt to allow non-WM apps which want to do their shutdown processing in Phase 2 to do so before the WM is shutdown (also in Phase 2). I don't specifically know of any such apps, but, in principle there could be. > As for testing, I've tested the following use cases: > > 1. Plain saving session. It's generally works even without any patches > (except Qt patch https://codereview.qt-project.org/#/c/140115 I've applied > much earlier). Works fine. > > 2. Saving session with several "modified" clients — in my case it was KWrite > with unsaved document and Konsole with "vim" running in one of the tabs. On > logging out I was gently and sequencially asked for both KWrite and Konsole. > Saved nothing and logged out. Works fine. > > 3. Triggering session save with several "modified" clients and cancel > logging out in one of them. In my case they were KWrite again with an > unsaved document and Kate with an unsaved document. Here I tried two > sub-clauses: > > a) at first, say "don't save" to KWrite and "Cancel" to Kate. Session > continued successfully, no data is lost in both programs. Just KWin moved > both of them to "Any virtual desktop" — it's apparently the default > behaviour, appropriate for now. So works fine; > > b) finally, say "save" to KWrite and "don't save" to Kate. Session > finished and then restored successfully except Kate crashed, which is > apparently separate bug in Kate, which I will address, too. In terms of SM > works fine. > > As a conclusion, I think, this patch can be candidate for merge right now > without even waiting for Qt patch — there's no harm in saving documents > beforehand, but, of course, without Qt changes of this report: > https://codereview.qt-project.org/#/c/140115 applications won't be restarted. > > I'm attaching the patch with my modification. If there's no objection, John, > I'd suggest you as original author to post it to https://reviewboard.kde.org > so that we can have it reviewed and potentially merged as soon as possible. Thanks again for the help and interest and I'll get on the https://reviewboard.kde.org post asap. -- You are receiving this mail because: You are watching all bug changes.
