<table><tr><td style="">romangg added a comment.
</td><a style="text-decoration: none; padding: 4px 8px; margin: 0 8px 8px; float: right; color: #464C5C; font-weight: bold; border-radius: 3px; background-color: #F7F7F9; background-image: linear-gradient(to bottom,#fff,#f1f0f1); display: inline-block; border: 1px solid rgba(71,87,120,.2);" href="https://phabricator.kde.org/D12820">View Revision</a></tr></table><br /><div><div><p>The file org_kde_plasma_virtual_desktop.xml should be renamed to virtual-desktops.xml or plasma-virtual-desktops.xml to have a similar name format as the other protocol files we have. Also I would in general prefer to just call it "VirtualDesktop..." instaed of "PlasmaVirtualDesktop..." in the class names (file names alike).</p>
<p>Sometimes it says "desktop" instead of "virtual desktop". Although it's cumbersome I would recommend to always use the term "virtual desktop".</p>
<p>Since virtual desktops form a grid I would have thought first at a 2D matrix / 2D array, i.e.: <tt style="background: #ebebeb; font-size: 13px;">QVector< QVector< PlasmaVirtualDesktopInterface* > ></tt> for storing the global information of all Virtual Desktops and their layout in <tt style="background: #ebebeb; font-size: 13px;">PlasmaVirtualDesktopManagementInterface</tt>. You currently use a QList <tt style="background: #ebebeb; font-size: 13px;">orderedDesktops</tt> for all virtual desktops and "holes" when inserting a virtual desktop in the grid in <tt style="background: #ebebeb; font-size: 13px;">sortDesktops</tt>. This seems a bit clunky and error-prone (in particular the calculations to reorder in <tt style="background: #ebebeb; font-size: 13px;">sortDesktops</tt>).</p>
<p>Now a general thought on the layout mechanism: Currently the number of rows is set and together with the number of virtual desktops this results in a column number. This concept is not straightforward. It is also very limiting in the end: a compositor would not be able to set patterns different from a flow left to right, top to bottom. Even the current KCM is already more involved by offering the option to change row and column count.</p>
<p>I propose the following: Remove the <tt style="background: #ebebeb; font-size: 13px;">layout_position</tt> event from org_kde_plasma_virtual_desktop. Instead let event <tt style="background: #ebebeb; font-size: 13px;">layout</tt> of org_kde_plasma_virtual_desktop_management be: <tt style="background: #ebebeb; font-size: 13px;">layout(wl_array *virtual_desktops, uint32_t columns)</tt> with <tt style="background: #ebebeb; font-size: 13px;">virtual_desktops</tt> representing a matrix with <tt style="background: #ebebeb; font-size: 13px;">columns</tt> many columns of all virtual desktops ids from left to right, top to bottom with 0 in cells where no virtual desktop is placed. This way a compositor can create arbitrary 2D patterns and change the count and position of virtual desktops atomically when sending the layout event (together with added, remove events if necessary) and the done event afterwards.</p></div></div><br /><div><strong>INLINE COMMENTS</strong><div><div style="margin: 6px 0 12px 0;"><div style="border: 1px solid #C7CCD9; border-radius: 3px;"><div style="padding: 0; background: #F7F7F7; border-color: #e3e4e8; border-style: solid; border-width: 0 0 1px 0; margin: 0;"><div style="color: #74777d; background: #eff2f4; padding: 6px 8px; overflow: hidden;"><a style="float: right; text-decoration: none;" href="https://phabricator.kde.org/D12820#inline-65859">View Inline</a><span style="color: #4b4d51; font-weight: bold;">test_plasma_virtual_desktop.cpp:3</span></div>
<div style="font: 11px/15px "Menlo", "Consolas", "Monaco", monospace; white-space: pre-wrap; clear: both; padding: 4px 0; margin: 0;"><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"><span style="color: #74777d">Copyright 2014 Martin Gräßlin <mgraesslin@kde.org></span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"><span style="color: #74777d">Copyright 2018 Msrco Martin <mart@kde.org></span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">M<u>a</u>rco</p></div></div><br /><div style="border: 1px solid #C7CCD9; border-radius: 3px;"><div style="padding: 0; background: #F7F7F7; border-color: #e3e4e8; border-style: solid; border-width: 0 0 1px 0; margin: 0;"><div style="color: #74777d; background: #eff2f4; padding: 6px 8px; overflow: hidden;"><a style="float: right; text-decoration: none;" href="https://phabricator.kde.org/D12820#inline-65873">View Inline</a><span style="color: #4b4d51; font-weight: bold;">plasmavirtualdesktop.h:141</span></div>
<div style="font: 11px/15px "Menlo", "Consolas", "Monaco", monospace; white-space: pre-wrap; clear: both; padding: 4px 0; margin: 0;"><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"><span style="color: #a0a000">Q_SIGNALS</span><span class="p">:</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span style="color: #aa4000">void</span> <span class="n">removed</span><span class="p">();</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">This signal is unneeded when <tt style="background: #ebebeb; font-size: 13px;">desktopRemoved</tt> is there as well.</p></div></div><br /><div style="border: 1px solid #C7CCD9; border-radius: 3px;"><div style="padding: 0; background: #F7F7F7; border-color: #e3e4e8; border-style: solid; border-width: 0 0 1px 0; margin: 0;"><div style="color: #74777d; background: #eff2f4; padding: 6px 8px; overflow: hidden;"><a style="float: right; text-decoration: none;" href="https://phabricator.kde.org/D12820#inline-65881">View Inline</a><span style="color: #4b4d51; font-weight: bold;">org_kde_plasma_virtual_desktop.xml:5</span></div>
<div style="font: 11px/15px "Menlo", "Consolas", "Monaco", monospace; white-space: pre-wrap; clear: both; padding: 4px 0; margin: 0;"><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"><span style="color: #304a96"> Copyright (C) 2015 Martin Gräßlin</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"><span style="color: #304a96"> Copyright (C) 2015 Marco Martin</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">2018 and only you Marco?</p></div></div><br /><div style="border: 1px solid #C7CCD9; border-radius: 3px;"><div style="padding: 0; background: #F7F7F7; border-color: #e3e4e8; border-style: solid; border-width: 0 0 1px 0; margin: 0;"><div style="color: #74777d; background: #eff2f4; padding: 6px 8px; overflow: hidden;"><a style="float: right; text-decoration: none;" href="https://phabricator.kde.org/D12820#inline-65878">View Inline</a><span style="color: #4b4d51; font-weight: bold;">org_kde_plasma_virtual_desktop.xml:26</span></div>
<div style="font: 11px/15px "Menlo", "Consolas", "Monaco", monospace; white-space: pre-wrap; clear: both; padding: 4px 0; margin: 0;"><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span style="color: #00702a"></description></span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span style="color: #00702a"><arg</span> <span style="color: #354bb3">name=</span><span style="color: #766510">"desktop"</span> <span style="color: #354bb3">type=</span><span style="color: #766510">"new_id"</span> <span style="color: #354bb3">interface=</span><span style="color: #766510">"org_kde_plasma_virtual_desktop"</span><span style="color: #00702a">/></span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span style="color: #00702a"><arg</span> <span style="color: #354bb3">name=</span><span style="color: #766510">"id"</span> <span style="color: #354bb3">type=</span><span style="color: #766510">"string"</span> <span style="color: #354bb3">summary=</span><span style="color: #766510">"Unique id of the desktop"</span><span style="color: #00702a">/></span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;"><tt style="background: #ebebeb; font-size: 13px;">name="id"</tt></p></div></div><br /><div style="border: 1px solid #C7CCD9; border-radius: 3px;"><div style="padding: 0; background: #F7F7F7; border-color: #e3e4e8; border-style: solid; border-width: 0 0 1px 0; margin: 0;"><div style="color: #74777d; background: #eff2f4; padding: 6px 8px; overflow: hidden;"><a style="float: right; text-decoration: none;" href="https://phabricator.kde.org/D12820#inline-65888">View Inline</a><span style="color: #4b4d51; font-weight: bold;">org_kde_plasma_virtual_desktop.xml:45</span></div>
<div style="font: 11px/15px "Menlo", "Consolas", "Monaco", monospace; white-space: pre-wrap; clear: both; padding: 4px 0; margin: 0;"><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span style="color: #00702a"><event</span> <span style="color: #354bb3">name=</span><span style="color: #766510">"layout"</span><span style="color: #00702a">></span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span style="color: #00702a"><description</span> <span style="color: #354bb3">summary=</span><span style="color: #766510">"Hint for the visual organization of the pager"</span><span style="color: #00702a">></span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">This event is redundant with the current implementation. A client can infer the number of rows and columns from the org_kde_plasma_virtual_desktop objects it receives and their maximum row and column values.</p>
<p style="padding: 0; margin: 8px;">Also a compositor might be interested in setting a custom layout like this: 5 virtual desktops as a cross (one top, 3 in a row mid, one bottom). While one can still say this has in some sense 3 rows and 3 columns, the event in this case loses its intended meaning of defining the "layout".</p>
<p style="padding: 0; margin: 8px;">Look at my review comment for a different approach.</p></div></div><br /><div style="border: 1px solid #C7CCD9; border-radius: 3px;"><div style="padding: 0; background: #F7F7F7; border-color: #e3e4e8; border-style: solid; border-width: 0 0 1px 0; margin: 0;"><div style="color: #74777d; background: #eff2f4; padding: 6px 8px; overflow: hidden;"><a style="float: right; text-decoration: none;" href="https://phabricator.kde.org/D12820#inline-65889">View Inline</a><span style="color: #4b4d51; font-weight: bold;">org_kde_plasma_virtual_desktop.xml:83</span></div>
<div style="font: 11px/15px "Menlo", "Consolas", "Monaco", monospace; white-space: pre-wrap; clear: both; padding: 4px 0; margin: 0;"><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span style="color: #00702a"><description</span> <span style="color: #354bb3">summary=</span><span style="color: #766510">"Position for the desktop"</span><span style="color: #00702a">></span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> Logical position of the visual model of this desktop in terms of rows and columns: there will be only one desktop on a (row, column)
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span style="color: #00702a"></description></span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">Mention that this event is send on bind and when the position changes.</p>
<p style="padding: 0; margin: 8px;">+ line breaks</p></div></div><br /><div style="border: 1px solid #C7CCD9; border-radius: 3px;"><div style="padding: 0; background: #F7F7F7; border-color: #e3e4e8; border-style: solid; border-width: 0 0 1px 0; margin: 0;"><div style="color: #74777d; background: #eff2f4; padding: 6px 8px; overflow: hidden;"><a style="float: right; text-decoration: none;" href="https://phabricator.kde.org/D12820#inline-65883">View Inline</a><span style="color: #4b4d51; font-weight: bold;">plasma-window-management.xml:273</span></div>
<div style="font: 11px/15px "Menlo", "Consolas", "Monaco", monospace; white-space: pre-wrap; clear: both; padding: 4px 0; margin: 0;"><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span style="color: #00702a"><request</span> <span style="color: #354bb3">name=</span><span style="color: #766510">"request_enter_virtual_desktop"</span> <span style="color: #354bb3">since=</span><span style="color: #766510">"8"</span><span style="color: #00702a">></span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span style="color: #00702a"><description</span> <span style="color: #354bb3">summary=</span><span style="color: #766510">"map window on a virtual desktop"</span><span style="color: #00702a">></span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">Just name it <tt style="background: #ebebeb; font-size: 13px;">enter_virtual_desktop</tt> I would say. For <tt style="background: #ebebeb; font-size: 13px;">request_leave_virtual_desktop</tt> the same.</p></div></div><br /><div style="border: 1px solid #C7CCD9; border-radius: 3px;"><div style="padding: 0; background: #F7F7F7; border-color: #e3e4e8; border-style: solid; border-width: 0 0 1px 0; margin: 0;"><div style="color: #74777d; background: #eff2f4; padding: 6px 8px; overflow: hidden;"><a style="float: right; text-decoration: none;" href="https://phabricator.kde.org/D12820#inline-65887">View Inline</a><span style="color: #4b4d51; font-weight: bold;">plasmavirtualdesktop_interface.cpp:143</span></div>
<div style="font: 11px/15px "Menlo", "Consolas", "Monaco", monospace; white-space: pre-wrap; clear: both; padding: 4px 0; margin: 0;"><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span class="p">}</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span class="p">});</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">indent (below as well)</p></div></div><br /><div style="border: 1px solid #C7CCD9; border-radius: 3px;"><div style="padding: 0; background: #F7F7F7; border-color: #e3e4e8; border-style: solid; border-width: 0 0 1px 0; margin: 0;"><div style="color: #74777d; background: #eff2f4; padding: 6px 8px; overflow: hidden;"><a style="float: right; text-decoration: none;" href="https://phabricator.kde.org/D12820#inline-65890">View Inline</a><span style="color: #4b4d51; font-weight: bold;">plasmavirtualdesktop_interface.cpp:200</span></div>
<div style="font: 11px/15px "Menlo", "Consolas", "Monaco", monospace; white-space: pre-wrap; clear: both; padding: 4px 0; margin: 0;"><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span class="n">org_kde_plasma_virtual_desktop_management_send_layout</span><span class="p">(</span><span class="n">resource</span><span class="p">,</span> <span class="n">rows</span><span class="p">,</span> <span class="n">columns</span><span class="p">);</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"><span class="p">}</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">Send done event in the end and flush the client.</p></div></div><br /><div style="border: 1px solid #C7CCD9; border-radius: 3px;"><div style="padding: 0; background: #F7F7F7; border-color: #e3e4e8; border-style: solid; border-width: 0 0 1px 0; margin: 0;"><div style="color: #74777d; background: #eff2f4; padding: 6px 8px; overflow: hidden;"><a style="float: right; text-decoration: none;" href="https://phabricator.kde.org/D12820#inline-65886">View Inline</a><span style="color: #4b4d51; font-weight: bold;">plasmavirtualdesktop_interface.h:77</span></div>
<div style="font: 11px/15px "Menlo", "Consolas", "Monaco", monospace; white-space: pre-wrap; clear: both; padding: 4px 0; margin: 0;"><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"><span style="color: #74777d"> */</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span class="n">PlasmaVirtualDesktopInterface</span> <span style="color: #aa2211">*</span><span style="color: #004012">createDesktop</span><span class="p">(</span><span style="color: #aa4000">const</span> <span class="n">QString</span> <span style="color: #aa2211">&</span><span class="n">id</span><span class="p">);</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">The compositor does not need to decide upon an id. Let KWayland do this internally (this also means that whenever <tt style="background: #ebebeb; font-size: 13px;">createDesktop</tt> is called it is guaranteed that a new PlasmaVirtualDesktopInterface will be returned.</p>
<p style="padding: 0; margin: 8px;">Or is this needed in regards to Activities down the road? If yes I would also argue that a KWayland internal id would be nicer and KWin should map them then to Activity ids.</p></div></div><br /><div style="border: 1px solid #C7CCD9; border-radius: 3px;"><div style="padding: 0; background: #F7F7F7; border-color: #e3e4e8; border-style: solid; border-width: 0 0 1px 0; margin: 0;"><div style="color: #74777d; background: #eff2f4; padding: 6px 8px; overflow: hidden;"><a style="float: right; text-decoration: none;" href="https://phabricator.kde.org/D12820#inline-65885">View Inline</a><span style="color: #4b4d51; font-weight: bold;">plasmavirtualdesktop_interface.h:103</span></div>
<div style="font: 11px/15px "Menlo", "Consolas", "Monaco", monospace; white-space: pre-wrap; clear: both; padding: 4px 0; margin: 0;"><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span class="n">explicit</span> <span class="n">PlasmaVirtualDesktopManagementInterface</span><span class="p">(</span><span class="n">Display</span> <span style="color: #aa2211">*</span><span class="n">display</span><span class="p">,</span> <span class="n">QObject</span> <span style="color: #aa2211">*</span><span class="n">parent</span> <span style="color: #aa2211">=</span> <span class="n">nullptr</span><span class="p">);</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span class="n">friend</span> <span class="n">class</span> <span class="n">PlasmaVirtualDesktopInterface</span><span class="p">;</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span class="n">friend</span> <span class="n">class</span> <span class="n">Display</span><span class="p">;</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">That the management class is friend to the things it manages (here the PlasmaVirtualDesktopInterface) ok, but the other way around... I would maybe try to work without it.</p></div></div></div></div></div><br /><div><strong>REPOSITORY</strong><div><div>R127 KWayland</div></div></div><br /><div><strong>REVISION DETAIL</strong><div><a href="https://phabricator.kde.org/D12820">https://phabricator.kde.org/D12820</a></div></div><br /><div><strong>To: </strong>mart, KWin, Plasma, graesslin, hein<br /><strong>Cc: </strong>romangg, kde-frameworks-devel, michaelh, ngraham, bruns<br /></div>