Introduce KNotificationJobUiDelegate
ClosedPublic

Authored by broulik on Mar 26 2020, 8:27 AM.

Details

Summary

This can be used for job error reporting using KNotification rather than a KDialog.

Test Plan

As discussed in D28286

auto *job = new KIO::ApplicationLauncherJob(service);
auto *delegate = new KNotificationJobUiDelegate;
delegate->setAutoErrorHandlingEnabled(true);
job->setUiDelegate(delegate);
job->start();

Is there a way to get the job description/title outside of its description emission? I kinda would like to show the job title in the notification but I don't see how.

Diff Detail

Repository
R289 KNotifications
Lint
Lint Skipped
Unit
Unit Tests Skipped
broulik created this revision.Mar 26 2020, 8:27 AM
Restricted Application added a project: Frameworks. · View Herald TranscriptMar 26 2020, 8:27 AM
Restricted Application added a subscriber: kde-frameworks-devel. · View Herald Transcript
broulik requested review of this revision.Mar 26 2020, 8:27 AM
nicolasfella added inline comments.Mar 27 2020, 12:25 PM
src/knotificationjobuidelegate.h
29

s/KMessageBox/KNotification/g?

broulik updated this revision to Diff 78644.Mar 27 2020, 1:34 PM
  • Fix docs
nicolasfella accepted this revision.Mar 27 2020, 1:37 PM

Otherwise looks good to me

src/knotificationjobuidelegate.h
29

@since

This revision is now accepted and ready to land.Mar 27 2020, 1:37 PM
dfaure requested changes to this revision.Mar 28 2020, 9:28 AM

Indeed missing @since 5.69

To get the description, just connect to the description signal and store it, until the time you need to show the error?

src/knotificationjobuidelegate.cpp
33

remove?

This revision now requires changes to proceed.Mar 28 2020, 9:28 AM
kossebau added inline comments.
src/knotificationjobuidelegate.cpp
25

Not using a nested class Private but a normal separate one like here to be named KNotificationJobUiDelegatePrivate seems more simple, avoids the need for things like Q_DECL_HIDDEN.

At least recently most code uses the latter, so would be good to standardize on that.

kossebau added inline comments.Mar 28 2020, 9:44 AM
src/knotificationjobuidelegate.h
59

Using QScopedPointer or std::unique_ptr instead of raw pointer also avoids need to manually delete in destructor, also better practice these days.

broulik planned changes to this revision.Mar 29 2020, 10:34 AM
broulik added inline comments.
src/knotificationjobuidelegate.cpp
25

I thought we wanted to migrate towards nested Private class? :0

dfaure added inline comments.Mar 29 2020, 10:43 AM
src/knotificationjobuidelegate.cpp
25

I don't remember any past discussion about this, but as I discovered in KIO commit 3d2330968b, nested Private classes have the problem that you can't forward-declare them elsewhere in order to make them friends of another class.

What would be the arguments in favour of nested Private classes?
All I see is the risk of forgetting Q_DECL_HIDDEN, incompatibility with Q_D/Q_Q macros and more generally, inconsistency with Qt, plus the friend problem above.

broulik added a comment.EditedMar 30 2020, 7:17 AM

To get the description, just connect to the description signal and store it, until the time you need to show the error?

Should the new app start jobs have one, maybe?

broulik marked 5 inline comments as done.Mar 30 2020, 7:19 AM
broulik updated this revision to Diff 78839.Mar 30 2020, 7:27 AM
  • Use QScopedPointer
  • No nested private class
  • Store description and use it as notification title

Is this good now? But I think I'll wait until after tagging, it's not really urgent to get it in

dfaure accepted this revision.Apr 2 2020, 7:59 AM

I'm OK with this landing into 5.69

Feel free to emit a description from the two new jobs.

Thanks,
David.

This revision is now accepted and ready to land.Apr 2 2020, 7:59 AM
This revision was automatically updated to reflect the committed changes.
ahmadsamir added inline comments.
src/knotificationjobuidelegate.cpp
25

Someone should update the part about the nested Private classes https://community.kde.org/Policies/Binary_Compatibility_Issues_With_C++#Using_a_d-Pointer

Done, thanks for the pointer.