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

Authored by trmdi on Dec 8 2019, 8:22 AM.

Details

Summary

I'm trying to implement the idea of @davidedmundson from this https://phabricator.kde.org/T10172 with my limited knowledge of C++/Qt/KDE/programming. This is the start of a new class so it could lack many things but it's usable.

  • It allows clients to set the available screen rect/region from outside through dbus.
  • The final available screen rect/region would be the intersection of all set values.
  • When a client dies, its values would be removed automatically.
Test Plan

+ qdbus-qt5 org.kde.plasmashell /StrutManager test org.kde.lattedock LVDS-1 0 24 1366 768
+ After killing latte-dock, the values are got only from plasmashellCorona.

Diff Detail

Repository
R120 Plasma Workspace
Branch
add-otherShellHelper (branched from master)
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 20402
Build 20420: arc lint + arc unit
There are a very large number of changes, so older changes are hidden. Show Older Changes
Restricted Application added a subscriber: plasma-devel. · View Herald TranscriptDec 8 2019, 8:22 AM
trmdi requested review of this revision.Dec 8 2019, 8:22 AM
trmdi edited the summary of this revision. (Show Details)Dec 8 2019, 8:35 AM
trmdi edited the summary of this revision. (Show Details)Dec 8 2019, 8:41 AM

@trmdi this is a big change so it needs first to be approved theoretically from main plasma devs

shell/shellcorona.cpp
1046–1051

I think is missing a "{" here

1047

indent issues for all function

trmdi updated this revision to Diff 71079.Dec 8 2019, 8:46 AM
  • Fix typo
trmdi marked 2 inline comments as done.Dec 8 2019, 8:47 AM
trmdi edited the test plan for this revision. (Show Details)Dec 8 2019, 8:52 AM
trmdi updated this revision to Diff 71391.Dec 12 2019, 6:36 PM
trmdi edited the summary of this revision. (Show Details)Dec 12 2019, 6:48 PM
trmdi updated this revision to Diff 71408.Dec 13 2019, 2:53 AM
  • emit signals when setConnected
trmdi edited the test plan for this revision. (Show Details)Dec 13 2019, 3:04 AM
trmdi updated this revision to Diff 71488.Dec 14 2019, 6:05 AM
  • Improve comment
trmdi added a reviewer: apol.Dec 15 2019, 1:32 AM
trmdi updated this revision to Diff 71594.Dec 15 2019, 12:59 PM
  • No need to setConnected manually
trmdi edited the test plan for this revision. (Show Details)Dec 15 2019, 1:05 PM
trmdi edited the summary of this revision. (Show Details)Dec 15 2019, 1:19 PM
davidedmundson requested changes to this revision.Dec 15 2019, 3:07 PM

Concept is fine.

shell/othershellhelper.cpp
7 ↗(On Diff #71594)

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

20 ↗(On Diff #71594)

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.

42 ↗(On Diff #71594)

int's for screens is not wayland safe.

string would be better

67 ↗(On Diff #71594)

What if 2 clients connect?

Also, please think about handling the connecting client crashing.

shell/othershellhelper.h
12 ↗(On Diff #71594)

The interface should be named after something more specific.

shell/shellcorona.cpp
1073–1074

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

shell/shellcorona.h
124

why are we shadowing this?

This revision now requires changes to proceed.Dec 15 2019, 3:07 PM
trmdi updated this revision to Diff 71647.Dec 16 2019, 5:31 AM
  • Follow David's guide
trmdi updated this revision to Diff 71648.Dec 16 2019, 6:05 AM
trmdi marked 2 inline comments as done.
  • Add a missed line
trmdi edited the summary of this revision. (Show Details)Dec 16 2019, 6:14 AM
trmdi edited the test plan for this revision. (Show Details)
trmdi added a comment.Dec 16 2019, 6:27 AM

@davidedmundson

Concerning the type of screenId should be QString, I think it need another patch, because Plasma::Corona's availableScreenRect also requires that type: https://api.kde.org/frameworks/plasma-framework/html/corona_8cpp_source.html#l00331

trmdi updated this revision to Diff 71649.Dec 16 2019, 6:40 AM
  • Add another missed line

Much nicer!

Re: string Vs int

It can be ints within plasmashell, it just needs to be strings over the dbus protocol.

The problem is that on wayland (and to some extent x11 is racey) the order of screens within QApplication::screens might not be in sync across processes.

trmdi updated this revision to Diff 71650.Dec 16 2019, 8:07 AM
  • Use QString type for the dbus screenId parameters
trmdi edited the test plan for this revision. (Show Details)Dec 16 2019, 8:23 AM
trmdi edited the summary of this revision. (Show Details)Dec 17 2019, 3:09 AM

I have a question about naming. If I had an application like panel or latte I would call setAvailableScreenRect? That seems kinda backwards to me? That part of the screen is not available isn't it but rather not available anymore? Maybe I'm understanding the approach taken here wrong?

trmdi added a comment.EditedDec 17 2019, 9:08 AM

I have a question about naming. If I had an application like panel or latte I would call setAvailableScreenRect? That seems kinda backwards to me? That part of the screen is not available isn't it but rather not available anymore? Maybe I'm understanding the approach taken here wrong?

availableScreenRect = screen_geometry - panels

https://api.kde.org/frameworks/plasma-framework/html/classPlasma_1_1Corona.html#ad8ffd115128a128efd830540eeba5dd1

The approach here is we have availablescreenrect of plasmashell, then latte send another one. The final result is the intersection of all availablescreenrects.

I have a question about naming. If I had an application like panel or latte I would call setAvailableScreenRect? That seems kinda backwards to me? That part of the screen is not available isn't it but rather not available anymore? Maybe I'm understanding the approach taken here wrong?

availableScreenRect = screen_geometry - panels

https://api.kde.org/frameworks/plasma-framework/html/classPlasma_1_1Corona.html#ad8ffd115128a128efd830540eeba5dd1

The approach here is we have availablescreenrect of plasmashell, then latte send another one. The final result is the intersection of all availablescreenrects.

Thanks for the explanation! That makes it clearer to me.

trmdi updated this revision to Diff 71802.Dec 18 2019, 4:38 PM
  • Remove unneeded code
trmdi added a comment.EditedDec 21 2019, 4:38 PM

The problem is that on wayland (and to some extent x11 is racey) the order of screens within QApplication::screens might not be in sync across processes.

Is it possible to handle this? Are the name property of screens always unique ? Can the external apps like latte be responsible for this?

trmdi added a comment.EditedDec 21 2019, 5:13 PM

@mvourlakos
Latte can guard that the order of screens within Latte is synced with plasmashell right? Because I see this: https://github.com/KDE/latte-dock/blob/master/app/screenpool.cpp

@mvourlakos
Latte can guard that the order of screens within Latte is synced with plasmashell right? Because I see this: https://github.com/KDE/latte-dock/blob/master/app/screenpool.cpp

why the screens order is relevant in this patch? the external application isnt just using the screen name?

trmdi added a comment.Dec 21 2019, 6:17 PM

@mvourlakos
Latte can guard that the order of screens within Latte is synced with plasmashell right? Because I see this: https://github.com/KDE/latte-dock/blob/master/app/screenpool.cpp

why the screens order is relevant in this patch? the external application isnt just using the screen name?

Because I don't know which one latte could use for setting the availablescreenrect. Currently I use id, but David said that the id might be not synced between processes.
So you mean I should use screen name instead? The screen names are the same within latte vs plasmashell right?

@mvourlakos
Latte can guard that the order of screens within Latte is synced with plasmashell right? Because I see this: https://github.com/KDE/latte-dock/blob/master/app/screenpool.cpp

why the screens order is relevant in this patch? the external application isnt just using the screen name?

Because I don't know which one latte could use for setting the availablescreenrect. Currently I use id, but David said that the id might be not synced between processes.
So you mean I should use screen name instead? The screen names are the same within latte vs plasmashell right?

Even though Latte understands the plasma screen ids and can use them, I dont think the external apps should have that burden. The screenName is enough from my understanding.

trmdi updated this revision to Diff 71979.Dec 22 2019, 6:09 AM
  • Use screenName instead of id in setAvailableScreenRect/Region()
trmdi edited the test plan for this revision. (Show Details)Dec 22 2019, 6:11 AM
trmdi updated this revision to Diff 71980.Dec 22 2019, 6:27 AM
mvourlakos added inline comments.Dec 22 2019, 7:12 AM
shell/shellcorona.h
94

maybe concerning naming, plasmaAvailableScreenRegion would be better

95

maybe concerning naming, plasmaAvailableScreenRect would be better

davidedmundson added inline comments.Dec 29 2019, 2:26 PM
shell/strutmanager.h
23

(const QString &service, const QString &screenName, const QRect &rect);

saves some (shallow) copies

26

what's this about?

trmdi added inline comments.Dec 29 2019, 2:28 PM
shell/strutmanager.h
26

This is just for easily testing using the qdbus command.

trmdi updated this revision to Diff 72384.Dec 30 2019, 2:52 PM
  • Use const reference for some parameters
trmdi marked an inline comment as done.Dec 30 2019, 2:53 PM
davidedmundson accepted this revision.Dec 30 2019, 3:48 PM

maybe drop the ::test method when merging

This revision is now accepted and ready to land.Dec 30 2019, 3:48 PM
trmdi updated this revision to Diff 72387.Dec 30 2019, 3:51 PM
  • Drop the test method
This revision was automatically updated to reflect the committed changes.
trmdi added a comment.Dec 30 2019, 4:05 PM

Thank you so much @davidedmundson!

@mvourlakos: I think you can improve Latte now. :)

Thank you so much @davidedmundson!

@mvourlakos: I think you can improve Latte now. :)

of course!