D25807: Allow to set the available screen rect/region from outside through dbus

[David Edmundson]

davidedmundson requested changes to this revision.
davidedmundson added a comment.
This revision now requires changes to proceed.


  Concept is fine.

INLINE COMMENTS

> othershellhelper.cpp:7
> +
> +OtherShellHelper::OtherShellHelper(ShellCorona *plasmashellCorona) : QObject(qobject_cast<QObject *>(plasmashellCorona)),
> +    m_plasmashellCorona(plasmashellCorona),

Ideally we want the ClassName to focus on what something does, rather than what the usage happens to be.

i.e
StrutManager or some such

> othershellhelper.cpp:20
> +
> +QByteArray OtherShellHelper::availableScreenRegion(int id, bool plasmashell) const
> +{

A QRegion is a list of QRects.

You should be able to do

QList<QRect>

It might work out the box, it might need: https://techbase.kde.org/Development/Tutorials/D-Bus/CustomTypes

Using a QByteArray is a bit meh.

> othershellhelper.cpp:42
> +
> +void OtherShellHelper::setAvailableScreenRegion(int screenId, QByteArray region) {
> +    QRegion r;

int's for screens is not wayland safe.

string would be better

> othershellhelper.cpp:67
> +
> +    m_connected = connected;
> +

What if 2 clients connect?

Also, please think about handling the connecting client crashing.

> othershellhelper.h:12
> +    Q_OBJECT
> +    Q_CLASSINFO("D-Bus Interface","org.kde.plasmashell")
> +

The interface should be named after something more specific.

> shellcorona.cpp:1091-1092
> +{
> +    if (m_otherShellHelper->connected() && !m_otherShellHelper->availableScreenRect(id).isNull()) {
> +        return m_otherShellHelper->availableScreenRect(id);
> +    }

Why not a union? One can have a panel and latte?

> shellcorona.h:125
>      void glInitializationFailed();
> +    void _availableScreenRectChanged();
> +    void _availableScreenRegionChanged();

why are we shadowing this?

REPOSITORY
  R120 Plasma Workspace

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

To: trmdi, #plasma, mvourlakos, davidedmundson, mart, apol
Cc: plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, alexeymin, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/plasma-devel/attachments/20191215/e2adb6c3/attachment-0001.html>