in order for the pager to work correctly and not having to use a weird mix of wayland and dbus apis, the rows
number needs to be in the protocol
Details
Details
- Reviewers
davidedmundson - Group Reviewers
Plasma KWin - Maniphest Tasks
- T4457: [kwayland] Virtual Desktop protocol
- Commits
- R127:d1e8b45d9310: Add rows info to the plasma virtual desktop protocol
new passing autotest
Diff Detail
Diff Detail
- Repository
- R127 KWayland
- Lint
Automatic diff as part of commit; lint not applicable. - Unit
Automatic diff as part of commit; unit tests not applicable.
Comment Actions
As per this, progress on this is currently blocking wrapping up Wayland virtual desktops support for 5.15.
Comment Actions
I thought we had a whole conversation in the first set of reviews that views (layout) was independent of data.
The desktop cube shows everything flat, desktop grid is configurable (and hardcoded).
The pager is just another view, there's no need to sync with kwin's grid?
| src/client/protocols/plasma-virtual-desktop.xml | ||
|---|---|---|
| 56 | This needs to be in a version 2 and done at the end with appropriate bumps everywhere. | |
| src/server/plasmavirtualdesktop_interface.h | ||
| 52 | Is this even implemented?? I can't find it If so it's technically safe to remove from the header, but it's a pretty rubbish situation to begin with. | |
Comment Actions
The desktop cube shows everything flat, desktop grid is configurable (and hardcoded).
The desktop grid effect uses the rows setting from the KCM for me.
I don't think it's easy for users to understand that the KCM is configuring KWin only, instead of all of Plasma. Especially because the Pager can't escape KWin's spatial notions anyway: If you use the Pager to switch desktops, by default KWin does a slide animation that's directional based on the rows setting. If the Pager's layout differs from KWin's, the animation played won't fit what's visible on the Pager. Considering the Pager is all about providing a spatial overview into the system, this seems weird.
Plus this protocol is meant to be a private one for use between KWin and plasmashell. I think it's OK for them to integrate tightly, it's not the same as exposing layout information to regular clients. I'd argue the entire PlasmaWindowManagement protocol already does so after all.
| autotests/client/test_plasma_virtual_desktop.cpp | ||
|---|---|---|
| 280 | why? we want to compare the number of rows, no? | |
| autotests/client/test_plasma_virtual_desktop.cpp | ||
|---|---|---|
| 280 | What if rowsChanged is never emitted? In KWin, we usually do QVERIFY(spy.wait()); | |
| src/client/protocols/plasma-virtual-desktop.xml | ||
|---|---|---|
| 56 | This isn't done. | |
| src/server/plasmavirtualdesktop_interface.h | ||
|---|---|---|
| 52 | Yeah, given that it was introduced quite recently and it's not implemented, maybe it would be better to delete the method. In either case, if you try to use the method, you'll probably get linker errors. Also, maybe we don't need PlasmaVirtualDesktopManagementInterface::Private::{rows,columns}. | |
| 56 | Wrong version. | |
| src/client/protocols/plasma-virtual-desktop.xml | ||
|---|---|---|
| 56 | event needs to be at the end (like how you keep source compatibility in a C++ enum) | |
| src/server/plasmavirtualdesktop_interface.cpp | ||
| 201 | Technically we should do: if (wl_resource_get_version(*it) < org_kde_plasma_virtual_desktop_management_send_rows_since_version) {return;} Though in this specific case as it's only plasma using this API and restart kwin means we'll restart plasma I guess it doesn't really matter. | |
| src/server/plasmavirtualdesktop_interface.cpp | ||
|---|---|---|
| 94 | This one should be bumped as well. | |