https://bugs.kde.org/show_bug.cgi?id=346768
--- Comment #62 from John Stanley <[email protected]> --- (In reply to Alexey Chernov from comment #61) > (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. > Yea, but, in this case we are instigating the CloseEvent (not Qt via close()/quit() for example), and so as I understand it, no actual close will be triggered by event acceptance here. That is, I thought that an actual close is done by Qt in, e.g., the close() method after Qt does a sendEvent() and checks for acceptance Here, we only use sendEvent() to decide whether or not to cancel the session > 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.
