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.
Description
Status | Assigned | Task | ||
---|---|---|---|---|
Open | None | T12089 KIO for KF6 | ||
Open | ahmadsamir | T12193 Refactor JobUiDelegateExtension to be async |
Could we add those jobs in KF5 and mark the old deprecate class JobUiDelegateExtension deprecated, candidate for KF6 cleanup ?
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.
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
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
Just to keep this updated, the interface has been implemented and we're working on porting the, many, KIO Jobs to it.