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
