Differential D11818

Don't restart the blocking process on session restore
ClosedPublic
Actions

Authored by aleksejshilin on Mar 30 2018, 7:59 PM.

Details

Reviewers
dhaumann
Group Reviewers
Kate
Commits
R40:0a73e00e6dd7: Don't restart the blocking process on session restore
Summary

All blocking Kate processes were restarted on session restore which
led to launching (possibly) multiple redundant instances.

This change asks the session manager to not restart such processes.

Note: QObject::connect() is used here because the session manager
can't be accessed directly according to the documentation [1]:

In Qt, session management requests for action are handled by
the two signals QGuiApplication::commitDataRequest() and
QGuiApplication::saveStateRequest(). Both provide a reference
to a QSessionManager object as argument. The session manager
can only be accessed in slots invoked by these signals.

[1] http://doc.qt.io/qt-5/qsessionmanager.html#details

BUG: 360066
FIXED-IN: 18.04

Test Plan
0. Make sure that Plasma is configured to restore previous session
   on startup.
1. Open Dolphin and create 'test1' and 'test2' text files.
2. Open 'test1' in Kate from Dolphin.
3. Open 'test2' in the already running Kate instance from Dolphin.
4. Log out and back in. Check that there are no redundant Kate
   instances running.

Diff Detail

Repository
R40 Kate
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
aleksejshilin created this revision.Mar 30 2018, 7:59 PM
Restricted Application added a project: Kate. · View Herald TranscriptMar 30 2018, 7:59 PM
aleksejshilin requested review of this revision.Mar 30 2018, 7:59 PM
ngraham edited the summary of this revision. (Show Details)Mar 30 2018, 8:11 PM
dhaumann added a subscriber: dhaumann.Mar 31 2018, 7:13 AM

Would it also be possible to call setRestartHint() directly, or why is the connect required?

ngraham added a subscriber: ngraham.Mar 31 2018, 2:16 PM

Excellent, this resolves the issue in https://bugs.kde.org/show_bug.cgi?id=360066 for me!

aleksejshilin added a comment.Mar 31 2018, 2:48 PM
In D11818#237436, @dhaumann wrote:

Would it also be possible to call setRestartHint() directly, or why is the connect required?

See QSessionManager documentation:

In Qt, session management requests for action are handled by the two signals QGuiApplication::commitDataRequest() and QGuiApplication::saveStateRequest(). Both provide a reference to a QSessionManager object as argument. The session manager can only be accessed in slots invoked by these signals.

dhaumann accepted this revision.Mar 31 2018, 7:49 PM

Thanks for the additional info. Please add this comment to the commit log when you push, so that we have it in the history. Thanks!

This revision is now accepted and ready to land.Mar 31 2018, 7:49 PM
aleksejshilin edited the summary of this revision. (Show Details)Apr 2 2018, 9:53 AM
aleksejshilin added a comment.Apr 2 2018, 9:55 AM

Am I expected to cherry-pick this commit to the Applications/18.04 branch after pushing it to master?

ngraham added a comment.Apr 2 2018, 1:12 PM

You'd want to land the diff on the 18.04 branch, then merge to master. See https://community.kde.org/Infrastructure/Phabricator#Landing_on_the_.22Stable_branch.22

Closed by commit R40:0a73e00e6dd7: Don't restart the blocking process on session restore (authored by aleksejshilin). · Explain WhyApr 2 2018, 4:10 PM
This revision was automatically updated to reflect the committed changes.
dhaumann added a comment.Apr 2 2018, 8:25 PM

Just as more background: no, you are definitely not expected to push to the branch. You only should push to the branch if the bug justifies a mostly untested patch (untested in the sense of that it's probably only you who did proper testing). That said, it usually is up to you to decide. Personally, I believe crashes and severe bug fixes should be backported, but given the risk of introducing regressions in stable versions, commits to the stable branch are often not necessary. It all boils down to risk, and the fact that you take responsibility for what you do :-)

Revision Contents
Changeset List

PathPackages
Mkate/main.cpp (12 lines)

Diff 31154

View Options

kate/main.cpp

Loading...