Make use of foreign protocol
ClosedPublic

Authored by mart on Aug 24 2017, 4:45 PM.

Details

Summary

not sure if that is the right "parent" that has
to be set, set the transient for surfaces that
have setParentOf of an imported set

Test Plan

m_XdgForeign->transientFor(surface()) finds the surface is expected,
not sure how to test it further

Diff Detail

Repository
R108 KWin
Branch
master
Lint
No Linters Available
Unit
No Unit Test Coverage
mart created this revision.Aug 24 2017, 4:45 PM
Restricted Application added a project: KWin. · View Herald TranscriptAug 24 2017, 4:45 PM
Restricted Application added subscribers: KWin, kwin, plasma-devel. · View Herald Transcript

Looks good to me.

not sure how to test it further

If you did want to autotest it, it'd be in the integration folder; it uses the real shell_client with a fake workspace.

If you want a real world use-case for this:

kde-polkit-agent-1 has a special bit of code to pass a WId (currently an int) to the agent before making a polkit call.
kwallet also passes a WId in the open() call. Both will require lots of API changes though :(

graesslin requested changes to this revision.Aug 24 2017, 5:54 PM
graesslin added a subscriber: graesslin.
graesslin added inline comments.
shell_client.cpp
1319

I would delegate this to waylandServer, so that there is no need to pass the foreign global to each ShellClient.

1403

This connect could be done in waylandServer then there would not be a reason to have this method at all.

shell_client.h
129

Please drop the UnstableInterface.

212

No need to explicitly set the QPointer to nullptr.

wayland_server.cpp
162

Why is the xdgForeign passed to the client?

This revision now requires changes to proceed.Aug 24 2017, 5:54 PM
mart added inline comments.Aug 25 2017, 1:03 PM
shell_client.cpp
1319

how could it be done as addtransient/transientfor/settransient are all protected?

mart added a comment.Aug 25 2017, 2:58 PM

one way i could do, is to add setters for xdgshell to set transient from there, to then use its tracking, it would depend from xdgshell existing, but that's what the protocol says anyways

graesslin added inline comments.Aug 25 2017, 4:08 PM
shell_client.cpp
1319

what I was thinking of is something like:

SurfaceInterface *WaylandServer::findForeignForSurface(SurfaceInterface *surface)
{
    return m_XdgForeign->transientFor(surface);
}

and here a simple:

if (!s) {
    s = waylandSurface()->findForeignForSurface(surface());
}

but I think your question is more about how to do the connect: just make waylandServer forward the signal, so that ShellClient can connect to it.

What I don't want is that the global is passed to ShellClient - we don't do that for any other global in ShellClient. Normally it always goes through WaylandServer. There are very few exceptions to it where a class has direct access to a global.

mart updated this revision to Diff 18761.Aug 25 2017, 5:03 PM
mart edited edge metadata.
  • use xdgshell setTransientFor
Restricted Application edited projects, added Plasma; removed KWin. · View Herald TranscriptAug 25 2017, 5:03 PM
mart added a comment.Aug 25 2017, 5:06 PM

here a different approach, if the other one is better, i can get back to it and add WaylandServer::findForeignForSurface

mart updated this revision to Diff 18766.Aug 25 2017, 5:45 PM
  • use WaylandServer::findForeignTransientForSurface
Restricted Application edited projects, added KWin; removed Plasma. · View Herald TranscriptAug 25 2017, 5:45 PM
mart added a comment.Aug 25 2017, 5:49 PM

it still does some weird hoops to set it modal to the parent, it should probably be set somehow client side but doesn't seem to be yet anything in xdgshell?

What I absolutely don't understand is why you want to make it modal. I don't see a reason for this to be modal and the X11 variant where this was used is not modal. E.g. kwallet is not modal, polkit is not modal and a file dialog through a flatpack portal should not be modal .

Furthermore our Wayland implementation does not even support any modal windows yet:

AbstractClient *ShellClient::findModal(bool allow_itself)
{
    Q_UNUSED(allow_itself)
    return nullptr;
}
shell_client.h
65–67

I don't see this new method being used.

wayland_server.cpp
165–172

why the setModal? Why should a foreign transient be modal?

173

as an alternative feel free to make ShellClient::setTransient public (it's only private because nothing used it so far) and do:

 if (childClient && childClient->surface() == child) {
    childClient->setTransient();
}
wayland_server.h
193

why add the parent if it's never used?

Ok a file open dialog might be modal, but also not everywhere (e.g. it is in Firefox, but isn't in libreoffice)

There's a major pitfall to inter-process modality:
In case of a persistent transient (used by several clients), the leader may loose the focus access "forever" (because the modal window remains)

Thus and in general, transients should certainly not be unconditionally modal, but the ability to control this attribute to the WM will be useful.
If client A crucially relies on input in client B to proceed, it will typically reject internal focus distribution and input handling.

This is complex enough if the client spawns a nested event loop and tries to pass the focus on to the foreign window, but in a multi-process case, client A might simply block for the I/O to gain this behavior.
In this case a "dead" window might receive the input focus.

Also the modality controls the forced visibility of the transient along the leader.

mart updated this revision to Diff 18958.Aug 30 2017, 12:43 PM
  • use WaylandServer::findForeignTransientForSurface
Restricted Application edited projects, added Plasma; removed KWin. · View Herald TranscriptAug 30 2017, 12:43 PM
This revision was automatically updated to reflect the committed changes.
mart marked 4 inline comments as done.
mart reopened this revision.Aug 30 2017, 1:30 PM
mart updated this revision to Diff 18964.Aug 30 2017, 1:30 PM
  • use xdgshell setTransientFor
  • use WaylandServer::findForeignTransientForSurface
  • drop unused code
Restricted Application edited projects, added KWin; removed Plasma. · View Herald TranscriptAug 30 2017, 1:30 PM
mart added a comment.Aug 30 2017, 1:32 PM

removed the modal hack completely

mart retitled this revision from [WIP] make use of foreign protocol to Make use of foreign protocol.Sep 5 2017, 1:17 PM
mart updated this revision to Diff 20554.Oct 10 2017, 11:13 AM
  • adapt to api changes
Restricted Application edited projects, added Plasma; removed KWin. · View Herald TranscriptOct 10 2017, 11:13 AM
graesslin accepted this revision.Oct 10 2017, 6:03 PM
This revision is now accepted and ready to land.Oct 10 2017, 6:03 PM
This revision was automatically updated to reflect the committed changes.