(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.