https://bugs.kde.org/show_bug.cgi?id=346768
--- Comment #63 from Alexey Chernov <[email protected]> --- (In reply to John Stanley from comment #62) > (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 Yes, you're probably right then, since we send close event by ourselves. I think, it's quite easy to test this as well — just left one client untouched and another one with modifications before logging out, cancel the shutdown in the latter and see what happens. I believe, you have tried the initial version of the patch, did you happen to follow this scenario? > > 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.
