When open chromium, konsole, thunderbird, then logout, relogin, but only thunderbird successful WM_SAVE_YOURSELF, chromium or konsole failed to be opened. plasma-workspace: 5.4.90 KF5: 5.15.0 Qt5: 5.5.1 Reproducible: Always Steps to Reproduce: 1. open chromium, konsole, thunderbird, then logout 2. relogin Actual Results: but only thunderbird successful WM_SAVE_YOURSELF Expected Results: chromium and konsole able to be opened.
my ~/.config/ksmserverrc [General] screenCount=1 [LegacySession: saved at previous logout] clientMachine1=localhost command1=thunderbird count=1 [Session: saved at previous logout] clientId1=1014cd7d2d4000144435205700000012260003 clientId2=1014cd7d2d4000144435208500000012260012 clientId3=10bd534f46000144610537700000070460007 count=3 discardCommand1[$e]=rm,$HOME/.config/session/kwin_1014cd7d2d4000144435205700000012260003_1446105607_787343 discardCommand2[$e]=rm,$HOME/.config/session/kmix_1014cd7d2d4000144435208500000012260012_1446105607_737385 program1=kwin_x11 program2=/bin/kmix program3=/usr/lib/mozilla/kmozillahelper restartCommand1=kwin_x11,-session,1014cd7d2d4000144435205700000012260003_1446105607_787343 restartCommand2=/bin/kmix,-session,1014cd7d2d4000144435208500000012260012_1446105607_737385 restartCommand3=/usr/lib/mozilla/kmozillahelper,-session,10bd534f46000144610537700000070460007_1446105607_737492 restartStyleHint1=0 restartStyleHint2=0 restartStyleHint3=0 userId1=lesliezhai userId2=lesliezhai userId3=lesliezhai wasWm1=true wasWm2=false wasWm3=false
When open konsole, dolphin, systemsettings5, then logout legacy.cpp, line 113 windowSessionId(*it, leader) is NOT EMPTY! legacy.cpp, line 214 windowWmCommand(w) is EMPTY!
While debugging the problem with Konsole, I found a common problem with Qt5 applications: QGuiApplicationPrivate::commitData() (supposed to make sure that the session is saved) calls QApplicationPrivate::tryCloseAllWindows() which, when QGuiApplication::quitOnLastWindowClosed() is true, invokes QApplication::quit() which terminates the application immediately after it reports successful session saving to the session manager ksmserver. During session shutdown, ksmserver still removes its bookkeeping data about applications that were terminated for any reason - including those that quit right after session saving. When ksmserver saves the list of existing application slightly later, konsole is gone and, while it has a session save file, that file isn't "registered" in ksmserverrc, which makes it useless. There are at several problems: - QGuiApplicationPrivate::commitData() effectively quits the application if the application didn't call QGuiApplication::setQuitOnLastWindowClosed(false). This is despite Qt's own documentation recommending against quitting the application due to a request to save the session data. - setQuitOnLastWindowClosed(false) is not a good solution. Quit on last window closed is a useful feature, the problem is more that session saving implies closing windows, which doesn't make too much sense. It is supposed to prevent interaction with a state that won't be saved anymore, but there are other ways to do that, e.g. preventing user input and network event handling. - ksmserver should maybe stop removing applications from its internal list while shutting down. Because shutdown can be aborted, changes would have to be queued and applied after an aborted shutdown.
Hi Andreas, Thanks for your analysis, I wrongly argued that it might be libSM issue, I will try your solution.
Hello Leslie, just in case you were planning to change ksmserver, I've since realized that saving the session restore data of each application right away (just after receiving its session restore data) in a separate, dedicated data structure is a much better idea than messing around with the main clients list. When session saving is aborted, simply throw that data away. No interactions to worry about. I still don't agree with Qt's session saving behavior... but I doubt that it's fixable, for compatibility reasons.
https://bugs.kde.org/show_bug.cgi?id=341930
Hi Andreas https://git.reviewboard.kde.org/r/126311/ workaround monkey patch ;P hope you can fix it in right way ;-)
I don't know whether there's a QTBUG reported, but the problem is that QGuiApplication tries to ::tryCloseAllWindows() in QGuiApplicationPrivate::commitData() This is plain wrong, since the window should NOT be closed as long as the session is running. If the session survives for some reason, you'd end up with a half-destroyed environment. => Window closing and process termination has to happen by the smserver once it's clear that the session is gonna end. What the routine *wanted* to do was to send close events to all toplevel widgets (to allow clients to perform last clean-ups, data storage etc.) and check whether all events are accepted. If not, that means some client wanted some window to remain open and that should break the session termination, but without having closed half other windows and maybe some processes before. Don't you dare to call window->closeEvent() directly, the client logic may be in some eventfilter ;-)
reported ;-) https://bugreports.qt.io/browse/QTBUG-49939
In reply to comment 7: Yes, that looks like a working monkey patch :) I'm trying to get this https://codereview.qt-project.org/#/c/142232/ merged to fix the bug properly. I'm also looking at a preliminary fix in ksmserver, but I'm not sure if I understand sub-session support, and on a related note if sub-session support as currently implemented is worth keeping. It does not seem to work very well...
Note: sub-session support in the session manager is basically support for activities. Session restore of activities has never worked well enough to be useful for me.
Hi Andreas, Thanks for your patch! I do not have Qt developer's account, so I could not +1 for your great job. Hope Qt5.x will integrate your patch to fix the restore session relative issue. About ksmserver session management improvement, as you said make it robust http://marc.info/?l=kde-core-devel&m=144832700109449&w=1 is there someone else to work together to make ksmserver better, make KDE5 better ;-) Thanks again for your great job! Regards, Leslie Zhai
Andreas, the Qt patch kills the ability to cancel the logout process (when the process or user prevents closing a window), I'm not sure it will be accepted and if, you probably will have caused a feature breakage. See comment #8. Instead of actually closing, the system likely wants to ask whether anyone is ok with closing this window (ie. cause close events w/o actually closing the window)
(In reply to Thomas Lübking from comment #13) > Andreas, the Qt patch kills the ability to cancel the logout process (when > the process or user prevents closing a window), I'm not sure it will be > accepted and if, you probably will have caused a feature breakage. See > comment #8. Instead of actually closing, the system likely wants to ask > whether anyone is ok with closing this window (ie. cause close events w/o > actually closing the window) How session management works is that the application connects to the QGuiApplication::saveStateRequest() signal. When that signal arrives, the application either saves its state or calls QSessionManager::cancel() on the instance passed in the signal. Session management never works by just trying to close windows without telling the application that it was triggered by session management. In the absence of any proper mechanism, one *could* do it that way, but it's a bad idea because relevant information and actions are not available to the application that way.
The typical fail will be the "there are 5 tabs open, close/cancel" dialogs. Afaiu this was added because of some MS Windows behavior and even if not, one has to assume that clients simply rely on window "save your work first!" protection to be in place and not somewhen been killed by the SM (what will happen when the SM got an "ok" and then tries to close windows or sigterm the process, would it not?) Large behavioral changes could be denied itr, sending out close events should bail you out ;-)
The session manager getting an OK means asking the client and at this point the client can cancel the shutdown, or save its state and wait to get killed.
Yes, my concern is that status quo for several/many applications is that they do no connect to saveStateRequest() or commitDataRequest() and *only* perform interaction on window close events (since this is the regular case *during* the session) Stripping this mechanism will cause a behavioral change that is prone to cause data loss on session exit - thus such change may be denied upstream. If so, the status quo can be restored by sending close events w/o causing a) the actual loss of windows in the running session b) the application to "accidentally" quit early I don't discuss "what should be", but "what mess have we caused and how do we get around that" ;-)
How should that be done, sending close events and expecting applications to save their state in response, but not close windows or the application? It would mean that applications are session management aware but don't use the session management API and implement a very crude version of session management. A stupid thing to do. I think the only choice here is to break session management in applications that actually support it, or break session management in applications that don't (properly) support it, where it may or may not work semi-accidentally. And let's face it, the only Qt applications that really care about session management and do it correctly are X applications, most of which are KDE applications. Something tells me that those are going to be fine.
(In reply to Andreas Hartmetz from comment #18) > How should that be done, sending close events and expecting applications to > save their state in response, but not close windows or the application? QCloseEvent ce; QApplication::sendEvent(window, &ce); You only want to emit the event, the widget doesn't close in response - the event is (usually) emitted when it wants to close. > It would mean that applications are session management aware No, it means they give a shit about "session management", but they do care about what happens when the user (or anything) tries to close a window. The present code in Qt's SM tells me that this is considered the predominant way to handle data-safety - also because it's the regular incident. Notice again that this data protection mechanism has *nothing* to do with session management in particular, eg. whenever you try to close a kwrite window w/ modified text, it will ask you "err, really? maybe safe the file before?" - and at this point the user can also say "whooops, no - I didn't want to close at all" The present code triggers this mechanism and it might be required to preserve that.
*** Bug 354800 has been marked as a duplicate of this bug. ***
I'm actually aware of the problem with session management since last summer, now I've upgraded my stuff to have more KF5-based applications and suprisingly found it still just doesn't work. So I've dived deeper into it this time, reading all the discussion here and last part of bug #341930, both Andreas's change proposals and the thread in the mailing-list. I likely agree with comment #18 of Andreas and in my point of view it is the following: 1. As a whole it's a massive regression since KDE4 which affects all Qt5 applications, most of which behave correctly as a session clients. Even server parts of both KWin and KSMServer now behave correctly, thanks to Thomas's fixes, I think. But as a whole it just doesn't work at all. 2. This problem is caused by some bug in processing session management messages in Qt, which earlier wasn't a big pain and could be avoided, but due to significant changes in the whole interaction process, in the API etc. now it can't be avoided and lead to (1). 3. There's initial change (https://codereview.qt-project.org/#/c/142232/) by Andreas, which perfectly fixes the problem with any observable problems. It also fixes a fault in the session management protocol implementation for at least two OSs, which is good for Qt itself. 4. There could be potentially affected client applications which: a) were already been ported to/written for Qt5; b) process some valuable data which shouldn't be lost; c) would like to use session management to prevent loss of unsaved data; d) still don't care to follow session management protocol correctly and just exploit old hacks and errors in its implementation, which exist historically, but now is moved to a new place. Unfortunately, this term is a little objectless since it wasn't mentioned any real-life application like this. 5. I completely don't like the proposed way to preserve the compatibility with (4) and make the use case of broken session management client implementation legal and default, but also try to allow proper-written apps to still survive somehow, adding some strange workarounds to Qt as closing all the windows, but not too much, or API properties to enable proper processing of SM messages. To sum it all up, I've applied the patch (3) and have all the session management things back again without any other changes to KDE or whatever, it's already released versions (KF-5.18.0, Plasma-5.5.3, applications-15.12.1). I'll also test Windows behaviour with some toy application. Unless any problems arise, I see no reason why this tiny and simple (and right) fix isn't applied and merged.
> 5. I completely don't like the proposed way to preserve the compatibility with (4) and make > the use case of broken session management client implementation legal and default, but > also try to allow proper-written apps to still survive somehow, adding some strange > workarounds to Qt as closing all the windows, but not too much, or API properties to enable > proper processing of SM messages. No ofense, but what you "like" is completely irrelevant. You propose to intentionally break clients by library changes in some minor update, to teach developers to do right, but while you might aim their face, you're gonna hit the users (and probably yours) We had that (I kindly remind of the qDeleteAll fix ...) and it cooked up hell. commitDataRequest hardly shows up in lxr.kde.org, what means it's probably not used at all and aboutToQuit (which isn't used but could come to rescue) isn't used too much either. The BY FAR! omnipresent pattern is to listen to queryClose() which is called/emitted on -guess what- close events from KMainWindow. And that's for pretty much sure why the (wrong) behavior in QSessionManager exists. Is this behavior correct? No. Does this matter? NO! It's ok to spam a #warning that this behavior is shit and deprecate and kill it for Qt6 and we might even bail out (aka "fix") KMainWindow applications NOW by invoking queryClose() on QGuiApplicationPrivate::commitData() but regardless, we MUST assume this to be a global default pattern that applications (also beyond KDE) rely on (also because it's absolutely natural to intercept closing to save data and not think of closing on session end could be something entirely different - actually the illegal behavior happens to be the most sane one...) Now, *actually* closing windows to test interaction on session end is of course just as wrong - if the user cancels the logout by such incident, we should not have closed random other windows before (letting alone that it causes this but) - therefore I frankly do not understand what's so complicated about just faking a close event to serve the present "save your stuff" pattern in a majority in clients without causing the destructive close itself which may not only be a bit premature, but also triggers this bug. It's the least invasive solution that does not require everyone to signal "yes, i can sessionmanagement" (what's not gonna happen) and we don't risk loosing the users data (or breaking the ability to cancel the logout)
(In reply to Thomas Lübking from comment #22) > > 5. I completely don't like the proposed way to preserve the compatibility with (4) and make > > the use case of broken session management client implementation legal and default, but > > also try to allow proper-written apps to still survive somehow, adding some strange > > workarounds to Qt as closing all the windows, but not too much, or API properties to enable > > proper processing of SM messages. > > No ofense, but what you "like" is completely irrelevant. Comments like this clearly don't help the discussion or solving the problem, especially when you start your reply with them. I won't answer you the same style, but given that it's not the first one from you, my earnest request to you is to please respect each other and avoid such comments in future. In case of any questions feel free to consult https://www.kde.org/code-of-conduct/. Thank you. > You propose to intentionally break clients by library changes in some minor > update Never mentioned minor update or particular version. Please don't distort. > to teach developers to do right No intention to do it, but any specs probably means something like that. > but while you might aim their face, you're gonna hit the users (and probably yours) Users were already hit when the significant part of functionality important for someone's every day use case is broken. I just can't get why it's OK to break everything for one part of users and ultimately save broken implementation to preserve some ephemeral compatibility for another. That's probably the biggest question for me in this thread. Maybe I'm wrong and those who use sessions are somewhat less important than users that sometimes save their data on quitting? It's worth mentioning then, and I'll immediately give up. > We had that (I kindly remind of the qDeleteAll fix ...) and it cooked up > hell. Still better than a couple of API methods like "enableSpecifiedBehaviour()" or deleting and trying to catch SIGSEGV, right?) > commitDataRequest hardly shows up in lxr.kde.org, what means it's probably > not used at all and aboutToQuit (which isn't used but could come to rescue) > isn't used too much either. > > The BY FAR! omnipresent pattern is to listen to queryClose() which is > called/emitted on -guess what- close events from KMainWindow. > And that's for pretty much sure why the (wrong) behavior in QSessionManager > exists. If it wasn't done before for some reason, it's better to just fix the applications, especially given that you don't need any changes in Qt to have just the same functionality with the new approach. If it's still too much to change while porting to Qt5/KF5, I really wonder what porting is. Once again: we all could already apply the fix of Andreas and be busy fixing the necessary applications rather than keep discussing here. > Is this behavior correct? No. > Does this matter? NO! > > It's ok to spam a #warning that this behavior is shit and deprecate and kill > it for Qt6 On the Qt6 release you would say that everyone already rely on the workaround there was in Qt5 etc. etc. That's an endless story. By the way, do you really think it's so much major change that it can't be changed before Qt6? Seriously, with no API change and with just removing unexpected actions? > and we might even bail out (aka "fix") KMainWindow applications > NOW by invoking queryClose() on QGuiApplicationPrivate::commitData() but > regardless, we MUST assume this to be a global default pattern that > applications (also beyond KDE) rely on (also because it's absolutely natural > to intercept closing to save data and not think of closing on session end > could be something entirely different - actually the illegal behavior > happens to be the most sane one...) I just kindly remind your description of current Plasma 5 and it's application state: https://bugs.kde.org/show_bug.cgi?id=341930#c30. It was written months ago, but nothing changed too dramatically from then. Even if the proper fix could break some apps, they all are *already in* transition process, Wayland is just around the corner with another transition process, so now it's the perfect time to fix something to make it finally working properly rather than make life easier for now and have this pain for years again and again. > > Now, *actually* closing windows to test interaction on session end is of > course just as wrong - if the user cancels the logout by such incident, we > should not have closed random other windows before (letting alone that it > causes this but) - therefore I frankly do not understand what's so > complicated about just faking a close event to serve the present "save your > stuff" pattern in a majority in clients without causing the destructive > close itself which may not only be a bit premature, but also triggers this > bug. > > It's the least invasive solution that does not require everyone to signal > "yes, i can sessionmanagement" (what's not gonna happen) and we don't risk > loosing the users data (or breaking the ability to cancel the logout) In a couple of years nobody, including you, would remember what on earth are these close events coming when SM server asked to save state, what's the proper processing of it and how it's connected with the documentation and specs. What is especially sad, is that nobody would understand the reason of preserving some stability in actually unstable environment, still being ported to the new framework. That's like having ugly workarounds now which were added to preserve stability of Plasma in KDE 4.0. It's nonsense. To sum it up, I think, the most valueable thing here is that we seem to generally agree on what the proper fix is (especially with your comment #8). The only arguable point is whether it's safe to have it fixed now or should another (possible API-changing) workaround should be added instead.
(In reply to Alexey Chernov from comment #23) > Comments like this clearly don't help Seriously, you asked for breaking clients because that's what you'd "like" to do - what did you expect to hear? That's simply not an acceptable stance. > Never mentioned minor update or particular version. Please don't distort. So you meant to schedule this for Qt6? > Users were already hit when the significant part of functionality important > for someone's every day use case is broken. Let's be honest: session restorage is apparently relevant for only a minority of users. Loosing your data is however relevant for everyone. And the latter is the by far more severe issue. Restarting applications is merely an annoyance, loosing your work is truely expensive. Also there's absolutely NO reason why we should not care about both - except that you'd "like" to break client code and risk data loss for some reason that completely escapes me. > Still better than a couple of API methods like "enableSpecifiedBehaviour()" I fully agree on that proposal to be of little help - it will be mostly ignored or used w/o accounting the implications. > Once again: we all could already apply the fix of Andreas and be busy fixing > the necessary applications rather than keep discussing here. It does NOT only affect KDE applications, there're hundreds of Qt applications which might have adopted this pattern - or simply don't care about session management itfp. Also the proper order is to fix and roll out clients, *then* remove the deprecated upstream code. That's why "=> Qt6" for this approach. > On the Qt6 release you would say that everyone already rely on the > workaround there was in Qt5 etc. etc. No. Because you would tell people during Qt5 don't do this and don't rely on it because it's not gonna work with Qt6, so that when things are ported to Qt6, client code has to be fixed. Breaking it now and depending client behavior on whether it's linked against Qt 5.6 or Qt 5.7 is plain wrong and begging for trouble. > I just kindly remind your description of current Plasma 5 and it's > application state: https://bugs.kde.org/show_bug.cgi?id=341930#c30. Off topic? This was a global statement. Session management in particular is a different thing: few people seem to really care about restoring sessions. > In a couple of years We'll have seen Qt6 and this removed, but even if not - it doesn't matter. The QGuiApplication code will have a "// TODO Qt6" comment and the client code does not care about why there's a close event (which might be rejected, thus not causing eg. deletion anyway) > The only arguable point is whether it's safe to have it fixed now or should > another (possible API-changing) workaround should be added instead. No. Actually I propose to fix the "workaround" already present in QGuiApplication by turning "close()" into just sendind a close event (for that's actually the desired action) and by this fix session storage and maintain data protection with the present approaches. Breaking that behavior may happen for Qt6, anything else will be perceived as regression. On a sidenote: QGuiApplication::commitDataRequest() may be the "preferred" hook, but there's actually nothing that explicitly forbids "fake a close in addition", notably since it will trigger similar, if not equal client code. Given the status quo, I'd probably even just remove the commitDataRequest signal in Qt6 and rely only on faking close events - why should client code have to care about two different "aboutToClose" signals? Sounds stupid to me.
(In reply to Thomas Lübking from comment #24) > (In reply to Alexey Chernov from comment #23) > > > Comments like this clearly don't help > Seriously, you asked for breaking clients because that's what you'd "like" > to do - what did you expect to hear? That's simply not an acceptable stance. No one here presents any absolutely true point, otherwise there were no discussion. I just wrote my point of view and emphasized that it's just mine and not some ultimate truth. > > Never mentioned minor update or particular version. Please don't distort. > So you meant to schedule this for Qt6? No. I just stated that I didn't mention any particular version, no other implications. As to your question, I'd prefer properly test the patch, including success scenario for the default not-aware-of-session-management application, and release it as soon as possible. > > Users were already hit when the significant part of functionality important > > for someone's every day use case is broken. > Let's be honest: session restorage is apparently relevant for only a > minority of users. According to what? Your assumption? It's not too evident for me, sorry. > Loosing your data is however relevant for everyone. And the latter is the by > far more severe issue. Restarting applications is merely an annoyance, > loosing your work is truely expensive. Hey, how could session management be "apparently relevant for only a minority of users", but fixes in its behaviour be crucial for a lot of them? Don't you contradict with yourself in these two points? Anyway, it's very subjective and I wouldn't argue on what's more important. I agree that data loss is the worst thing which could happen. I just think it doesn't mean it should result in some messy API or library code when someone's relying on the undocumented side-effects. Just because it will surely lead to more bugs and more data loss in the future. It's just the bugs which should be fixed either in library and its clients. It's better to fix them when no one really relies on the stability too much. It looks like this time is now for KF5-based application and environment. > Also there's absolutely NO reason why we should not care about both - except > that you'd "like" to break client code and risk data loss for some reason > that completely escapes me. No, that's just postponing and messing up the whole problem. If, as you stated, almost no one implemented easy and pretty simple interaction appeared in Qt5, even less would care of possible bugs and corner cases of the workaround, more complex protocol with close event you propose. There would be just another argument that it's just too messy, not to mention already existing argument that no one uses session management. > > Still better than a couple of API methods like "enableSpecifiedBehaviour()" > > I fully agree on that proposal to be of little help - it will be mostly > ignored or used w/o accounting the implications. Ok. > > Once again: we all could already apply the fix of Andreas and be busy fixing > > the necessary applications rather than keep discussing here. > > It does NOT only affect KDE applications, there're hundreds of Qt > applications which might have adopted this pattern - or simply don't care > about session management itfp. > Also the proper order is to fix and roll out clients, *then* remove the > deprecated upstream code. That's why "=> Qt6" for this approach. Fully agree here, but we should confirm that nobody said in the beggining that upstream changes were about to break session management for KF5 applications. It was just broken. Since we have what we have, there's no other way than to start fixing it on both sides. I think nobody is against if it would be synchronized. > > On the Qt6 release you would say that everyone already rely on the > > workaround there was in Qt5 etc. etc. > > No. Because you would tell people during Qt5 don't do this and don't rely on > it because it's not gonna work with Qt6, so that when things are ported to > Qt6, client code has to be fixed. Oh, you're too optimistic here. Why it's still not fixed during porting on Qt5? Only because it just works. It won't be fixed until it's broken or would be planned to fix as we discussed above. > Breaking it now and depending client behavior on whether it's linked against > Qt 5.6 or Qt 5.7 is plain wrong and begging for trouble. That's again due to your assumption that session management is of lower priority. I'm pretty sure there would be packages that would require just most recent Qt version, and it would be acceptable. What's wrong in relying on changes in recent Qt release and informing the maintainer of it with more strict requirement? There're backports if someone is interested in special cases. > > I just kindly remind your description of current Plasma 5 and it's > > application state: https://bugs.kde.org/show_bug.cgi?id=341930#c30. > Off topic? This was a global statement. Session management in particular is > a different thing: > few people seem to really care about restoring sessions. Repeating it again doesn't make it proved, sorry :) No, it's not off-topic, but an argument that it's generally normal to break something in this state of stability. To get things simpler: I think, it's normal to make this change in this changes time frame, you think, it's too late and we have to wait for the next. > > In a couple of years > We'll have seen Qt6 and this removed, but even if not - it doesn't matter. > The QGuiApplication code will have a "// TODO Qt6" comment and the client > code does not care about why there's a close event (which might be rejected, > thus not causing eg. deletion anyway) In this way all the bugfixes should be postponed till Qt6 as someone could rely on the buggy behaviour or side-effects. By the way, are you completely sure your changes won't break any clients? What if someone relies exactly on the current code? > > The only arguable point is whether it's safe to have it fixed now or should > > another (possible API-changing) workaround should be added instead. > > No. > Actually I propose to fix the "workaround" already present in > QGuiApplication by turning "close()" into just sendind a close event (for > that's actually the desired action) and by this fix session storage and > maintain data protection with the present approaches. It's still changes that should be taken into account both in SM server and clients as long as it's neither "old" nor standard scenario. It's just hotfixing for quite unclear reason. > Breaking that behavior may happen for Qt6, anything else will be perceived > as regression. > > On a sidenote: > QGuiApplication::commitDataRequest() may be the "preferred" hook, but > there's actually nothing that explicitly forbids "fake a close in addition", > notably since it will trigger similar, if not equal client code. > Given the status quo, I'd probably even just remove the commitDataRequest > signal in Qt6 and rely only on faking close events - why should client code > have to care about two different "aboutToClose" signals? Sounds stupid to me. Your own assumptions again, but I don't tend to consider them irrelevant, so in constructive way: that's not so stupid, since commitDataRequest doesn't mean anyone is quitting or closing windows, but means some data interactions. In the opposite, close event or aboutToClose do mean closing and quitting, but don't imply any data interactions (as it's too late). When you close windows on commitData() and interact with both user and SM server while you should close you windows, it pretty much will lead to hurt and pain.
(In reply to Alexey Chernov from comment #25) > According to what? According to "This is not fixed in years and each and every session management code was ported as "#if 0"" If there was some relevant interest, it would be fixed long time, since it's really not that hard. > > Loosing your data is however relevant for everyone. And the latter is the by > > far more severe issue. Restarting applications is merely an annoyance, > > loosing your work is truely expensive. > Hey, how could session management be "apparently relevant for only a > minority of users", but fixes in its behaviour be crucial for a lot of them? [...] > Fully agree here, but we should confirm that nobody said in the beggining > that upstream changes were about to break session management for KF5 > applications. It was just broken. Errr... what? Session management (in terms of "please restore the desktop as I left it") doesn't seem very important, but if you click "logout" and *booom*, gone is your work of the last two hours because the application had no chance (or, well, listened to the wrong events) to ask you to save it, that's pretty important... We're apparently talking past each other. There're two steps: a) logout, clients ask to save your stuff. That works (because of the close event) b) login, clients should restart. That's broken (because the close event is not just an event, but the window "illegally" being withdrawn during logout) You propse to fix (b) by breaking (a) and I'm trying to convince you that this is a really bad idea. > bugs which should be fixed either in library and its clients. It's better to > fix them when no one really relies on the stability too much. It looks like > this time is now for KF5-based application and environment. Yes, we should fix KMainWindow now (if faking close events is finally not considered a permanent behavior despite the majority of clients will probably do that in return to the data commit request - with a fair share actually just calling close() ...) but that has absolutely no implications on whether it's ok to easily break away from established (even though maybe wrong) behavior. > No, that's just postponing and messing up the whole problem. If, as you > stated, almost no one implemented easy and pretty simple interaction > appeared in Qt5, even less would care of possible bugs and corner cases of > the workaround, more complex protocol with close event you propose. There > would be just another argument that it's just too messy, not to mention > already existing argument that no one uses session management. Sorry, but I really cannot read any sense into this paragraph. Please try to rephrase it. The above isn't English grammar at all. > It won't be fixed until it's broken So you demand to jeopardize userdata because otherwise code won't be fixed. Sorry, but there's no way you're ever gonna convince me in this. Any solution that builds upon "jeopardize userdata" is not a solution at all. It's malicious. > I'm pretty sure there would be packages that would require just > most recent Qt version, and it would be acceptable. And jeopardize userdata. What exactly should this help in this case? > What's wrong in relying on changes in recent Qt release That you are not. "Recent Qt releases" start to jeopardize userdata because (no way) all Qt client code has been fixed, ie. distros must not use Qt 5.7 until all client code releases picked up the change. > In this way all the bugfixes should be postponed till Qt6 If a bugfix is *very* likely gonna break more things more badly than it fixes and there is absolutely no need to take this risk? Yes, avoid that fix and come up with a more robust interim solution. > By the way, are you completely sure your changes won't break any clients? > What if someone relies exactly on the current code? Err, on the close event? Well, that's exactly what I suggest to preserve. You're not gonna see more from a ::close() call in client code (it's no virtual and not invoked as slot) > In the opposite, close event or aboutToClose do mean closing > and quitting, but don't imply any data interactions (as it's too late) Errr, no. Please read the Qt API docs. You simply ::ignore() the close event and nothing is gonna be closed or changed. That's why it's massively intercepted to ask users to save stuff - what's also explicitly suggested in the API docs. And that's why somebody considered it a good idea to explicitly close windows on session end, what it's just not. That "somebody" actually wanted to trigger and check the result of the event, but not really close windows. And that's what I'm proposing to deal with the situation. The justification for QGuiApplication::commitDataRequest() though is, that a QGuiApplication doesn't necessarily have any windows at all and still wants to save stuff on logout (and of course before the SIGTERM) ===================== Completely untested patch (not even a build attempt) for Qt 5.6: ===================== diff --git a/src/gui/kernel/qguiapplication.cpp b/src/gui/kernel/qguiapplication.cpp index 7d469dd..3b09ddd 100644 --- a/src/gui/kernel/qguiapplication.cpp +++ b/src/gui/kernel/qguiapplication.cpp @@ -3232,8 +3232,31 @@ void QGuiApplicationPrivate::commitData() Q_Q(QGuiApplication); is_saving_session = true; emit q->commitDataRequest(*session_manager); - if (session_manager->allowsInteraction() && !tryCloseAllWindows()) - session_manager->cancel(); + if (session_manager->allowsInteraction()) { + // trigger close event handling in all windows + // this allows them to clean up and if they refuse + // to close, we cancel the logout + // NOTICE: do NOT actually close windows - this behavior is + // explicitly wrong and would prevent the window from being + // restored with the session + QCloseEvent closeEvent; + QWindowList list = QGuiApplication::topLevelWindows(); + QWindowList processedWindows; + for (int i = 0; i < list.size(); ++i) { + QWindow *w = list.at(i); + if (!processedWindows.contains(w)) { + sendEvent(w, &closeEvent); + if (!ce.isAccepted()) { + session_manager->cancel(); + break; // logout canceled, no further close hinting + } + processedWindows.append(w); + list = QGuiApplication::topLevelWindows(); + i = -1; + } + } + } + } is_saving_session = false; } ====================== --- off topic > > > Never mentioned minor update or particular version. Please don't distort. > > So you meant to schedule this for Qt6? > No. I just stated that I didn't mention any particular version, no other > implications. https://en.wikipedia.org/wiki/Software_versioning major.minor.patch|revision|release|build So either you point Qt6 or a minor release. --- off topic on off topic > No, it's not off-topic Yes it is. KDE's problems lie in different areas (predominantly QtQuick, to be precise) - broken session management isn't a showstopper at all in lights of "desktop completely freezes", "plasmashell constantly runs at 100% CPU" and "everything crashes all the time with obscure backtraces" bugreports.
You can't just send fake close events to clients that don't expect that. That... technique... is a KDE specialty. KDE applications are written to deal with it. In the general case, though, it is legitimate to start destroying internal data structures in a close event, and it is legitimate not to expect more than one close event before the window is actually closed. Case in point: fixing KMainWindow and KApplication to restore their KDE4 behavior (I have locally tested Qt and KDE patches to that effect) makes Kate crash on logout. Changing behavior but not API is *worse* than adding API that optionally changes behavior - it silently breaks expectations of existing software. We can, however, implement a workaround in KDE (and then fix our stuff when something breaks): At the end of the slot handling commitDataRequest(), install an event filter on the QGuiApplication, which nicely filters ALL events to everything (TODO: check that - otherwise we'd just install an event filter on all toplevel windows). In that filter, check whether QGuiApplication::isSavingSession() is still true: if so, filter out and ignore() all CloseEvents. If not, have the the filter uninstall and delete itself for performance reasons. If you look at QWidgetPrivate::close_helper(), you see that it always sends a close event to ask windows if they agree to be closed, and they can always refuse. Now which repository should that go in? It would be ugly to copy and paste the necessary code around - it should be roughly ten lines.
(In reply to Thomas Lübking from comment #26) > (In reply to Alexey Chernov from comment #25) > > > According to what? > According to "This is not fixed in years and each and every session > management code was ported as "#if 0"" > If there was some relevant interest, it would be fixed long time, since it's > really not that hard. Rather interesting indicator. Why don't you apply it to Qt5 or KF5 then? What a selective vision :) > > > Loosing your data is however relevant for everyone. And the latter is the by > > > far more severe issue. Restarting applications is merely an annoyance, > > > loosing your work is truely expensive. > > > Hey, how could session management be "apparently relevant for only a > > minority of users", but fixes in its behaviour be crucial for a lot of them? > [...] > > Fully agree here, but we should confirm that nobody said in the beggining > > that upstream changes were about to break session management for KF5 > > applications. It was just broken. > > Errr... what? > Session management (in terms of "please restore the desktop as I left it") > doesn't seem very important, but if you click "logout" and *booom*, gone is > your work of the last two hours because the application had no chance (or, > well, listened to the wrong events) to ask you to save it, that's pretty > important... Hmmm... so session management "doesn't seem very important", but there're applications which expect a) to be closed gently, and also b) to have an opportunity to interact to the user the very special way, so that the rest of the world is waiting for them and doesn't logout, but it's surely not session management. Nice. Following this way we have, I think, thousands of apps which don't use X, kernel etc. and other not so important stuff. > We're apparently talking past each other. > There're two steps: > a) logout, clients ask to save your stuff. That works (because of the close > event) > b) login, clients should restart. That's broken (because the close event is > not just an event, but the window "illegally" being withdrawn during logout) > > You propse to fix (b) by breaking (a) and I'm trying to convince you that > this is a really bad idea. The matter is just that if you like the fruitful results of some service or protocol, you need to follow the rules of it. If you violate them and currently it just works, it's natural that anyone can change something internally and you are going to fail. Rather atomic thing. My proposal is just to have library code of Qt following the proper interaction process, which is expected by anyone who haven't read this discussion and just wants to support session management in the application. Nothing against any workarounds in KSMServer, KF wrappers or anywhere else downstream. > > bugs which should be fixed either in library and its clients. It's better to > > fix them when no one really relies on the stability too much. It looks like > > this time is now for KF5-based application and environment. > Yes, we should fix KMainWindow now (if faking close events is finally not > considered a permanent behavior despite the majority of clients will > probably do that in return to the data commit request - with a fair share > actually just calling close() ...) but that has absolutely no implications > on whether it's ok to easily break away from established (even though maybe > wrong) behavior. There's no one accepted fix of Qt to fix anything against. There's a way to fix applications to interact with session manager properly though and add some fixes and workarounds to make it work somehow, at least with any local Qt patch. According to the comments of https://codereview.qt-project.org/#/c/146566/, that's something what Andreas is doing. > > No, that's just postponing and messing up the whole problem. If, as you > > stated, almost no one implemented easy and pretty simple interaction > > appeared in Qt5, even less would care of possible bugs and corner cases of > > the workaround, more complex protocol with close event you propose. There > > would be just another argument that it's just too messy, not to mention > > already existing argument that no one uses session management. > > Sorry, but I really cannot read any sense into this paragraph. > Please try to rephrase it. The above isn't English grammar at all. Again. Please try to reread it: https://www.kde.org/code-of-conduct/. Hopefully, it's English would impress you more. > > It won't be fixed until it's broken > So you demand to jeopardize userdata because otherwise code won't be fixed. > Sorry, but there's no way you're ever gonna convince me in this. > Any solution that builds upon "jeopardize userdata" is not a solution at > all. It's malicious. Just one workaround to close the clients gently (with timeout or whatever) in session server would save anyone who is afraid of "jeopardize userdata". I just don't want this apparently kind purpose of preserving good old behaviour to "jeopardize" Qt library code quality. > > I'm pretty sure there would be packages that would require just > > most recent Qt version, and it would be acceptable. > And jeopardize userdata. What exactly should this help in this case? Is it planned to repeat your recently invented "jeopardize userdata" formula in every message?) What did you answer here? I just meant that it's ok to manifest that some package now require more recent Qt version and support of the previous versions is untested. > > What's wrong in relying on changes in recent Qt release > That you are not. "Recent Qt releases" start to jeopardize userdata because > (no way) all Qt client code has been fixed, ie. distros must not use Qt 5.7 > until all client code releases picked up the change. Hmmm... you're either too idealistic here or just kidding. We've already discussed it and generally I have nothing to add here. In short: I (and probably most of the distribution maintainers, since AFAIK there's no any LTS release with Plasma 5 yet) don't consider Plasma 5 environment to be absolutely stable, it's actually being still constructed. In fact, there're still some serious bugs to be fixed, which you seem to admit. So I think it's appropriate to use this chance to fix some important at least for someone protocol, rather than wait till another transfer with subtly broken (well, broken) part during the whole this period. > > In this way all the bugfixes should be postponed till Qt6 > If a bugfix is *very* likely gonna break more things more badly than it > fixes and there is absolutely no need to take this risk? Yes, avoid that fix > and come up with a more robust interim solution. Who decides, what's more and what's robust?) > > By the way, are you completely sure your changes won't break any clients? > > What if someone relies exactly on the current code? > Err, on the close event? Well, that's exactly what I suggest to preserve. > You're not gonna see more from a ::close() call in client code (it's no > virtual and not invoked as slot) Didn't tested your solution at all. I just asked whether you're sure that this quite complex hack wouldn't suddenly break something. > > In the opposite, close event or aboutToClose do mean closing > > and quitting, but don't imply any data interactions (as it's too late) > Errr, no. Please read the Qt API docs. > You simply ::ignore() the close event and nothing is gonna be closed or > changed. Misunderstanding again. You asked, what commitDataRequest is for. That's a part of the answer on this. > That's why it's massively intercepted to ask users to save stuff - what's > also explicitly suggested in the API docs. > And that's why somebody considered it a good idea to explicitly close > windows on session end, what it's just not. That "somebody" actually wanted > to trigger and check the result of the event, but not really close windows. > And that's what I'm proposing to deal with the situation. > > The justification for QGuiApplication::commitDataRequest() though is, that a > QGuiApplication doesn't necessarily have any windows at all and still wants > to save stuff on logout (and of course before the SIGTERM) > > ===================== > Completely untested patch (not even a build attempt) for Qt 5.6: > ===================== > > diff --git a/src/gui/kernel/qguiapplication.cpp > b/src/gui/kernel/qguiapplication.cpp > index 7d469dd..3b09ddd 100644 > --- a/src/gui/kernel/qguiapplication.cpp > +++ b/src/gui/kernel/qguiapplication.cpp > @@ -3232,8 +3232,31 @@ void QGuiApplicationPrivate::commitData() > Q_Q(QGuiApplication); > is_saving_session = true; > emit q->commitDataRequest(*session_manager); > - if (session_manager->allowsInteraction() && !tryCloseAllWindows()) > - session_manager->cancel(); > + if (session_manager->allowsInteraction()) { > + // trigger close event handling in all windows > + // this allows them to clean up and if they refuse > + // to close, we cancel the logout > + // NOTICE: do NOT actually close windows - this behavior is > + // explicitly wrong and would prevent the window from being > + // restored with the session > + QCloseEvent closeEvent; > + QWindowList list = QGuiApplication::topLevelWindows(); > + QWindowList processedWindows; > + for (int i = 0; i < list.size(); ++i) { > + QWindow *w = list.at(i); > + if (!processedWindows.contains(w)) { > + sendEvent(w, &closeEvent); > + if (!ce.isAccepted()) { > + session_manager->cancel(); > + break; // logout canceled, no further close hinting > + } > + processedWindows.append(w); > + list = QGuiApplication::topLevelWindows(); > + i = -1; > + } > + } > + } > + } > is_saving_session = false; > } I think, code comment describes the complexity of the interaction so brightly. Looks like really dangerous way to be used and thoroughly tested. > > ====================== > > > --- off topic > > > > > Never mentioned minor update or particular version. Please don't distort. > > > So you meant to schedule this for Qt6? > > No. I just stated that I didn't mention any particular version, no other > > implications. > > https://en.wikipedia.org/wiki/Software_versioning > major.minor.patch|revision|release|build > > So either you point Qt6 or a minor release. Oh, formal approach. OK: I stated that I didn't mentioned any particular version. It implies nothing. Informally: if you use Qt for more than a couple of versions, you should have known that changing the left digit means something like platform restructure and changing the middle one pretty often promises major changes. That's why waiting with bug fixing till Qt6 is a little too conservative. > --- off topic on off topic > > > No, it's not off-topic > Yes it is. KDE's problems lie in different areas (predominantly QtQuick, to > be precise) - broken session management isn't a showstopper at all in lights > of "desktop completely freezes", "plasmashell constantly runs at 100% CPU" > and "everything crashes all the time with obscure backtraces" bugreports. Are you a moderator here? :) You've just stated what was discussed above, "broken session management isn't a showstopper at all". It needs to be just fixed the right way.
We cannot change Qt in a way that breaks existing applications. Qt5 has not exactly just been released, and commercial customers value stability very much. Some of them even pay for Qt licenses, which is good for all Qt users, so really, we should not make things worse for them.
(In reply to Andreas Hartmetz from comment #29) > We cannot change Qt in a way that breaks existing applications. Qt5 has not > exactly just been released, and commercial customers value stability very > much. Some of them even pay for Qt licenses, which is good for all Qt users, > so really, we should not make things worse for them. The same way commercial customers or applications would be affected with API changes. I think, this issue (and fix) more concerns the environment than the application itself.
(In reply to Alexey Chernov from comment #30) > The same way commercial customers or applications would be affected with API > changes. How an ABI styable API extension could affect anyone is frankly beyond me - I doubt it will help to resolve the problem but there's really no problem with it.
(In reply to Andreas Hartmetz from comment #27) > We can, however, implement a workaround in KDE (and then fix our stuff when > something breaks): > [...] > Now which repository should that go in? It would be ugly to copy and paste > the necessary code around - it should be roughly ten lines. That's some sort of problem. KXmlGui/KMainWindow would cover most™ cases, but certainly not all. The idea of KF5 is to merely optionally extend Qt and the QPA plugin would affect every Qt application. Since this will however also require to fix the application wrt listening to the datacommit request itfp, this could only apply to the fixed cases - which would for a general fix then be KMainWindow, calling queryClose() on this occasion.
For a KMainWindow solution, one should not require a nasty global eventfilter on the application - handling KMainWindow::closeEvent() should be sufficient, but there might be an additional pitfall on modal windows (ie. if there's already a save dialog, we might have to forcefully activate that to cause user interaction) However, as long as QGuiApplication cancels the session lougout as long as any close event is ignored, this cannot be applied either (ie. still requires your new patch to Qt) and, of course, this doesn't provide a solution for Qt5 either - session management on Qt5 will remain broken forever and just some KDE applications work around that. That's a pretty crappy situation :-(
Yes indeed, it doesn't work because ignoring close events cancels logout. Damn.
How exactly did you try the kwrite crashing workaround? Just by sending a zombie closeEvent? Do you still have a backtrace? (Let's say it's legit for a leaf widget to assume that the close event it doesn't ignore() will cause a close with all implications on future user interaction, data deletion and stuff, but it's not necessarily good style.
Fun fact (though you likely already know) - KApplication::commitData(.) *did* send a fake close event to everything but KMainWindow ...
Created attachment 96913 [details] Fix session saving / KApplication changes
Created attachment 96914 [details] Fix session saving / KMainWindow changes
These patches mostly fix session saving (and therefore restoring), together with the necessary Qt patch. Applications not using KApplication or KMainWindow will need to call QSessionManager::setAutoCloseWindowsEnabled(false) themselves. There a some processes like that in your average KDE session but the patches are trivial so I'm not posting them.
Please file a review request (attaching the frameworks group), the bug is assigned to Seli and I'm not sure anybody but us reads it. I can already say that it lacks a QT_VERSION test. About the Qt side, one might want to consider using a Qt::ApplicationAttribute instead? "Qt::AA_DoNotKludgeSessionSaving"?
Those patches are just what I currently have, they are just intended to show the important logic changes. I wasn't really planning to even submit them for review because unfortunately I seem to be the expert on session management. It seems pretty clear that applications either largely expect KDE4 behavior through old APIs, which the patches restore, or they don't but still expect not to get killed for no good reason. The solution there is clear as well.
So, looking into the Qt sources a bit more it seems that a) Session management is FUBAR in Qt5 and actually was likewise FU in Qt4 - adding the proposed flag will only restore the completely FU condition of Qt4, ie. allow clients (KDE) to workaround the brokeness ... b) this actually *only* seems to affect the xcb platform ("GuiApplicationPrivate::commitData()" is only called by "QPlatformSessionManager::appCommitData()" which is only called by "sm_performSaveYourself(QXcbSessionManager *sm)" in QXcbSessionManager) Restoring the Qt4 state however won't cut it: KApplication is deprecated and there's no Guarantee for a KMainWindow around. Also every "plain" Qt5 client that can be found on KDE users desktops (qupzilla, trojita, ... whatever) won't be covered. If you "seem to be the expert on session management" we need to define a roadmap out of this mess, or it's never gonna happen. What do you think about this process: 1. emit commitDataRequest() 2. at this point either the logout should be canceled (requires API addition on QSessionManager to wire up sm_cancel; internal requirement only though. Maybe kept in d_ptr contexts) or there thould be no windows with QWidget::isWindowModified() or modal transients around since that indicates some user interaction should have taken place but didn't 3. If we detect missing interaction, spam a QMessageBox and inform/ask the user: "It looks like the application should have intercepted the logout request, but didn't. Do you wish to cancel the logout and explicitly save data? Since this is an application bug and this failsafe check will be removed with Qt6, please file a bug against the application." 4. If this still didn't cancel the logout, start closing windows depending on an application attribute (see below) and a trumping environment variable QT_CLOSE_WINDOWS_ON_LOGOUT_REQUEST (to pass the user ultimate control over the behavior) 5. Remove attribute, failsafe check and (actually even documented, we can NO WAY just remove this in Qt5) window closing in Qt6 I'd go for the application attribute since indeed there's little point in adding functions to implement a short-term workaround. Actually, I might even be in favor of a dynamic property (QObject::setProperty()) since it will spare the clients compile time checks - we an add this to KFooBar now, even though Qt 5.6 (or whatever) is not on the horizon.
Guys, I just wanted to say a big thanks for looking into this :). Being the original reporter of related bug #341930, session management in *very* important for me. While your technical discussion is way far above my technical abilities, if there is anything I can do, like testing patches, please feel free to ask ! Thanks again :) :) :)
I don't think that either dynamic properties or changing behavior that has been pretty much proven to be not broken by being around for over 10 years with no complaints will fly upstream, and I don't think they are a very good idea myself. For Qt5, an application attribute might be a good idea. For Qt6, I don't know. I mean there is the small problem that pseuso-SM for applications that think ignoring SM is fine only works if it asks absolutely nothing from applications. An application attribute might even work there as well: if you do care about SM, you have let's say at least 20 lines to write so you can live with another trivial one.
..and frankly, I don't feel like gold-plating the solution to this mess. It's not going to be pretty either way, nobody cares too much except when their stuff breaks (ours did), and there are many people to convince to effect really big changes. There is bigger fish to fry.
Well, "shrug, I don't care then" - Qt seems to be dropping desktop support anyway, so it probably doesn't really matter. For principal reasins I'd however object " proven to be not broken by being around for over 10 years": It wasn't "broken" because one major client class (KDE) simply replaced that "unbroken" code. Nor did I mean to support directly "changing behavior" - I simply suggested to raise awareness of the problem by intercepting "this looks unimplemented" conditions to allow getting rid of the "unbroken" code in Qt6 w/o pain. Closing windows certainly is wrong (and apparently wasn't done before Qt4, seems more like a closeEvent was faked previously) because it alters the session even on a canceled session exit, regardless on whether it breaks restoring them (which one would kludge away in the session manager) Ok, bottom line is SM is FUBAR on Qt5 and there's no intention to ever fix that, merely re-allow client code to bypass the broken Qt behavior. I'll tag future bugs respectively.
Git commit 58e49487aece3de19aae90bbb9b80cd5aab94d04 by Andreas Hartmetz. Committed on 19/02/2016 at 18:55. Pushed by ahartmetz into branch 'master'. Fix session management for KApplication based applications. - Call QGuiApplication::setFallbackSessionManagementEnabled(false) to prevent premature application exit - Wire up the saveStateRequest() and commitDataRequest() signals to the appropriate methods that had to be turned into slots first. Those methods were never even called, they were not ported properly. - Cancel logout when the user decides to do that. A comment in the code was not sufficient to do that. (?!?!) M +16 -1 src/kdeui/kapplication.cpp M +15 -14 src/kdeui/kapplication.h http://commits.kde.org/kdelibs4support/58e49487aece3de19aae90bbb9b80cd5aab94d04
Git commit f7cbcc77722256db084d3b0ab6ce76173e959f0e by Andreas Hartmetz. Committed on 19/02/2016 at 18:49. Pushed by ahartmetz into branch 'master'. Fix session management broken since KF5 / Qt5. Requires Qt 5.6 branch not more than a few days old, or >= 5.6.0 when it is released. Parts of the fix are: - Call QGuiApplication::setFallbackSessionManagementEnabled(false) to prevent application suicide through a mechanism that tries to help applications without any proper session management support, but badly interferes with applications that do implement proper session management, such as KDE applications. - Add back commitData[Request] handling. For some reason it was removed during porting. - Change the returned types of saveState() and commitData() to void. The return values were unused. M +41 -3 src/kmainwindow.cpp M +2 -1 src/kmainwindow_p.h http://commits.kde.org/kxmlgui/f7cbcc77722256db084d3b0ab6ce76173e959f0e
Git commit a08befeac43647e222f48dfd7bed067be81573c4 by Andreas Hartmetz. Committed on 19/02/2016 at 19:08. Pushed by ahartmetz into branch 'master'. KNotes: fix session save / restore. Requires Qt >= 5.6.0 or recent 5.6 branch. M +3 -0 knotes/src/apps/knotesapp.cpp http://commits.kde.org/kdepim/a08befeac43647e222f48dfd7bed067be81573c4
*** Bug 349432 has been marked as a duplicate of this bug. ***
*** Bug 357942 has been marked as a duplicate of this bug. ***
*** Bug 343518 has been marked as a duplicate of this bug. ***
*** Bug 355707 has been marked as a duplicate of this bug. ***
*** Bug 357641 has been marked as a duplicate of this bug. ***
Just upgraded to 5.6.0, it is NOT fixed, but quite to the contrary, session restore is now even more broken than before. Until now Firefox and Cairo-Dock were restored, but not anymore. Please reopen!
Sorry, forgot details: Arch Linux 64 bit qt5 5.6.0 plasma 5.6.0 plasma-framework 5.20.0
Firefox or cairo-dock were certainly not targetted or affected by the related patches. This only affects Qt applications directly, so if that indeed triggered something, either the session crashes on logout or the mentioned applications used some timer based self-quitting (and are thus no more alive when the session ends)
(In reply to Thomas Lübking from comment #57) > Firefox or cairo-dock were certainly not targetted or affected by the > related patches. I did some more testing, and indeed Qt apps (or at least most of them) seem to get restored properly now. The following get restored: - Dolphin - Konversation - Konsole - Ksysguard - KDE system settings The following do NOT get restored: - Firefox - Google Chrome - VLC Media Player - Cairo-Dock Isn't Chrome using Qt tho? I don't know. And I have no idea what may lie beneath this issue, I'm just a noob and all I can do is telling what I experience. I have another issue which may be related, but the two are spanning across different time frames so it doesn't seem likely. This issue is shutdown being halt with "A stop job is running for session c2 of user" with a 1:30 timeout counter. However, session restore was broken ever since I use Plasma/KDE 5, which is more than a year I think, while this issue only started 1-2 months ago. Also, the "stop job is running" issue appears randomly, while session restore always failed consistently.
(In reply to Storm Engineer from comment #58) > The following do NOT get restored: > - Firefox https://forum.kde.org/viewtopic.php?f=111&t=131515 > Isn't Chrome using Qt tho? No. And as mentioned, Qt (vlc) still *is* broken itr. It by this patch just got the *option* to not act brokenly. Every client needs to opt into that. (But that's not been different with Qt4, just no additional Qt API was required then and kdelibs fixed it for most KDE applications implicitly) > I have another issue which may be related, but the two are spanning across > different time frames so it doesn't seem likely. This issue is shutdown > being halt with "A stop job is running for session c2 of user" with a 1:30 Seems a systemd bug, see https://bugs.freedesktop.org/show_bug.cgi?id=70593 or https://bugs.freedesktop.org/show_bug.cgi?id=70593 It might be related to firefox session management, you could try whether you also get this problems when quitting all applications (ensure there're no stale processes left!) before exitting the session and then (if it's now gone) see whether adding some process back at the session end (ie. leave dolphin running, try shutdown; then leave firefox for the next shutdown) causes this. The process may not quit on sigterm.
Just wanted to say thank you. I've got ~10 articles open in okular at a given time, and since it has no session management of its own this fix was most appreciated. Wish I could beam you a beer.
Yup also worked for ArchLinux now ;-)
(In reply to Storm Engineer from comment #58) > (In reply to Thomas Lübking from comment #57) > > Firefox or cairo-dock were certainly not targetted or affected by the > > related patches. > > I did some more testing, and indeed Qt apps (or at least most of them) seem > to get restored properly now. The following get restored: > - Dolphin > - Konversation > - Konsole > - Ksysguard > - KDE system settings > > The following do NOT get restored: > - Firefox > - Google Chrome > - VLC Media Player > - Cairo-Dock > > Isn't Chrome using Qt tho? I don't know. > > And I have no idea what may lie beneath this issue, I'm just a noob and all > I can do is telling what I experience. > > I have another issue which may be related, but the two are spanning across > different time frames so it doesn't seem likely. This issue is shutdown > being halt with "A stop job is running for session c2 of user" with a 1:30 > timeout counter. However, session restore was broken ever since I use > Plasma/KDE 5, which is more than a year I think, while this issue only > started 1-2 months ago. Also, the "stop job is running" issue appears > randomly, while session restore always failed consistently. The problem not restored GTK+ applications in Plasma 5.6.x (built with Qt 5.6.x) was cased removing support for XSM protocol: https://quickgit.kde.org/?p=plasma-workspace.git&a=commit&h=5f0ca1305db4a925dbdbf927f541497be334feff https://bugs.kde.org/show_bug.cgi?id=362671 I reported bug related with restoring GTK+ applications, and before couple of days it has been restored. Check this report: https://bugs.kde.org/show_bug.cgi?id=362671 (fix applied in branch 5.6, branch 5.7 and master).
(In reply to Leslie Zhai from comment #61) > Yup also worked for ArchLinux now ;-) I use Antergos (Arch based distro) and I observed next problem. I mean that konsole is not restored in such case. One day I shut down system (calling proper option in K menu) and when next day I login konsole is not restored, wheras kwrite, kate, dolphin, okular are restored. This issue not happens when I shut down computer and run it again the same day. Additionally I tried shut down using qdbus command like this: "qdbus org.kde.ksmserver /KSMServer logout 0 2 2", but with the same result. So I wonder if this issue is related only for Arch based distributions or this is some bug in konsole :/.
*** Bug 366673 has been marked as a duplicate of this bug. ***
*** Bug 363876 has been marked as a duplicate of this bug. ***