Details

Reviewers

zzag
graesslin

Group Reviewers

KWin

Commits

R108:f521d4bbe165: [wayland] add enter/leave virtual desktop API

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

Harbormaster completed remote builds in B4624: Diff 44957.Nov 6 2018, 12:00 PM

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

address comment

Harbormaster completed remote builds in B4628: Diff 44961.Nov 6 2018, 12:34 PM

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.

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.

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

Some additional corner cases:

effects->addWindowToDesktop(w, NET::OnAllDesktops);
effects->removeWindowFromDesktop(w, NET::OnAllDesktops);

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

Harbormaster completed remote builds in B4765: Diff 45183.Nov 9 2018, 4:47 PM

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]?

isOnAllDesktops() is broken.

This revision now requires changes to proceed.Nov 9 2018, 8:15 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 mentioned this in D16865: Unbreak AbstractClient::setDesktop.Nov 13 2018, 4:01 PM

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.

Harbormaster completed remote builds in B4924: Diff 45442.Nov 14 2018, 1:27 AM

whitespace

Harbormaster completed remote builds in B4925: Diff 45443.Nov 14 2018, 1:37 AM

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

Closed by commit R108:f521d4bbe165: [wayland] add enter/leave virtual desktop API (authored by davidedmundson). · Explain WhyNov 14 2018, 11:08 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.

In D16704#359297, @davidedmundson wrote:

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.

In D16704#359307, @graesslin wrote:

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

			Path	Packages
M			abstract_client.h (11 lines)
M			abstract_client.cpp (39 lines)
M			autotests/integration/virtual_desktop_test.cpp (39 lines)
M			autotests/mock_effectshandler.h (5 lines)
M			effects.h (2 lines)
M			effects.cpp (22 lines)
M			effects/desktopgrid/desktopgrid.cpp (14 lines)
M			libkwineffects/kwineffects.h (18 lines)
M			libkwineffects/kwineffects.cpp (11 lines)
M			useractions.cpp (5 lines)

Diff	ID	Base	Description	Created	Lint	Unit
Base			Base
Diff 1	44957	2b15c1d		Nov 6 2018, 12:00 PM	★	★
Diff 2	44961	2b15c1d	address comment	Nov 6 2018, 12:34 PM	★	★
Diff 3	45183	7664a74	As setDesktop was changed to "move" this left unSetDesktop non-symetric.	Nov 9 2018, 4:47 PM	★	★
Diff 4	45442	2fb2fb9	testVirtualDesktop (which is different from testVirtualDesktops) now passes.	Nov 14 2018, 1:27 AM	★	★
Diff 5	45443	2fb2fb9	whitespace	Nov 14 2018, 1:37 AM	★	★
Diff 6	45449	e6cf2e3	R108:f521d4bbe165776fc5306aa898a82f88dc2a9cf7	Nov 14 2018, 11:08 AM	★	★

[wayland] add enter/leave virtual desktop API
ClosedPublic
Actions

Details

Diff Detail

Revision Contents
Changeset List

Diff 45449

abstract_client.h

abstract_client.cpp

autotests/integration/virtual_desktop_test.cpp

autotests/mock_effectshandler.h

effects.h

effects.cpp

effects/desktopgrid/desktopgrid.cpp

libkwineffects/kwineffects.h

libkwineffects/kwineffects.cpp

useractions.cpp

[wayland] add enter/leave virtual desktop APIClosedPublicActions

Details

Diff Detail

Revision ContentsChangeset List

Diff 45449

abstract_client.h

abstract_client.cpp

autotests/integration/virtual_desktop_test.cpp

autotests/mock_effectshandler.h

effects.h

effects.cpp

effects/desktopgrid/desktopgrid.cpp

libkwineffects/kwineffects.h

libkwineffects/kwineffects.cpp

useractions.cpp

[wayland] add enter/leave virtual desktop API
ClosedPublic
Actions

Revision Contents
Changeset List