D16602: [libkwineffects] Replace property name lookup with calling the virtual methods

zzag added a comment.


  \o/
  
  For the record:  a348df0d3449d311ae8ba9152c47b4df920bf10d <https://phabricator.kde.org/R108:a348df0d3449d311ae8ba9152c47b4df920bf10d>.

INLINE COMMENTS

> test_window_paint_data.cpp:178
> +    }
> +    QRect contentsRect() const {
> +        return QRect();

Missing override.

> effects.cpp:1735
> +
> +//windowClass
> +QString EffectWindowImpl::windowClass() const

We probably don't need this comment.

> effects.cpp:1775
> +{
> +    if (auto client = qobject_cast<Client*>(toplevel)){
> +        return client->basicUnit();

Missing whitespace between the type and "*".

> effects.h:358
>      explicit EffectWindowImpl(Toplevel *toplevel);
> -    virtual ~EffectWindowImpl();
> +    virtual ~EffectWindowImpl() override;
>  

Please remove "virtual".

> effects.h:434
> +
> +    const EffectWindowGroup* group() const override;
> +

Missing whitespace between the type and "*".

> toplevel.h:464
> +     * @see captionSuffix
> +    **/
> +    virtual QString caption() const;

It's off by one space.

> toplevel.h:467-471
> +    virtual bool isMinimized() const;
> +    virtual bool isModal() const;
> +    virtual bool isFullScreen() const;
> +    virtual bool keepAbove() const;
> +    virtual bool keepBelow() const;

Can you please add comments for these too?
For example, that's not really clear what "keepAbove" means. One could think that's an action.

> toplevel.h:472-476
> +    /*
> +    *   When a click is done in the decoration and it calls the group
> +    *   to change the visible client it starts to move-resize the new
> +    *   client, this function stops it.
> +    */

Please re-format it.

REPOSITORY
  R108 KWin

REVISION DETAIL
  https://phabricator.kde.org/D16602

To: davidedmundson, #kwin
Cc: zzag, broulik, kwin, mkulinski, ragreen, jackyalcine, Pitel, iodelay, bwowk, ZrenBot, ngraham, lesliezhai, ali-mohamed, hardening, jensreuterberg, abetts, sebas, apol, mart