https://bugs.kde.org/show_bug.cgi?id=346768
--- Comment #61 from Alexey Chernov <4er...@gmail.com> --- (In reply to John Stanley from comment #60) > (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. Yeah, I've looked at it just briefly applying the patch and now have looked closer. As far as I can see, it still can close window as soon as queryClose() returns true (as it accepts the event). But you're right that there's some code of auto-saving, yes. Another problem arises in that basically the client should ask session manager a permission to interact with the user to get the monopoly attention, but it's not so, as queryClose doesn't have any clue of session manager, it just shows the dialog. But still such things are minor, we need to bring back session restore first. > > 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. Oh, yes, didn't notice, thanks for pointing out. > > 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. OK. > > 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. Yes, thank you. -- You are receiving this mail because: You are watching all bug changes.
