Expose wl_display_set_global_filter as a virtual method
ClosedPublic

Authored by davidedmundson on Sep 29 2017, 12:42 AM.

Details

Summary

This allows a server to filter which globals are visible and bindable by
clients.

Design rationale:
Could be it's own class with Display as an arg, but we need the lifespan
to exactly match Display, and the cardinality to match Display and it
needs to be set after we're started but before clients connect.

Better to enfore rules with code than with documentation.

I'm filtering by interface name as there isn't any other good
identifier of what a wl_global refers to, even if you could assume
you can cast the userdata to a Server::Global.


Note also that I posted my proposal on T4437 of how the kwin side should
behave.
We probably need to restart a discussion / fight to the death on plasma-
devel about that before proceeding.

Test Plan

Attached unit test

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.
Restricted Application added subscribers: Frameworks, plasma-devel. · View Herald Transcript
graesslin added inline comments.
src/server/filtered_display.cpp
63

I didn't know such API exists! This is great ;-)

But I think you need to increase the minimum Wayland version.

davidedmundson added inline comments.Sep 29 2017, 2:53 PM
src/server/filtered_display.cpp
63

Somewhat.
This is new in 1.13, we depend on 1.7.

Debian is currently on 1.12.
(my general rule is: "is Debian has it, everyone else must do")

Should we #ifdefs and then no-op (with a qwarning) on older libwayland?

graesslin added inline comments.Sep 29 2017, 3:38 PM
src/server/filtered_display.cpp
42

Why a const cast? Or other said: could we overload the getConnection to accept a const wl_client*?

46

that looks like dead code.

graesslin added inline comments.Sep 29 2017, 3:40 PM
src/server/filtered_display.cpp
63

Debian testing is already at 1.14. My general rule is: "if Debian testing has it, everyone else must do". Given that ifdef is always a mess I would say: go for increase!

davidedmundson added inline comments.Sep 29 2017, 3:56 PM
src/server/filtered_display.cpp
42

I tried.

modding getConnection means modding modding ClientConnection constructor, modding that means modding the Private ctor

that gets into

wl_client_add_destroy_listener(c, &listener);

c is now const, that fails.


We have 3 options:

  • a const_cast somewhere
  • Change Display to create ClientConnections when they come in, not on demand, then we can make getconnection const everywhere. (IMHO, maybe worth it, as our current clientConnected signal is rather misleading)
  • return PID in the API here instead of the client.

Btw. today is a great day: I finally can clean up my whiteboard. I had drafted an idea how to do what this change does more than a year ago and it's still waiting for the future me to implement it. But with this change I can finally clean it without having a bad conscience as this is so much cooler and better than my API design!

src/server/filtered_display.cpp
42

ouch, then const_cast is clearly better.

Restricted Application edited projects, added Plasma; removed Plasma on Wayland. · View Herald TranscriptSep 29 2017, 4:06 PM
graesslin accepted this revision.Sep 29 2017, 5:01 PM

Due you know whether we are allowed to block? E.g. to show a polkit dialog? My assumption would be we are not allowed to block here.

This revision is now accepted and ready to land.Sep 29 2017, 5:01 PM

It doesn't explicitly say anything, but I don't see how we can.

I do have an approach where we would be able to show a polkit dialog described in T4437.

This revision was automatically updated to reflect the committed changes.

One of our CI jobs is no longer happy:

13:02:41 -- The following REQUIRED packages have not been found:
13:02:41 
13:02:41  * Wayland (required version >= 1.13) , C library implementation of the Wayland protocol: a protocol for a compositor to talk to its clients , <http://wayland.freedesktop.org>
13:02:41
bcooksley reopened this revision.Oct 11 2017, 9:29 AM
This revision is now accepted and ready to land.Oct 11 2017, 9:29 AM
bcooksley requested changes to this revision.Oct 11 2017, 9:32 AM
bcooksley added a subscriber: bcooksley.

Please remedy the Wayland dependency incompatibility.

In the event the relevant package is not available in the distribution in question, you'll need to contact it's packagers and make the appropriate arrangements before filing any Sysadmin tickets for a version upgrade to be made.

This revision now requires changes to proceed.Oct 11 2017, 9:32 AM

This was reverted since yesterday when we noticed CI failed.

Please remedy the Wayland dependency incompatibility.

In the event the relevant package is not available in the distribution in question, you'll need to contact it's packagers and make the appropriate arrangements before filing any Sysadmin tickets for a version upgrade to be made.

@bcooksley in the old CI system we had a PPA for the latest and greatest Wayland in the Ubuntu installs. Why is this missing now? It didn't occur to me that this would cause a regression as we fixed this issue years ago...

graesslin accepted this revision.Oct 11 2017, 3:14 PM

I'm accepting this revision again. The CI system needs to have the PPA activated again as the old CI system had.

I'm not resubmitting something just for the purpose of you two winding each other up.

We're in no rush for this. I'll check what suse has and go from there.

I'm not resubmitting something just for the purpose of you two winding each other up.

Eh, it was not meant like that. I just wanted to point out that this does not need any changes in the code, but in the CI system.

We're in no rush for this. I'll check what suse has and go from there.

I created T7182 for updating the image.

@davidedmundson The CI image is updated, you can push this now.

This revision was automatically updated to reflect the committed changes.