[prev in list] [next in list] [prev in thread] [next in thread] 

List:       kde-bugs-dist
Subject:    [frameworks-kxmlgui] [Bug 346768] After login only one tab is present, when several tabs where opene
From:       John Stanley via KDE Bugzilla <bugzilla_noreply () kde ! org>
Date:       2016-02-07 23:10:10
Message-ID: bug-346768-17878-hcEDj3BoJA () http ! bugs ! kde ! org/
[Download RAW message or body]

https://bugs.kde.org/show_bug.cgi?id=346768

--- Comment #64 from John Stanley <jpsinthemix@gmail.com> ---
(In reply to Alexey Chernov from comment #63)
> (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?

I did do a quick 'n dirty check with konsole (had to patch konsole's
MainWindow::queryClose() to allow session cancellation though): open several
apps plus konsole, launch vi in konsole, attempt to logout, refuse, and it
works as expected -- session is canceled and windows remain open,

> 
> > > 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.=
[prev in list] [next in list] [prev in thread] [next in thread]