D28295: Introduce KNotificationJobUiDelegate

dfaure added a comment. Done, thanks for the pointer. REPOSITORY R289 KNotifications REVISION DETAIL https://phabricator.kde.org/D28295 To: broulik, #frameworks, nicolasfella, dfaure Cc: ahmadsamir, kossebau, kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, bruns

D28295: Introduce KNotificationJobUiDelegate

ahmadsamir added inline comments. INLINE COMMENTS > dfaure wrote in knotificationjobuidelegate.cpp:24 > I don't remember any past discussion about this, but as I discovered in KIO > commit 3d2330968b > , > nested Privat

D28295: Introduce KNotificationJobUiDelegate

This revision was automatically updated to reflect the committed changes. Closed by commit R289:3dafde9c61b9: Introduce KNotificationJobUiDelegate (authored by broulik). REPOSITORY R289 KNotifications CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D28295?vs=78839&id=79104 REVISION DE

D28295: Introduce KNotificationJobUiDelegate

dfaure accepted this revision. dfaure added a comment. This revision is now accepted and ready to land. I'm OK with this landing into 5.69 Feel free to emit a description from the two new jobs. Thanks, David. REPOSITORY R289 KNotifications REVISION DETAIL https://phabricator.k

D28295: Introduce KNotificationJobUiDelegate

broulik added a comment. Is this good now? But I think I'll wait until after tagging, it's not really urgent to get it in REPOSITORY R289 KNotifications REVISION DETAIL https://phabricator.kde.org/D28295 To: broulik, #frameworks, nicolasfella, dfaure Cc: kossebau, kde-frameworks-devel,

D28295: Introduce KNotificationJobUiDelegate

broulik updated this revision to Diff 78839. broulik added a comment. - Use `QScopedPointer` - No nested private class - Store `description` and use it as notification title REPOSITORY R289 KNotifications CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D28295?vs=78644&id=78839

D28295: Introduce KNotificationJobUiDelegate

broulik marked 5 inline comments as done. REPOSITORY R289 KNotifications REVISION DETAIL https://phabricator.kde.org/D28295 To: broulik, #frameworks, nicolasfella, dfaure Cc: kossebau, kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, bruns

D28295: Introduce KNotificationJobUiDelegate

broulik added a comment. > To get the description, just connect to the description signal and store it, until the time you need to show the error? Should the jew app start jobs have one, maybe? REPOSITORY R289 KNotifications REVISION DETAIL https://phabricator.kde.org/D28295 To: br

D28295: Introduce KNotificationJobUiDelegate

dfaure added inline comments. INLINE COMMENTS > broulik wrote in knotificationjobuidelegate.cpp:24 > I thought we wanted to migrate towards nested `Private` class? :0 I don't remember any past discussion about this, but as I discovered in KIO commit 3d2330968b

D28295: Introduce KNotificationJobUiDelegate

broulik planned changes to this revision. broulik added inline comments. INLINE COMMENTS > kossebau wrote in knotificationjobuidelegate.cpp:24 > Not using a nested class `Private` but a normal separate one like here to be > named `KNotificationJobUiDelegatePrivate` seems more simple, avoids the

D28295: Introduce KNotificationJobUiDelegate

kossebau added inline comments. INLINE COMMENTS > knotificationjobuidelegate.h:58 > +class Private; > +Private *const d; > +}; Using QScopedPointer or std::unique_ptr instead of raw pointer also avoids need to manually delete in destructor, also better practice these days. REPOSITORY

D28295: Introduce KNotificationJobUiDelegate

kossebau added inline comments. INLINE COMMENTS > knotificationjobuidelegate.cpp:24 > + > +class Q_DECL_HIDDEN KNotificationJobUiDelegate::Private > +{ Not using a nested class `Private` but a normal separate one like here to be named `KNotificationJobUiDelegatePrivate` seems more simple, avoid

D28295: Introduce KNotificationJobUiDelegate

dfaure requested changes to this revision. dfaure added a comment. This revision now requires changes to proceed. 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? INLINE COMMENTS > knotif

D28295: Introduce KNotificationJobUiDelegate

broulik added a dependent revision: D28347: Port services and shell runner away from KRun. REPOSITORY R289 KNotifications REVISION DETAIL https://phabricator.kde.org/D28295 To: broulik, #frameworks, nicolasfella, dfaure Cc: kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, b

D28295: Introduce KNotificationJobUiDelegate

nicolasfella accepted this revision. nicolasfella added a comment. This revision is now accepted and ready to land. Otherwise looks good to me INLINE COMMENTS > knotificationjobuidelegate.h:28 > + * @class KNotificationJobUiDelegate knotificationjobuidelegate.h > KNotificationJobUiDelegate >

D28295: Introduce KNotificationJobUiDelegate

broulik updated this revision to Diff 78644. broulik added a comment. - Fix docs REPOSITORY R289 KNotifications CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D28295?vs=78520&id=78644 REVISION DETAIL https://phabricator.kde.org/D28295 AFFECTED FILES src/CMakeLists.txt src/k

D28295: Introduce KNotificationJobUiDelegate

nicolasfella added inline comments. INLINE COMMENTS > knotificationjobuidelegate.h:29 > + * > + * A UI delegate using KMessageBox for interaction (showing errors and > warnings). > + * s/KMessageBox/KNotification/g? REPOSITORY R289 KNotifications REVISION DETAIL https://phabricator.kde.or

D28295: Introduce KNotificationJobUiDelegate

broulik created this revision. broulik added reviewers: Frameworks, nicolasfella, dfaure. Herald added a project: Frameworks. Herald added a subscriber: kde-frameworks-devel. broulik requested review of this revision. REVISION SUMMARY This can be used for job error reporting using `KNotification