davidedmundson added a comment.
looks generally good.
1 nitpick about QML icon usage, and one question.
INLINE COMMENTS
> onscreennotification.h:47
> + explicit OnScreenNotification(QObject *parent = nullptr);
> + virtual ~OnScreenNotification();
> + bool isVisible() const;
Not an actual issue, but technically, this isn't a new virtual, but an override
> onscreennotification.h:78
> + QScopedPointer<QQmlComponent> m_qmlComponent;
> + QPointer<QQmlEngine> m_qmlEngine;
> + QScopedPointer<QObject> m_mainItem;
why QPointer?
If this were to get deleted externally, you're still left with a QQmlContext
with a dangly pointer internally and a crash if it's used.
> osd.cpp:73
> +{
> + if (!kwinApp()->shouldUseWaylandForCompositing()) {
> + // FIXME: only supported on Wayland
I don't get why this line exists, you can render an internal window on either
platform..
but if it should exist, why it is not put it for show too?
> main.qml:38
> + mainItem: RowLayout {
> + KQuickControlsAddons.QIconItem {
> + Layout.minimumWidth: 64
If you're rendering onto a Plasma Dialog, it should be a Plasma Icon
otherwise you have a potentially white icon on white background situation.
REPOSITORY
R108 KWin
REVISION DETAIL
https://phabricator.kde.org/D3723
EMAIL PREFERENCES
https://phabricator.kde.org/settings/panel/emailpreferences/
To: graesslin, #kwin, #plasma
Cc: davidedmundson, plasma-devel, kwin, lesliezhai, ali-mohamed, hardening,
jensreuterberg, abetts, sebas
