[wayland] add enter/leave virtual desktop API
ClosedPublic

Authored by davidedmundson on Nov 6 2018, 12:00 PM.

Details

Summary

As setDesktop was changed to "move" this left unSetDesktop non-symetric.

This replaces it with explicit API to enter/leave.

This also moves new API to the new object based API rather than still
using ints.

Where numbers are used it has been tidied up so that desktop IDs are
uint, which should be used when we have a list of desktops.
int is used only when we have either a desktop ID or NET::OnAllDesktops
(-1)

Effects API cleared up to use this and use a set of x11 IDs, which
avoids any potential complications of handling add and removes any
ambiguity with what happens if you leave all desktops and such.

Test Plan

testVirtualDesktops passes (with pending kwayland patch)
Moving a window in the desktop grid on X11 behaves
Moving a window in the desktop grid on wayland behaves

Diff Detail

Repository
R108 KWin
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
davidedmundson created this revision.Nov 6 2018, 12:00 PM
Restricted Application added a project: KWin. · View Herald TranscriptNov 6 2018, 12:00 PM
Restricted Application added a subscriber: kwin. · View Herald Transcript
davidedmundson requested review of this revision.Nov 6 2018, 12:00 PM

Performing dynamic_cast without nullptr check is waste of time, use static_cast instead.

address comment

zzag added a subscriber: zzag.Nov 6 2018, 12:53 PM
zzag added inline comments.
abstract_client.cpp
575–576

Noob question: What would happen if the client is already on all desktops and you call enterDesktop for the first desktop?

584

Noob question: Same here, what would happen if the client is already on all desktops?

effects.cpp
921

If we pass Deleted, then we don't have to do anything. Also, why not qobject_cast?

Coding style nitpick:

auto client = qobject_cast<AbstractClient *>(static_cast<EffectWindowImpl *>(w)->window());
if (!client) {
    return;
}

VirtualDesktop *desktop = VirtualDesktopManager::self()->desktopForX11Id(desktop);
if (!desktop) {
    return;
}

client->enterDesktop(desktop);
libkwineffects/kwineffects.h
951–952

What's the difference between windowToDesktop and addWindowToDesktop? Deprecate it?

zzag added inline comments.Nov 6 2018, 12:58 PM
effects.cpp
921
VirtualDesktop *desktop = VirtualDesktopManager::self()->desktopForX11Id(desktop);
if (!desktop) {
    return;
}

This part might be broken because of NET::OnAllDesktops. Sorry.

zzag added a comment.Nov 6 2018, 1:09 PM

Noob question: What would happen if the client is already on all desktops and you call enterDesktop for the first desktop?

OK, I see, it we'll be on the first desktop.

zzag added a comment.Nov 6 2018, 1:13 PM

The removal is a little bit confusing.

Suppose there are 4 virtual desktops.

If a client is on all virtual desktops and for some reason an effect decides to remove it from the third virtual desktop, then the client still will be on all virtual desktops.

davidedmundson added inline comments.Nov 6 2018, 3:15 PM
abstract_client.cpp
575–576

that works fine

584

It currently does nothing.
Probably it should end up on everything but this one, especially as we have the non-atomic moves in the effects API.

effects.cpp
921

Why not qobject_cast?

Just consistency with the existing code

libkwineffects/kwineffects.h
951–952

move rather than adding.

So it's more akin to
addWindowToDesktop(new) + removeWindowFromDesktop(old)

so it's not entirely useless API.

zzag added inline comments.Nov 6 2018, 3:57 PM
libkwineffects/kwineffects.h
951–952

Can you please then document what it's doing?

davidedmundson planned changes to this revision.Nov 7 2018, 3:44 PM
zzag added a comment.Nov 7 2018, 3:48 PM

Some additional corner cases:

  • effects->addWindowToDesktop(w, NET::OnAllDesktops);
  • effects->removeWindowFromDesktop(w, NET::OnAllDesktops);
davidedmundson edited the summary of this revision. (Show Details)
davidedmundson edited the test plan for this revision. (Show Details)
davidedmundson removed subscribers: zzag, anthonyfieroni.
As setDesktop was changed to "move" this left unSetDesktop non-symetric.

This replaces it with explicit API to enter/leave.

This also moves new API to the new object based API rather than still
using ints.

Where numbers are used it has been tidied up so that desktop IDs are
uint, which should be used when we have a list of desktops.
int is used only when we have either a desktop ID or NET::OnAllDesktops
(-1)

Effects API cleared up to use this and use a set of x11 IDs, which
avoids any potential complications of handling add and removes in different orders
and removes any ambiguity with what happens if you leave NET_ALL
zzag added inline comments.Nov 9 2018, 6:50 PM
autotests/integration/virtual_desktop_test.cpp
260–264

Could we remove the client from the second virtual desktop here? (it should stay on the first and the third virtual desktop)

libkwineffects/kwineffects.h
960–961

const QVector<uint> &desktopIds

davidedmundson added inline comments.Nov 9 2018, 6:55 PM
autotests/integration/virtual_desktop_test.cpp
260–264

Why would we do that?

zzag added inline comments.Nov 9 2018, 7:07 PM
autotests/integration/virtual_desktop_test.cpp
260–264

As far as I understand,

void AbstractClient::leaveDesktop(VirtualDesktop *virtualDesktop)
{
    QVector<VirtualDesktop*> currentDesktops;
    if (m_desktops.isEmpty()) {
        currentDesktops = VirtualDesktopManager::self()->desktops();

is not covered by any test.

zzag added inline comments.Nov 9 2018, 7:56 PM
autotests/integration/virtual_desktop_test.cpp
213

Shouldn't it be desktops()[1]?

zzag requested changes to this revision.Nov 9 2018, 8:15 PM

isOnAllDesktops() is broken.

This revision now requires changes to proceed.Nov 9 2018, 8:15 PM
davidedmundson added a comment.EditedNov 9 2018, 11:49 PM

isOnAllDesktops() is broken.

That's the pending patch. We currently doesn't correctly cover the isOnAll desktops case properly.

On X If you have 1 desktop, and you're on desktop 1 that's not the same as being on all desktops.
Likewise on Wayland if you have 2 desktops and are on 1,2 that should not be same as being on all desktops.

Using an empty list for storage and the getter is fine, but it can't be done with enterDesktop and leaving a desktop to be on all makes no sense, so I need a new call.

Edit: fixed typos from typing on phone.

davidedmundson marked 11 inline comments as done.Nov 13 2018, 4:27 PM

testVirtualDesktop (which is different from testVirtualDesktops) now passes.

Big change is that if you have N dekstops, setting on all N should not set on all.

I did want to redo the way the wayland protocol for setting on all desktops
(as sending leave to enter all is silly) but will do that after so at least this test
can pass sooner.

graesslin accepted this revision.Nov 14 2018, 7:48 AM
graesslin added a subscriber: graesslin.

Let's try to get it in. I want to have passing tests this week again.

zzag accepted this revision.Nov 14 2018, 9:51 AM
This revision is now accepted and ready to land.Nov 14 2018, 9:51 AM
This revision was automatically updated to reflect the committed changes.

Let's try to get it in. I want to have passing tests this week again.

Sure.
I've taken over the VD stuff so will finish that - and I'll take the ShellClient one too.

The XClipboard test issue will change when we merge Roman's clipboard patchset, so worth doing that first.

The XClipboard test issue will change when we merge Roman's clipboard patchset, so worth doing that first.

I'm considering to temporarily qskip the test until Roman's changeset gets merged as I don't understand why it fails on the CI system.

zzag added a comment.Nov 15 2018, 11:25 AM

I'm considering to temporarily qskip the test until Roman's changeset gets merged

+1