Details

Reviewers

Group Reviewers

KWin
Plasma

Commits

R127:d671fcd0c09d: Add option to use wl_display_add_socket_auto

Summary

If automaticSocketNaming is enabled, it will use wl_display_add_socket_auto
to allocate the next free socket. The resulting name can be retrieved using
socketName after a successful start afterwards.

Test Plan

Ran the new autotest, passes. kwin_wayland still uses the old behaviour.

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.

fvogt created this revision.Nov 23 2018, 1:43 PM

Restricted Application added a project: Frameworks. · View Herald TranscriptNov 23 2018, 1:43 PM

Restricted Application added a subscriber: kde-frameworks-devel. · View Herald Transcript

fvogt requested review of this revision.Nov 23 2018, 1:43 PM

Harbormaster completed remote builds in B5284: Diff 46075.Nov 23 2018, 1:43 PM

romangg added a subscriber: romangg.Nov 23 2018, 1:47 PM

I don't think we need a second variable effectiveSocketName. Just test if socketName is empty. If it is call wl_display_add_socket_auto, otherwise call wl_display_add_socket.

According to description some autotest fails. Which one exactly?

In D17122#395537, @romangg wrote:

I don't think we need a second variable effectiveSocketName. Just test if socketName is empty. If it is call wl_display_add_socket_auto, otherwise call wl_display_add_socket.

If socketName is overwritten after using wl_display_add_socket_auto, it's not possible to call start twice without resetting socketName again.

If the actual socket's name is not written to any variable, it's impossible to set WAYLAND_DISPLAY correctly in KWin.

The new variable is used to be fully API compatible except if socketName was explicitly set to an empty string.

According to description some autotest fails. Which one exactly?

testSocketName as the default value of socketName changed.

FAIL!  : TestWaylandServerDisplay::testSocketName() Compared values are not the same
   Actual   (display.socketName())       : ""
   Expected (QStringLiteral("wayland-0")): "wayland-0"
   Loc: [/home/fabian/kderepos/kwayland/autotests/server/test_display.cpp(54)]

In D17122#395696, @fvogt wrote:

In D17122#395537, @romangg wrote:

I don't think we need a second variable effectiveSocketName. Just test if socketName is empty. If it is call wl_display_add_socket_auto, otherwise call wl_display_add_socket.

If socketName is overwritten after using wl_display_add_socket_auto, it's not possible to call start twice without resetting socketName again.

If the actual socket's name is not written to any variable, it's impossible to set WAYLAND_DISPLAY correctly in KWin.

The new variable is used to be fully API compatible except if socketName was explicitly set to an empty string.

Is a Display object meant to be started and terminated more than once? But ok, let's make sure.

According to description some autotest fails. Which one exactly?

testSocketName as the default value of socketName changed.
FAIL!  : TestWaylandServerDisplay::testSocketName() Compared values are not the same
   Actual   (display.socketName())       : ""
   Expected (QStringLiteral("wayland-0")): "wayland-0"
   Loc: [/home/fabian/kderepos/kwayland/autotests/server/test_display.cpp(54)]

Is there an argument against just checking on empty string at this location? Since the socket name is not yet set in line 54, there shouldn't be one.

In D17122#395704, @romangg wrote:

In D17122#395696, @fvogt wrote:

In D17122#395537, @romangg wrote:

I don't think we need a second variable effectiveSocketName. Just test if socketName is empty. If it is call wl_display_add_socket_auto, otherwise call wl_display_add_socket.

If socketName is overwritten after using wl_display_add_socket_auto, it's not possible to call start twice without resetting socketName again.

If the actual socket's name is not written to any variable, it's impossible to set WAYLAND_DISPLAY correctly in KWin.

The new variable is used to be fully API compatible except if socketName was explicitly set to an empty string.

Is a Display object meant to be started and terminated more than once? But ok, let's make sure.

Not sure, but it's technically possible and I don't see anything discouraging it.

According to description some autotest fails. Which one exactly?

testSocketName as the default value of socketName changed.
FAIL!  : TestWaylandServerDisplay::testSocketName() Compared values are not the same
   Actual   (display.socketName())       : ""
   Expected (QStringLiteral("wayland-0")): "wayland-0"
   Loc: [/home/fabian/kderepos/kwayland/autotests/server/test_display.cpp(54)]
Is there an argument against just checking on empty string at this location? Since the socket name is not yet set in line 54, there shouldn't be one.

The argument is that the default value changed, which is technically an ABI break - that's why this is an RFC. If you say that it's fine, I'll do the change and remove the RFC.

In D17122#395705, @fvogt wrote:
In D17122#395704, @romangg wrote:
In D17122#395696, @fvogt wrote:
In D17122#395537, @romangg wrote:

According to description some autotest fails. Which one exactly?

testSocketName as the default value of socketName changed.
FAIL!  : TestWaylandServerDisplay::testSocketName() Compared values are not the same
   Actual   (display.socketName())       : ""
   Expected (QStringLiteral("wayland-0")): "wayland-0"
   Loc: [/home/fabian/kderepos/kwayland/autotests/server/test_display.cpp(54)]
Is there an argument against just checking on empty string at this location? Since the socket name is not yet set in line 54, there shouldn't be one.
The argument is that the default value changed, which is technically an ABI break - that's why this is an RFC. If you say that it's fine, I'll do the change and remove the RFC.

Hmm, maybe then add another setter setAutomaticSocketNaming instead to switch to automatic socket name query instead. When it's not called before start it would fall back to old behavior.

Hmm, maybe then add another setter setAutomaticSocketNaming instead to switch to automatic socket name query instead. When it's not called before start it would fall back to old behavior.

That would need changes in KWin though so Plasma 5.16 only :-(

In D17122#395745, @fvogt wrote:

Hmm, maybe then add another setter setAutomaticSocketNaming instead to switch to automatic socket name query instead. When it's not called before start it would fall back to old behavior.

That would need changes in KWin though so Plasma 5.16 only :-(

Yes, that's fine. Do you need it for something in 5.15?

In D17122#395762, @romangg wrote:

In D17122#395745, @fvogt wrote:

Hmm, maybe then add another setter setAutomaticSocketNaming instead to switch to automatic socket name query instead. When it's not called before start it would fall back to old behavior.

That would need changes in KWin though so Plasma 5.16 only :-(

Yes, that's fine. Do you need it for something in 5.15?

I've been using a patched kwayland locally for several months now already, so I'd need to continue doing that for a another couple months. That's all though.

I'll do the change and remove the RFC.

Use a new bool instead.

Harbormaster completed remote builds in B7264: Diff 49802.Jan 18 2019, 10:49 AM

fvogt retitled this revision from RFC: Use wl_display_add_socket_auto by default to Add option to use wl_display_add_socket_auto.Jan 18 2019, 10:50 AM

fvogt edited the summary of this revision. (Show Details)

fvogt edited the test plan for this revision. (Show Details)

zzag added a subscriber: zzag.Jan 18 2019, 10:58 AM

zzag added inline comments.

src/server/display.cpp
86–88	Style: https://techbase.kde.org/Policies/Frameworks_Coding_Style#Variable_Declarations

romangg added inline comments.Jan 18 2019, 10:58 AM

src/server/display.cpp
87	bool running = false; bool automaticSocketNaming = false;

Add some style

Harbormaster completed remote builds in B7265: Diff 49806.Jan 18 2019, 11:56 AM

romangg requested changes to this revision.Jan 25 2019, 12:12 PM

romangg added inline comments.

autotests/server/test_display.cpp
223	This fails when one tries to run the test inside another Wayland session, which already uses the wayland-0 socket name. What you could do here is loop until you find the first non-existing `wayland-x` in the runtime dir. Then do it one more time from there until you find the second `wayland-y`. Remember x,y and compare these below against the created socket names.

This revision now requires changes to proceed.Jan 25 2019, 12:12 PM

fvogt added inline comments.Jan 25 2019, 12:15 PM

autotests/server/test_display.cpp
223	That won't work reliably either though - if wayland-0 is free, but wayland-1 is used, it would pick wayland-0 and wayland-2. Maybe it should just check that starting both displays at the same time succeeds and that their socket names are not equal.

davidedmundson added a subscriber: davidedmundson.Jan 25 2019, 12:19 PM

davidedmundson added inline comments.

autotests/server/test_display.cpp
223	I would suggest changing the XDG_RUNTIME_DIR for the duration of the test.

romangg added inline comments.Jan 25 2019, 12:24 PM

autotests/server/test_display.cpp
223	That's why I said "do it one more time from there until you find the second wayland-y". For example (semi-pseudo): QString name1, name2; int cnt = -1; while(true) { cnt++; const QString name = "wayland-" + str(cnt); if (!runtimeDir.exists(name)) { name1 = name; break; } } while(true) { cnt++; const QString name = "wayland-" + str(cnt); if (!runtimeDir.exists(name)) { name2 = name; break; } } David's solution is nicer though.

autotests/server/test_display.cpp

223

That's why I said "do it one more time from there until you find the second wayland-y".
For example (semi-pseudo):

QString name1, name2;

int cnt = -1;
while(true) {
    cnt++;
    const QString name = "wayland-" + str(cnt);
    if (!runtimeDir.exists(name)) {
        name1 = name;
        break;
    }
}
while(true) {
    cnt++;
    const QString name = "wayland-" + str(cnt);
    if (!runtimeDir.exists(name)) {
        name2 = name;
        break;
    }
}

David's solution is nicer though.

Replace XDG_RUNTIME_DIR, test still passes

Harbormaster completed remote builds in B7505: Diff 50238.Jan 25 2019, 12:25 PM

fvogt marked 4 inline comments as done.Jan 25 2019, 12:26 PM

romangg added a dependent revision: D18522: Name Wayland socket automatically when no socket name was specified.Jan 25 2019, 12:28 PM

romangg accepted this revision.Jan 25 2019, 12:30 PM

This revision is now accepted and ready to land.Jan 25 2019, 12:30 PM

Closed by commit R127:d671fcd0c09d: Add option to use wl_display_add_socket_auto (authored by fvogt). · Explain WhyJan 25 2019, 12:32 PM

This revision was automatically updated to reflect the committed changes.

romangg mentioned this in D15512: [startplasmacompositor] Add Wayland socket argument.Feb 8 2019, 3:30 PM

			Path	Packages
M			autotests/server/test_display.cpp (24 lines)
M			src/server/display.h (15 lines)
M			src/server/display.cpp (29 lines)

Diff	ID	Base	Description	Created	Lint	Unit
Base			Base
Diff 1	46075	3e8f229		Nov 23 2018, 1:43 PM	★	★
Diff 2	49802	bf1cd16	Use a new bool instead.	Jan 18 2019, 10:49 AM	★	★
Diff 3	49806	bf1cd16	Add some style	Jan 18 2019, 11:56 AM	★	★
Diff 4	50238	bf1cd16	Replace XDG_RUNTIME_DIR, test still passes	Jan 25 2019, 12:25 PM	★	★
Diff 5	50241	87d9d44	R127:d671fcd0c09d947726091540a7cc9d9210e088eb	Jan 25 2019, 12:32 PM	★	★

	Status	Author	Revision
	Closed	romangg	D18522 Name Wayland socket automatically when no socket name was specified
	Closed	fvogt	D17122 Add option to use wl_display_add_socket_auto

Add option to use wl_display_add_socket_auto
ClosedPublic
Actions

Details

Diff Detail

Revision Contents
Changeset List

Diff 50241

autotests/server/test_display.cpp

src/server/display.h

src/server/display.cpp

Add option to use wl_display_add_socket_autoClosedPublicActions

Details

Diff Detail

Revision ContentsChangeset List

Diff 50241

autotests/server/test_display.cpp

src/server/display.h

src/server/display.cpp

Add option to use wl_display_add_socket_auto
ClosedPublic
Actions

Revision Contents
Changeset List