D12820: Add KWayland virtual desktop protocol

[Roman Gilg]

romangg added a comment.


  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).
  
  Sometimes it says "desktop" instead of "virtual desktop". Although it's cumbersome I would recommend to always use the term "virtual desktop".
  
  Since virtual desktops form a grid I would have thought first at a 2D matrix / 2D array, i.e.: `QVector< QVector< PlasmaVirtualDesktopInterface* > >` for storing the global information of all Virtual Desktops and their layout in `PlasmaVirtualDesktopManagementInterface`. You currently use a QList `orderedDesktops` for all virtual desktops and "holes" when inserting a virtual desktop in the grid in `sortDesktops`. This seems a bit clunky and error-prone (in particular the calculations to reorder in `sortDesktops`).
  
  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.
  
  I propose the following: Remove the `layout_position` event from org_kde_plasma_virtual_desktop. Instead let event `layout` of org_kde_plasma_virtual_desktop_management be: `layout(wl_array *virtual_desktops, uint32_t columns)` with `virtual_desktops` representing a matrix with `columns` 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.

INLINE COMMENTS

> test_plasma_virtual_desktop.cpp:3
> +Copyright 2014  Martin Gräßlin <mgraesslin at kde.org>
> +Copyright 2018  Msrco Martin <mart at kde.org>
> +

M__a__rco

> plasmavirtualdesktop.h:141
> +Q_SIGNALS:
> +    void removed();
> +

This signal is unneeded when `desktopRemoved` is there as well.

> org_kde_plasma_virtual_desktop.xml:5
> +    Copyright (C) 2015 Martin Gräßlin
> +    Copyright (C) 2015 Marco Martin
> +

2018 and only you Marco?

> org_kde_plasma_virtual_desktop.xml:26
> +        </description>
> +        <arg name="desktop" type="new_id" interface="org_kde_plasma_virtual_desktop"/>
> +        <arg name="id" type="string" summary="Unique id of the desktop"/>

`name="id"`

> org_kde_plasma_virtual_desktop.xml:45
> +
> +    <event name="layout">
> +        <description summary="Hint for the visual organization of the pager">

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.

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".

Look at my review comment for a different approach.

> org_kde_plasma_virtual_desktop.xml:83
> +        <description summary="Position for the desktop">
> +            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)
> +        </description>

Mention that this event is send on bind and when the position changes.

+ line breaks

> plasma-window-management.xml:273
> +
> +    <request name="request_enter_virtual_desktop" since="8">
> +      <description summary="map window on a virtual desktop">

Just name it `enter_virtual_desktop` I would say. For `request_leave_virtual_desktop` the same.

> plasmavirtualdesktop_interface.cpp:143
> +            }
> +        });
> +

indent (below as well)

> plasmavirtualdesktop_interface.cpp:200
> +    org_kde_plasma_virtual_desktop_management_send_layout(resource, rows, columns);
> +}
> +

Send done event in the end and flush the client.

> plasmavirtualdesktop_interface.h:77
> +     */
> +    PlasmaVirtualDesktopInterface *createDesktop(const QString &id);
> +

The compositor does not need to decide upon an id. Let KWayland do this internally (this also means that whenever `createDesktop` is called it is guaranteed that a new PlasmaVirtualDesktopInterface will be returned.

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.

> plasmavirtualdesktop_interface.h:103
> +    explicit PlasmaVirtualDesktopManagementInterface(Display *display, QObject *parent = nullptr);
> +    friend class PlasmaVirtualDesktopInterface;
> +    friend class Display;

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.

REPOSITORY
  R127 KWayland

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

To: mart, #kwin, #plasma, graesslin, hein
Cc: romangg, kde-frameworks-devel, michaelh, ngraham, bruns
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-frameworks-devel/attachments/20180511/1cf86c3b/attachment-0001.html>