broulik added inline comments.
INLINE COMMENTS
> screencaststream.cpp:372
> + const auto emitFailureNotification = [](const QString &body) {
> + KNotification *notification = new
> KNotification(QStringLiteral("notification"), KNotification::CloseOnTimeout |
> KNotification::DefaultEvent);
> + notification->setTitle(i18n("Failed to start screencasting"));
This should be its own event, not abusing the "notification" one
> screencaststream.cpp:375
> + notification->setText(i18nc("Introduces an error message", "Error:
> %1", body));
> + notification->setIconName(QStringLiteral("data-error"));
> + notification->setUrgency(KNotification::HighUrgency);
You probably want `dialog-error`
> screencaststream.cpp:376
> + notification->setIconName(QStringLiteral("data-error"));
> + notification->setUrgency(KNotification::HighUrgency);
> + notification->sendEvent();
You sure this is needed to be high?
> ngraham wrote in screencaststream.cpp:374
> `.arg` is unnecessary and in various cases can cause i18n() errors; just do
> it inline: `notification->setText(i18n("Error: %1", body));`
>
> Also for short strings like this, it's nice to add some context for
> translators.
Is the "Error" even needed?
REPOSITORY
R838 Flatpak Support: KDE Portal for XDG Desktop
REVISION DETAIL
https://phabricator.kde.org/D28517
To: cblack, #plasma, #vdg, ngraham, apol
Cc: broulik, apol, ngraham, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev,
cblack, jraleigh, zachus, fbampaloukas, GB_2, ragreen, ZrenBot, himcesjf,
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, ahiemstra, mart
