Refactor JobUiDelegateExtension to be async
Open, Needs TriagePublic

Description

askFileRename, askSkip, askDeleteConfirmation and requestMessageBox should be async to avoid event-loop re-entrancy (see https://bugs.kde.org/show_bug.cgi?id=409607#c3). Each one could be a KJob, that either emits a signal with the return value, or emits result() and has a getter for the return value.

dfaure created this task.Nov 23 2019, 7:46 PM
meven added a subscriber: meven.Nov 25 2019, 11:02 AM

Could we add those jobs in KF5 and mark the old deprecate class JobUiDelegateExtension deprecated, candidate for KF6 cleanup ?

Yes. A lot of these tasks can start already.

dfaure added a comment.Sep 8 2020, 6:24 PM

More details:

Currently jobuidelegateextension.h defines virtual KIO::RenameDialog_Result askFileRename(KJob *job, [....]) = 0;
Instead, we would need an interface with an async result. Initially I was thinking of 4 jobs, but it's hard to have jobs in kiocore implemented in kiowidgets.
Instead, we could just do like with e.g. UntrustedProgramHandlerInterface: writing a new interface with 4 virtual methods and 4 signals (the "return type" is different for each), and implementing that interface in kiowidgets. I don't think we want 4 interfaces, that would be overkill. One wants widgets, QML [or a core-only impl in unittests] for all of these operations together.

dfaure renamed this task from Refactor JobUiDelegateExtension to be 4 jobs to Refactor JobUiDelegateExtension to be async.Sep 8 2020, 6:25 PM

Looking into this, it looks like askDeleteConfirmation and requestMessageBox use KMessageBox internally, so they're already sort of async, right?

I ended up implementing these two methods:
askUserFileRename (askFileRename)
askUserSkip (askSkip)

See: https://invent.kde.org/frameworks/kio/-/merge_requests/203

dfaure added a comment.Nov 1 2020, 8:48 PM

KMessageBox::warning[...], which ends up calling QDialog::exec(), is very much "sync" API and very much an open door to event loop re-entrancy. They have the same issue as the other two.

Hmm, I grep'ed for exec/open/show in KMessageBox sources, but didn't find any, I probably should have used gdb with a breakpoint on exec; I'll look into that next (I was surprised to see kmessagebox.h has 1000+ lines, all those convenience functions :)).

kwidgetsaddons/src/kmessagebox.cpp line 374:

const QDialogButtonBox::StandardButton result = QDialogButtonBox::StandardButton(guardedDialog->exec());

I looked again after your post (the one before last), and indeed it was there I just didn't make the connection...

(I dug some more and found an example to fix this, a commit in kdelibs to make knewfilemenu use async kmessagebox's).

(Also I think the documentation needs some polishing, possibly by adding a code example, which as we know is very useful copy/paste material :)).

For anyone following this, this has caused some work to have an async-centric variant of KMessageBox: https://invent.kde.org/frameworks/kwidgetsaddons/-/merge_requests/27

ahmadsamir moved this task from In Progress to Done on the KF6 board.Dec 28 2020, 8:41 PM

Just to keep this updated, the interface has been implemented and we're working on porting the, many, KIO Jobs to it.