Create Task
Maniphest T13140

Allow KCModule::save() to be async
Open, Needs TriagePublic
Actions

Description

Right now, save() has to be synchronous for the following scenarios:

  • kcmshell5, user clicks OK, it calls save() and quits right away
  • System Settings when switching modules with unsaved changes, will call save() and destroy the KCM

This can be undesirable especially for situations where DBus calls are made and potentially cause PolKit prompts.

There should be a saveDone() signal or similar the KCM can emit to signal when it's done saving settings? Perhaps with a setDelayed.. so simple save() is still possible easily. Or maybe it should be a SaveJob? :D

Related Objects
Search...

broulik created this task.May 11 2020, 12:50 PM
alex claimed this task.Mar 30 2021, 7:24 AM
alex added a subscriber: alex.

Currently systemsettings5 does not close right away, but the KCM is still immediately deleted when one switching modules and applying unselected changes and because we use now systemsettings5 by default to open the KCMs this is not as pressing as before.

I am not sure if we just want this to be another boolean value or maybe some more extendable enum which has states like Defaulted, NeedsSaving, IsSaving. This could come in handy if we want to have a saving/loading indicator which is provided by the frameworks.

Just some thoughts, this way we would not need to introduce public API which has to be cleaned up in KF6 anyways.

davidre added a subscriber: davidre.Apr 2 2021, 2:23 PM

That sounds like something like that could make sense indeed. Not sure if just a signal saved would be enough instead of assuming after save() returns that everything will be saved. But as you said an enum is extendable.

alex added a comment.May 6 2021, 5:34 PM

There have been other tasks about creating a common base class for both QML and QWidgets KCMs and moving classed in project. To not cause major compatibility issues I would take back my suggestions of making it an enum and will settle for an a property/signal instead.

alex moved this task from Backlog to In Progress on the KF6 board.May 8 2021, 1:05 PM
alex added a comment.Jul 1 2021, 11:23 AM

@broulik I thought about this and we basically need to check if the a property is set and if yes we don't immediately delete the KCM. I did some basic implementation of that, but it all feels really hacks, because we don't have a common base class for the QWidgets and QML KCMs, so one has to basically implement the property twice.

Maybe we could revisit this idea when KF6 is branched and some of the KCMUtils stuff figured out?

In the keys KCM we currently do all of the saving synchronously in a row, if that is a problem we could maybe start all the DBus calls and then wait for all of them to finish. This way the calls are at least parallel. cc @davidre

davidre added a comment.Jul 1 2021, 11:29 AM

Yeah could make sense, the saving was in fact async and then just made all the calls in saving sync to avoid not saving settings
https://invent.kde.org/plasma/plasma-desktop/-/commit/fcb04768142f6fbc4173f45516decf1182f4a206

But I don#t think it's a huge problem at the moment, at least to me there are other kcms that feel laggier

alex moved this task from In Progress to Waiting on KF6 Branching on the KF6 board.Jul 1 2021, 12:14 PM

Okay, then we can look after KF6 branching and the other tasks if this needs doing or should be closed as invalid.