[Differential] [Commented On] D3723: Introduce a KWin internal on-screen-notification service

[davidedmundson (David Edmundson)]

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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/plasma-devel/attachments/20161219/2578c980/attachment.html>