add pid to plasma window management protocol
ClosedPublic

Authored by sebas on May 7 2017, 6:54 PM.

Details

Summary

This patch adds a pid event to the plasma window management protocol. It
allows the compositor to tell allow a mapping between windows and processes.

Bumps the version number of the interface to 8 to indicate this.

Test Plan

autotest added, passed

Diff Detail

Repository
R127 KWayland
Branch
sebas/processid
Lint
No Linters Available
Unit
No Unit Test Coverage
sebas created this revision.May 7 2017, 6:54 PM
Restricted Application added projects: Plasma on Wayland, Frameworks. · View Herald TranscriptMay 7 2017, 6:54 PM
Restricted Application added subscribers: Frameworks, plasma-devel. · View Herald Transcript
davidedmundson added inline comments.
src/client/protocols/plasma-window-management.xml
267

uint is a native type: https://wayland.freedesktop.org/docs/html/ch04.html#sect-Protocol-Wire-Format

you don't need to do an implicit cast

graesslin requested changes to this revision.May 7 2017, 7:24 PM

Please also extend the autotests/client/test_wayland_windowmanagement.cpp

src/client/plasmawindowmanagement.h
385

copy and paste error

481

"@since" missing

This revision now requires changes to proceed.May 7 2017, 7:24 PM
sebas updated this revision to Diff 14407.May 11 2017, 2:23 PM
sebas edited edge metadata.
sebas marked 3 inline comments as done.

Address review comments

  • Minor corrections in docs
  • Use uint for the pid
  • uint32 for pids instead of int32
  • Fix @since in docs
  • send initial value when it's not 0
  • Add autotest in TestWindowManagement as well
Restricted Application edited projects, added Plasma; removed Plasma on Wayland. · View Herald TranscriptMay 11 2017, 2:23 PM
sebas updated this revision to Diff 14410.May 11 2017, 2:48 PM
  • also test default
Restricted Application edited projects, added Plasma on Wayland; removed Plasma. · View Herald TranscriptMay 11 2017, 2:48 PM
graesslin added inline comments.May 11 2017, 3:02 PM
src/client/plasmawindowmodel.cpp
84–86

I consider it as impossible that the PID changes.

graesslin added inline comments.May 11 2017, 3:06 PM
autotests/client/test_plasma_window_model.cpp
215

Please extend this

sebas updated this revision to Diff 14418.May 11 2017, 6:09 PM
  • Drop pid change handling in the model code
  • Extend model test

--Eike on sebas' box

Restricted Application edited projects, added Plasma; removed Plasma on Wayland. · View Herald TranscriptMay 11 2017, 6:09 PM
apol added a subscriber: apol.May 11 2017, 11:20 PM

Who is going to be setting this pid? Because note that containerized applications don't know their own pid (see kdbusaddons & knotifications patches for big shrug).

$ docker run ubuntu:17.04 pidof pidof
1
$ pidof pidof
6940
In D5747#108916, @apol wrote:

Who is going to be setting this pid? Because note that containerized applications don't know their own pid (see kdbusaddons & knotifications patches for big shrug).

It's KWin. Who gets the information from the socket.

hein accepted this revision.May 12 2017, 2:37 PM
graesslin requested changes to this revision.May 12 2017, 4:39 PM
graesslin added inline comments.
src/client/plasmawindowmodel.cpp
84

Unrelated newline

src/client/protocols/plasma-window-management.xml
263

The pid will never change.

This revision now requires changes to proceed.May 12 2017, 4:39 PM
sebas updated this revision to Diff 14442.May 12 2017, 5:42 PM
sebas edited edge metadata.
  • Update docs: the pid is just set, but doesn't logically change
  • ws--
Restricted Application edited projects, added Plasma on Wayland; removed Plasma. · View Herald TranscriptMay 12 2017, 5:42 PM
graesslin accepted this revision.May 13 2017, 6:20 AM
This revision is now accepted and ready to land.May 13 2017, 6:20 AM
This revision was automatically updated to reflect the committed changes.
graesslin reopened this revision.May 13 2017, 4:04 PM

Can reproduce the failing test locally. Please fix ASAP! Broken tests is not acceptable in KWayland. This change has been on review so long, I find it quite disappointing that it was pushed without checking whether the tests work.

This revision is now accepted and ready to land.May 13 2017, 4:04 PM
hein added a comment.May 14 2017, 9:09 AM

FWIW I did run make test on an earlier revision of the patch during the sprint while doing work on top of it; it must have regressed after.

sebas closed this revision.Jun 7 2017, 2:08 PM

Issue has been fixed elsewhere, otherwise code is submitted.