Differential D25106

Also allow invoking session restoration logic when apps are manually launched
ClosedPublic
Actions

Authored by ngraham on Nov 1 2019, 3:28 PM.

Details

Reviewers
davidedmundson
dfaure
vkrause
Group Reviewers
Frameworks
Commits
R263:46e5b32ed929: Also allow invoking session restoration logic when apps are manually launched
Summary

Right now session restoration logic only ever gets invoked when apps are started
automatically after rebooting, because of the check for qApp->isSessionRestored().
This limits the feature to only working for that use case, since qApp->isSessionRestored()
is set by Qt and we can't override it in the app itself to signal that we want the session
restoration behavior for other reasons. As a result, we can't use it to implement the
behavior of restoring the condition of app when it's manually launched, which has been
requested for several of our apps.

This patch removes the check for qApp->isSessionRestored(), which opens the door to
implementing the requested features in our apps.

CCBUG: 397463
CCBUG: 413564

Test Plan

D11382 now works

Normal session restoration behavior after a reboot is unchanged

Apps with session restoration behavior but without a user-facing option to invoke it
don't restore session at inappropriate times.

Diff Detail

Repository
R263 KXmlGui
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
ngraham created this revision.Nov 1 2019, 3:28 PM
Restricted Application added a project: Frameworks. · View Herald TranscriptNov 1 2019, 3:28 PM
Restricted Application added a subscriber: kde-frameworks-devel. · View Herald Transcript
ngraham requested review of this revision.Nov 1 2019, 3:28 PM
davidedmundson added a comment.Nov 8 2019, 1:34 PM

KConfigGui::sessionConfig();

This method behaves weirdly and not what I expected from reading this code.

If no-one has explicitly called KConfigGui::sessionConfig it creates a new one using qApp->sessionConfig() regardless of whether we're session restored or not. If we're not sessionRestored it'll effectively leak a bogus config with a broken ID name.

I think to accept the patch we would need to also change

kconfiggui.cpp:42 to be

  • if (!hasSessionConfig()) {

+ if (!hasSessionConfig() && qApp->isSessionRestored()) {

ngraham added a comment.Nov 19 2019, 6:30 PM

Can I get a review on this?

davidedmundson accepted this revision.Nov 20 2019, 10:06 AM

Normal session restoration behavior after a reboot is unchanged

Please make sure you try with a few apps that aren't just dolphin

This revision is now accepted and ready to land.Nov 20 2019, 10:06 AM
ngraham added a comment.Nov 20 2019, 2:25 PM

Yep, there are no changes in behavior or regressions when testing with Okular, Kate, System Settings and Discover.

Closed by commit R263:46e5b32ed929: Also allow invoking session restoration logic when apps are manually launched (authored by ngraham). · Explain WhyNov 20 2019, 2:26 PM
This revision was automatically updated to reflect the committed changes.
mlaurent reopened this revision.Nov 26 2019, 8:04 AM
mlaurent added a subscriber: mlaurent.

This patch creates a bug in kontact as it creates 2 kontacts now.

Could you reevaluate this patch please ?

Regards

This revision is now accepted and ready to land.Nov 26 2019, 8:04 AM
vkrause added a comment.Nov 26 2019, 8:23 AM

This indeed seems to have negative impact on Kontact, while other people get duplicates it doesn't start at all for me anymore. With this patch reverted it's back to normal.

lbeltrame added a subscriber: lbeltrame.Nov 26 2019, 8:55 AM

Found also by openQA: two kontact windows get opened.

davidedmundson added a comment.Nov 26 2019, 10:19 AM

This patch was supposed to be partnered with a change to KConfigGui::sessionConfig() to keep behaviour the same.

It seems that is not merged.

ngraham added a comment.Nov 26 2019, 2:59 PM

Oops, sorry. D25219 was just reviewed and has landed now; can you folks verify whether the problem in Kontact is fixed now?

mlaurent added a comment.Nov 26 2019, 3:55 PM

I updated kconfig and now it's ok. I can't see the "2 kontact" bug.
Thanks

Revision Contents
Changeset List

PathPackages
Msrc/kmainwindow.cpp (6 lines)

Diff 70055

View Options

src/kmainwindow.cpp

Loading...