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

[Michail Vourlakos]

mvourlakos added a comment.


  In D25807#584697 , @trmdi wrote:
  
  > Thank you so much @davidedmundson!
  >
  > @mvourlakos: I think you can improve Latte now. :)
  
  
  of course!

REPOSITORY
  R120 Plasma Workspace

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

To: trmdi, #plasma, mvourlakos, davidedmundson, mart, apol
Cc: davidre, plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, zachus, 
fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, alexeymin, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

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

[Tranter Madi]

trmdi added a comment.


  Thank you so much @davidedmundson!
  
  @mvourlakos: I think you can improve Latte now. :)

REPOSITORY
  R120 Plasma Workspace

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

To: trmdi, #plasma, mvourlakos, davidedmundson, mart, apol
Cc: davidre, plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, zachus, 
fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, alexeymin, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

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

[Tranter Madi]

This revision was automatically updated to reflect the committed changes.
Closed by commit R120:e8e360862fb7: Allow to set the available screen 
rect/region from outside through dbus (authored by trmdi).

REPOSITORY
  R120 Plasma Workspace

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D25807?vs=72387=72388

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

AFFECTED FILES
  shell/CMakeLists.txt
  shell/shellcorona.cpp
  shell/shellcorona.h
  shell/strutmanager.cpp
  shell/strutmanager.h

To: trmdi, #plasma, mvourlakos, davidedmundson, mart, apol
Cc: davidre, plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, zachus, 
fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, alexeymin, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

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

[Tranter Madi]

trmdi updated this revision to Diff 72387.
trmdi added a comment.


  - Drop the test method

REPOSITORY
  R120 Plasma Workspace

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D25807?vs=72384=72387

BRANCH
  add-otherShellHelper (branched from master)

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

AFFECTED FILES
  shell/CMakeLists.txt
  shell/shellcorona.cpp
  shell/shellcorona.h
  shell/strutmanager.cpp
  shell/strutmanager.h

To: trmdi, #plasma, mvourlakos, davidedmundson, mart, apol
Cc: davidre, plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, zachus, 
fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, alexeymin, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

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

[David Edmundson]

davidedmundson accepted this revision.
davidedmundson added a comment.
This revision is now accepted and ready to land.


  maybe drop the ::test method when merging

REPOSITORY
  R120 Plasma Workspace

BRANCH
  add-otherShellHelper (branched from master)

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

To: trmdi, #plasma, mvourlakos, davidedmundson, mart, apol
Cc: davidre, plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, zachus, 
fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, alexeymin, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

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

[Tranter Madi]

trmdi updated this revision to Diff 72384.
trmdi added a comment.


  - Use const reference for some parameters

REPOSITORY
  R120 Plasma Workspace

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D25807?vs=71980=72384

BRANCH
  add-otherShellHelper (branched from master)

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

AFFECTED FILES
  shell/CMakeLists.txt
  shell/shellcorona.cpp
  shell/shellcorona.h
  shell/strutmanager.cpp
  shell/strutmanager.h

To: trmdi, #plasma, mvourlakos, davidedmundson, mart, apol
Cc: davidre, plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, zachus, 
fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, alexeymin, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

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

[Tranter Madi]

trmdi added inline comments.

INLINE COMMENTS

> davidedmundson wrote in strutmanager.h:25
> what's this about?

This is just for easily testing using the qdbus command.

REPOSITORY
  R120 Plasma Workspace

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

To: trmdi, #plasma, mvourlakos, davidedmundson, mart, apol
Cc: davidre, plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, zachus, 
fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, alexeymin, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

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

[David Edmundson]

davidedmundson added inline comments.

INLINE COMMENTS

> strutmanager.h:22
> +public Q_SLOTS:
> +void setAvailableScreenRect(QString service, QString screenName, 
> QRect rect);
> +void setAvailableScreenRegion(QString service, QString screenName, 
> QList rects);

(const QString , const QString , const QRect );

saves some (shallow) copies

> strutmanager.h:25
> +
> +void test(QString service, QString screenName, int x, int y, int 
> width, int height);
> +

what's this about?

REPOSITORY
  R120 Plasma Workspace

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

To: trmdi, #plasma, mvourlakos, davidedmundson, mart, apol
Cc: davidre, plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, zachus, 
fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, alexeymin, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

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

[Tranter Madi]

trmdi added a comment.


  Ping!

REPOSITORY
  R120 Plasma Workspace

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

To: trmdi, #plasma, mvourlakos, davidedmundson, mart, apol
Cc: davidre, plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, zachus, 
fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, alexeymin, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

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

[Michail Vourlakos]

mvourlakos added inline comments.

INLINE COMMENTS

> shellcorona.h:94
> +// plasmashellCorona's value
> +QRegion _availableScreenRegion(int id) const;
> +QRect _availableScreenRect(int id) const;

maybe concerning naming, plasmaAvailableScreenRegion would be better

> shellcorona.h:95
> +QRegion _availableScreenRegion(int id) const;
> +QRect _availableScreenRect(int id) const;
> +

maybe concerning naming, plasmaAvailableScreenRect would be better

REPOSITORY
  R120 Plasma Workspace

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

To: trmdi, #plasma, mvourlakos, davidedmundson, mart, apol
Cc: davidre, plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, zachus, 
fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, alexeymin, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

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

[Tranter Madi]

trmdi updated this revision to Diff 71980.
trmdi added a comment.


  - Merge branch 'master' of https://anongit.kde.org/plasma-workspace into 
add-otherShellHelper
  - Just rename the screenId parameter of test()

REPOSITORY
  R120 Plasma Workspace

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D25807?vs=71979=71980

BRANCH
  add-otherShellHelper (branched from master)

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

AFFECTED FILES
  shell/CMakeLists.txt
  shell/shellcorona.cpp
  shell/shellcorona.h
  shell/strutmanager.cpp
  shell/strutmanager.h

To: trmdi, #plasma, mvourlakos, davidedmundson, mart, apol
Cc: davidre, plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, zachus, 
fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, alexeymin, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

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

[Tranter Madi]

trmdi updated this revision to Diff 71979.
trmdi added a comment.


  - Use screenName instead of id in setAvailableScreenRect/Region()

REPOSITORY
  R120 Plasma Workspace

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D25807?vs=71802=71979

BRANCH
  add-otherShellHelper (branched from master)

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

AFFECTED FILES
  shell/CMakeLists.txt
  shell/shellcorona.cpp
  shell/shellcorona.h
  shell/strutmanager.cpp
  shell/strutmanager.h

To: trmdi, #plasma, mvourlakos, davidedmundson, mart, apol
Cc: davidre, plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, zachus, 
fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, alexeymin, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

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

[Michail Vourlakos]

mvourlakos added a comment.


  In D25807#581178 , @trmdi wrote:
  
  > In D25807#581161 , @mvourlakos 
wrote:
  >
  > > In D25807#581126 , @trmdi 
wrote:
  > >
  > > > @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.

REPOSITORY
  R120 Plasma Workspace

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

To: trmdi, #plasma, mvourlakos, davidedmundson, mart, apol
Cc: davidre, plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, zachus, 
fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, alexeymin, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

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

[Tranter Madi]

trmdi added a comment.


  In D25807#581161 , @mvourlakos 
wrote:
  
  > In D25807#581126 , @trmdi wrote:
  >
  > > @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?

REPOSITORY
  R120 Plasma Workspace

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

To: trmdi, #plasma, mvourlakos, davidedmundson, mart, apol
Cc: davidre, plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, zachus, 
fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, alexeymin, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

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

[Michail Vourlakos]

mvourlakos added a comment.


  In D25807#581126 , @trmdi wrote:
  
  > @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?

REPOSITORY
  R120 Plasma Workspace

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

To: trmdi, #plasma, mvourlakos, davidedmundson, mart, apol
Cc: davidre, plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, zachus, 
fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, alexeymin, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

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

[Tranter Madi]

trmdi added a comment.


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

REPOSITORY
  R120 Plasma Workspace

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

To: trmdi, #plasma, mvourlakos, davidedmundson, mart, apol
Cc: davidre, plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, zachus, 
fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, alexeymin, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

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

[Tranter Madi]

trmdi added a comment.


  In D25807#578640 , @davidedmundson 
wrote:
  
  > 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?

REPOSITORY
  R120 Plasma Workspace

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

To: trmdi, #plasma, mvourlakos, davidedmundson, mart, apol
Cc: davidre, plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, zachus, 
fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, alexeymin, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

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

[Tranter Madi]

trmdi updated this revision to Diff 71802.
trmdi added a comment.


  - Remove unneeded code

REPOSITORY
  R120 Plasma Workspace

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D25807?vs=71650=71802

BRANCH
  add-otherShellHelper (branched from master)

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

AFFECTED FILES
  shell/CMakeLists.txt
  shell/shellcorona.cpp
  shell/shellcorona.h
  shell/strutmanager.cpp
  shell/strutmanager.h

To: trmdi, #plasma, mvourlakos, davidedmundson, mart, apol
Cc: davidre, plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, zachus, 
fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, alexeymin, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

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

[David Redondo]

davidre added a comment.


  In D25807#579144 , @trmdi wrote:
  
  > In D25807#579125 , @davidre 
wrote:
  >
  > > 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.

REPOSITORY
  R120 Plasma Workspace

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

To: trmdi, #plasma, mvourlakos, davidedmundson, mart, apol
Cc: davidre, plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, zachus, 
fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, alexeymin, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

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

[Tranter Madi]

trmdi added a comment.


  In D25807#579125 , @davidre wrote:
  
  > 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

REPOSITORY
  R120 Plasma Workspace

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

To: trmdi, #plasma, mvourlakos, davidedmundson, mart, apol
Cc: davidre, plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, zachus, 
fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, alexeymin, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

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

[David Redondo]

davidre added a comment.


  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?

REPOSITORY
  R120 Plasma Workspace

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

To: trmdi, #plasma, mvourlakos, davidedmundson, mart, apol
Cc: davidre, plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, zachus, 
fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, alexeymin, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

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

[Tranter Madi]

trmdi updated this revision to Diff 71650.
trmdi added a comment.


  - Use QString type for the dbus screenId parameters

REPOSITORY
  R120 Plasma Workspace

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D25807?vs=71649=71650

BRANCH
  add-otherShellHelper (branched from master)

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

AFFECTED FILES
  shell/CMakeLists.txt
  shell/shellcorona.cpp
  shell/shellcorona.h
  shell/strutmanager.cpp
  shell/strutmanager.h

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

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

[David Edmundson]

davidedmundson added a comment.


  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.

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

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

[Tranter Madi]

trmdi updated this revision to Diff 71649.
trmdi added a comment.


  - Add another missed line

REPOSITORY
  R120 Plasma Workspace

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D25807?vs=71648=71649

BRANCH
  add-otherShellHelper (branched from master)

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

AFFECTED FILES
  shell/CMakeLists.txt
  shell/shellcorona.cpp
  shell/shellcorona.h
  shell/strutmanager.cpp
  shell/strutmanager.h

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

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

[Tranter Madi]

trmdi added a comment.


  @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

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

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

[Tranter Madi]

trmdi updated this revision to Diff 71648.
trmdi marked 2 inline comments as done.
trmdi added a comment.


  - Add a missed line

REPOSITORY
  R120 Plasma Workspace

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D25807?vs=71647=71648

BRANCH
  add-otherShellHelper (branched from master)

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

AFFECTED FILES
  shell/CMakeLists.txt
  shell/shellcorona.cpp
  shell/shellcorona.h
  shell/strutmanager.cpp
  shell/strutmanager.h

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

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

[Tranter Madi]

trmdi updated this revision to Diff 71647.
trmdi added a comment.


  - Follow David's guide

REPOSITORY
  R120 Plasma Workspace

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D25807?vs=71594=71647

BRANCH
  add-otherShellHelper (branched from master)

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

AFFECTED FILES
  shell/CMakeLists.txt
  shell/shellcorona.cpp
  shell/shellcorona.h
  shell/strutmanager.cpp
  shell/strutmanager.h

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

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(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

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

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

[Tranter Madi]

trmdi updated this revision to Diff 71594.
trmdi added a comment.


  - No need to setConnected manually

REPOSITORY
  R120 Plasma Workspace

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D25807?vs=71488=71594

BRANCH
  add-otherShellHelper (branched from master)

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

AFFECTED FILES
  shell/CMakeLists.txt
  shell/othershellhelper.cpp
  shell/othershellhelper.h
  shell/shellcorona.cpp
  shell/shellcorona.h

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

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

[Tranter Madi]

trmdi updated this revision to Diff 71488.
trmdi added a comment.


  - Improve comment

REPOSITORY
  R120 Plasma Workspace

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D25807?vs=71408=71488

BRANCH
  add-otherShellHelper (branched from master)

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

AFFECTED FILES
  shell/CMakeLists.txt
  shell/othershellhelper.cpp
  shell/othershellhelper.h
  shell/shellcorona.cpp
  shell/shellcorona.h

To: trmdi, #plasma, mvourlakos, davidedmundson, mart
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

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

[Tranter Madi]

trmdi updated this revision to Diff 71408.
trmdi added a comment.


  - emit signals when setConnected

REPOSITORY
  R120 Plasma Workspace

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D25807?vs=71391=71408

BRANCH
  add-otherShellHelper (branched from master)

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

AFFECTED FILES
  shell/CMakeLists.txt
  shell/othershellhelper.cpp
  shell/othershellhelper.h
  shell/shellcorona.cpp
  shell/shellcorona.h

To: trmdi, #plasma, mvourlakos, davidedmundson, mart
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

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

[Tranter Madi]

trmdi updated this revision to Diff 71391.
trmdi added a comment.


  - Redirect the signals
  - Rename the functions
  - Merge branch 'master' of https://anongit.kde.org/plasma-workspace into 
add-otherShellHelper

REPOSITORY
  R120 Plasma Workspace

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D25807?vs=71079=71391

BRANCH
  add-otherShellHelper (branched from master)

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

AFFECTED FILES
  shell/CMakeLists.txt
  shell/othershellhelper.cpp
  shell/othershellhelper.h
  shell/shellcorona.cpp
  shell/shellcorona.h

To: trmdi, #plasma, mvourlakos, davidedmundson, mart
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

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

[Tranter Madi]

trmdi updated this revision to Diff 71079.
trmdi added a comment.


  - Fix typo

REPOSITORY
  R120 Plasma Workspace

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D25807?vs=71077=71079

BRANCH
  add-otherShellHelper

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

AFFECTED FILES
  shell/CMakeLists.txt
  shell/othershellhelper.cpp
  shell/othershellhelper.h
  shell/shellcorona.cpp
  shell/shellcorona.h

To: trmdi, #plasma, mvourlakos, davidedmundson, mart
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

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

[Michail Vourlakos]

mvourlakos added a comment.


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

INLINE COMMENTS

> shellcorona.cpp:1046
>  
>  QRegion ShellCorona::availableScreenRegion(int id) const
> +if 
> (!m_otherShellHelper->availableScreenRegion(static_cast(id)).isNull())
>  {

I think is missing a "{" here

> shellcorona.cpp:1047
>  QRegion ShellCorona::availableScreenRegion(int id) const
> +if 
> (!m_otherShellHelper->availableScreenRegion(static_cast(id)).isNull())
>  {
> +return 
> m_otherShellHelper->availableScreenRegion(static_cast(id));

indent issues for all function

REPOSITORY
  R120 Plasma Workspace

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

To: trmdi, #plasma, mvourlakos, davidedmundson, mart
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

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

[Tranter Madi]

trmdi created this revision.
trmdi added reviewers: Plasma, mvourlakos.
Herald added a project: Plasma.
Herald added a subscriber: plasma-devel.
trmdi requested review of this revision.

REVISION SUMMARY
  Allow to set the available screen rect/region from outside through dbus.

REPOSITORY
  R120 Plasma Workspace

BRANCH
  add-otherShellHelper

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

AFFECTED FILES
  shell/CMakeLists.txt
  shell/othershellhelper.cpp
  shell/othershellhelper.h
  shell/shellcorona.cpp
  shell/shellcorona.h

To: trmdi, #plasma, mvourlakos
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