Introduce QQuickItem to nest kwin_wayland
AbandonedPublic

Authored by bdhruve on Jun 24 2016, 6:46 AM.

Details

Reviewers
bshah
graesslin
Group Reviewers
Plasma on Wayland
Summary

This starts Wayland server with some basic interfaces

  • shm
  • compositor
  • seat
  • shell
  • output

And starts nested kwin_wayland

Diff Detail

Repository
R108 KWin
Branch
kwinqml
Lint
No Linters Available
Unit
No Unit Test Coverage
There are a very large number of changes, so older changes are hidden. Show Older Changes
bdhruve updated this revision to Diff 4797.Jun 28 2016, 11:24 AM

Fixed issues addressed by @graesslin

bdhruve added inline comments.Jun 28 2016, 11:55 AM
qml/kwinqml/kwinqml.h
34 ↗(On Diff #4697)

I have fixed all other issues, but i am not sure on how to do this?

bshah added a subscriber: bshah.Jun 29 2016, 9:39 AM

Some minor issues I noticed..

plugins/qml/kwinqml/CMakeLists.txt
14

Not required, you just use WaylandServer API.

plugins/qml/kwinqml/kwinplugin.h
2

This and all other header and C++ files are missing license headers,

See: https://community.kde.org/Policies/Licensing_Policy

plugins/qml/kwinqml/kwinqml.cpp
30–32

Coding style.

plugins/qml/kwinqml/kwinqml.h
2

You are missing include guard in this header file.

bshah requested changes to this revision.Jun 29 2016, 9:39 AM
bshah added a reviewer: bshah.
This revision now requires changes to proceed.Jun 29 2016, 9:39 AM
bdhruve updated this revision to Diff 4847.Jun 29 2016, 10:39 AM
bdhruve edited edge metadata.

Fixed issues mentioned by @bshah

bdhruve updated this revision to Diff 4848.Jun 29 2016, 10:53 AM
bdhruve edited edge metadata.

Fixed copyright header.

bshah requested changes to this revision.Jul 1 2016, 8:38 AM
bshah edited edge metadata.
bshah added inline comments.
plugins/qml/kwinqml/kwinqml.cpp
74–76

Somehow seems you removed code for passing socket through WAYLAND_SOCKET environment variable and passing of --xwayland argument from this code. which was in previous revisions..

This revision now requires changes to proceed.Jul 1 2016, 8:38 AM
bdhruve updated this revision to Diff 4886.Jul 1 2016, 9:48 AM
bdhruve edited edge metadata.

Fixed code as per mentioned by @bshah.

graesslin requested changes to this revision.Jul 1 2016, 12:03 PM
graesslin added a reviewer: graesslin.

I would like to see a test case which verifies that it starts the kwin_wayland instance and terminates it again.

plugins/qml/kwinqml/kwinplugin.cpp
25–26

Also here I dislike the name KWinApp and kwin.app - that's confusing due to kwinApp() already having a meaning.

plugins/qml/kwinqml/kwinqml.cpp
35–36

the call to the parent class is missing

80–82

we also need to pass at least --socket with a useful name. Otherwise it cannot be started in a Wayland session.

82

how is this instance being terminated again?

plugins/qml/kwinqml/kwinqml.h
38

please add documentation

41

QQuickItem *parent = nullptr

This revision now requires changes to proceed.Jul 1 2016, 12:03 PM

I would like to see a test case which verifies that it starts the kwin_wayland instance and terminates it again.

I am not sure on how to add testcase for this?
Here is code I use for testing locally:

import QtQuick 2.0
import org.kde.kwin.app 1.0
Text {

text: "Hello world"
KWinApp {
     id: kwin
     //socketName:"kwin-emulator-wayland-0"
     anchors.fill:parent
     Component.onCompleted: kwin.start()
}

}

bdhruve updated this revision to Diff 4932.Jul 4 2016, 11:46 AM
bdhruve edited edge metadata.

Fixed issues mentioned by @graesslin.

graesslin added inline comments.Jul 5 2016, 2:41 PM
config-kwin.h.cmake
6

best generate the absolute install path of the binary. We don't want a different kwin_wayland to be started just because it's in $PATH.

plugins/qml/kwinqml/kwinqml.cpp
48

why check isNull and isEmpty?

57

you also need to call create on m_shell.

plugins/qml/kwinqml/kwinqml.h
39

that's a comment and no doxygen documentation. For documentation you need to use

/**
 * Socket name to pass to Wayland Server Display
 **/
46

const QStrig &socketName

47–50

this also emits the changed if it does not change at all.

graesslin added inline comments.Jul 5 2016, 2:47 PM
plugins/qml/kwinqml/kwinqml.cpp
48–50

actually we don't need that. The socketName is for the kwin process which we start. Our Display in this class is set to ConnectClientsOnly, thus it won't create a socket.

bdhruve updated this revision to Diff 4972.Jul 6 2016, 7:48 AM
bdhruve edited edge metadata.

Fixed issues mentioned by @graesslin.

Looks good now and I think we can look into the next steps: rendering the KWin instance you launch. The launched KWin instance connects to your Wayland server, binds to the Shell and will create a ShellSurface. That you will get through a signal on m_shell - compare wayland_server.cpp:148. I would as a next step try to hook into this and see whether the surface gets created. Once you have that you can look into rendering it.

For that the ShellSurfaceInterface is connected to a SurfaceInterface. The rendering happens on the SurfaceInterface. There's a damaged signal which you should use to trigger a repaint of your QQuickItem. When rendering you can access the buffer on the SurfaceInterface. If KWin uses KWIN_COMPOSE=Q you can access the buffer as a QImage and just render it. Pretty straight forward - hopefully. But first try to see whether you get the ShellSurface created. If not we need to look into what goes wrong.

plugins/qml/kwinqml/kwinqml.cpp
74

There's one more env variable we should pass to the environment:
KWIN_COMPOSE=Q

In the long run we won't need that, but for the moment I think it makes our life easier. It forces the compositor to use QPainter instead of OpenGL, thus integrating the rendering will be way easier.

78–80

Please parse the arguments as:

QStringList arguments{QStringLiteral("--xwayland"), QStringLiteral("--socket"), m_socketName};

That way the list can be constructed in one go and not need to be appended multiple times.

81

I think we need to pass a few more arguments:

  • --width
  • --height

with the values set to the size of this argument.

plugins/qml/kwinqml/kwinqml.h
62–63

I still do not really see the point in those two methods. The start could be handled by implementing the componentComplete method. The stop by calling that from the destructor.

bdhruve updated this revision to Diff 5014.Jul 8 2016, 7:18 AM

Fixed issues mentioned by @graesslin and connect to surfaceCreated.

Looks good now and I think we can look into the next steps: rendering the KWin instance you launch. The launched KWin instance connects to your Wayland server, binds to the Shell and will create a ShellSurface. That you will get through a signal on m_shell - compare wayland_server.cpp:148. I would as a next step try to hook into this and see whether the surface gets created. Once you have that you can look into rendering it.

For that the ShellSurfaceInterface is connected to a SurfaceInterface. The rendering happens on the SurfaceInterface. There's a damaged signal which you should use to trigger a repaint of your QQuickItem. When rendering you can access the buffer on the SurfaceInterface. If KWin uses KWIN_COMPOSE=Q you can access the buffer as a QImage and just render it. Pretty straight forward - hopefully. But first try to see whether you get the ShellSurface created. If not we need to look into what goes wrong.

I've added the slot for checking if surface gets created but signal doesn't get emitted. Would you please show me the way to proceed further now.

Looks good now and I think we can look into the next steps: rendering the KWin instance you launch. The launched KWin instance connects to your Wayland server, binds to the Shell and will create a ShellSurface. That you will get through a signal on m_shell - compare wayland_server.cpp:148. I would as a next step try to hook into this and see whether the surface gets created. Once you have that you can look into rendering it.

For that the ShellSurfaceInterface is connected to a SurfaceInterface. The rendering happens on the SurfaceInterface. There's a damaged signal which you should use to trigger a repaint of your QQuickItem. When rendering you can access the buffer on the SurfaceInterface. If KWin uses KWIN_COMPOSE=Q you can access the buffer as a QImage and just render it. Pretty straight forward - hopefully. But first try to see whether you get the ShellSurface created. If not we need to look into what goes wrong.

I've added the slot for checking if surface gets created but signal doesn't get emitted. Would you please show me the way to proceed further now.

I am not getting how to proceed further now, can you please help me on this? Also, current confusion i have is, should kwin_wayland nested window appear when i run qmlscene? Because currently it does appear.

I am not getting how to proceed further now, can you please help me on this? Also, current confusion i have is, should kwin_wayland nested window appear when i run qmlscene? Because currently it does appear.

No, it should not show. If it shows it means that it picks the wrong backend. This of course explains why the slot is not hit. It doesn't connect to the Wayland server you start thus it never tries to create a surface.

Now the question is why it doesn't pick the right platform. The evaluation for that is in main_wayland.cpp - if I see correctly the problem is that there it's only checked for env variable WAYLAND_DISPLAY, but not also for WAYLAND_SOCKET.

bdhruve updated this revision to Diff 5302.Jul 19 2016, 12:07 PM
  • Fixed automatic selection of backend to look for WAYLAND_SOCKET also
  • Rendering kwin instance
  • Fixed minor issues.
graesslin requested changes to this revision.Jul 20 2016, 2:27 PM
graesslin edited edge metadata.
graesslin added inline comments.
plugins/qml/kwinqml/kwinqml.cpp
64

don't use qDebug, please use categorized logging.

69

ideally you don't need to repaint everything, but could just update the actual changed areas (carried in the damaged signal)

107

please watch coding style

113

you cannot just assume that there's a buffer attached to the surface. Please perform a nullptr check

This revision now requires changes to proceed.Jul 20 2016, 2:27 PM
bshah added inline comments.Jul 21 2016, 3:46 AM
plugins/qml/kwinqml/kwinqml.cpp
64

I'd say just remove this and debug statement below, they can get noisy, because it will be triggered at every frame repaint.

113–114

You need to move this out of the if statement, otherwise only first frame will be rendered.

bdhruve updated this revision to Diff 5389.Jul 21 2016, 12:20 PM
bdhruve edited edge metadata.
  • Performed the nullptr check for the buffer attached to the surface.
  • Removed qDebug() statements.
  • Fixed coding style.
  • Fixed issue of only first frame rendering.
bdhruve added inline comments.Jul 21 2016, 12:35 PM
plugins/qml/kwinqml/kwinqml.cpp
69

I am not much knowledgeable about how to render only specific area in QtQuick, i tried to search for it, but couldn't find anything related to it.

bdhruve updated this revision to Diff 5713.Aug 7 2016, 12:29 PM
bdhruve edited edge metadata.

Pass different input events from QML to Kwin,

  • Use mouse hover events to set pointer position.
  • Mouse press/release events to send pointer button events.
  • KeyPress events to send keys.

However, keyevents are not working as it should, if i press 'a' it types something different.

  • KeyPress events to send keys.

    However, keyevents are not working as it should, if i press 'a' it types something different.

might be that this is just not possible to send key events like that. In general QKeyEvent delivers the keysym. That is the scan code translated through the keyboard layout. The nativeSccanCode should have the original value but I would not trust it completely.

Maybe leave key events out for the moment as an we don't necessarily need key events.

  • KeyPress events to send keys.

    However, keyevents are not working as it should, if i press 'a' it types something different.

might be that this is just not possible to send key events like that. In general QKeyEvent delivers the keysym. That is the scan code translated through the keyboard layout. The nativeSccanCode should have the original value but I would not trust it completely.

Maybe leave key events out for the moment as an we don't necessarily need key events.

Ok, I will remove keyEvent stuff and add to-do item for that. Once that is done are there any issues in this code?

bdhruve updated this revision to Diff 5753.Aug 9 2016, 7:35 AM

Remove keyEvent handling.

I just remembered: on X11 one needs to subtract 8 from the key code...

bdhruve updated this revision to Diff 5755.Aug 9 2016, 8:06 AM

Added back the keyEvent handling and works perfectly now.

I just remembered: on X11 one needs to subtract 8 from the key code...

The keyEvents are working perfectly now because of the solution you gave. Thank you. :-)
Are there any other issues in the code?

graesslin added inline comments.Aug 9 2016, 1:31 PM
plugins/qml/kwinqml/kwinqml.cpp
154

only do that on X11, if we are on Wayland that would be wrong. So do something like:

const int magicOffset = KWindowSystem::isPlatformX11() ? 8 : 0;
m_seat->keyPressed(event->nativeScanCode() - magicOffset);

And I'm glad it was the 8 issue, luckily I remembered that - having stumbled over this weirdness myself in the past.

bdhruve updated this revision to Diff 5781.Aug 10 2016, 7:35 AM

Fix keyEvent code for non-X11 platforms.

bdhruve marked 28 inline comments as done.Aug 10 2016, 7:59 AM

Mark comments as done.

I would like to see this integrated. But I think a few things need to be done:

  • I would love to see the example you had in your blog post added to tests
  • I'm still unhappy about the name KWinQml - it's just not saying what it is. And actually it's not Qml at all. It's a declarative implementation. Might need brainstorming, maybe even broader on the mailinglist.

I would like to see this integrated. But I think a few things need to be done:

  • I would love to see the example you had in your blog post added to tests
  • I'm still unhappy about the name KWinQml - it's just not saying what it is. And actually it's not Qml at all. It's a declarative implementation. Might need brainstorming, maybe even broader on the mailinglist.

Alright i will do the said things. And regarding the name, how does KWinDeclarative sound to you?

Alright i will do the said things. And regarding the name, how does KWinDeclarative sound to you?

It still doesn't describe what it is. Thinking about it, I would suggest a name with "embedded" or "nested". KWin doesn't need to be part of the name - everything in KWin is about KWin after all. Granted the import name should include KWin. So for the import maybe org.kde.kwin.embedded?

davidedmundson added inline comments.
plugins/qml/kwinqml/kwinqml.cpp
45

this timer doesn't do anything, it's not started.

132

This texture leaks when this item is destroyed

171

you also want:

m_seat->setTimestamp(event->timestamp());

in all of these

and there's a wheelEvent(QWheelEvent*) that should be forwarded.

bdhruve abandoned this revision.Apr 25 2017, 12:17 PM

This revision was started last year which had lot of discussions. Had a few of unfinished issues to be fixed as well.
So creating a new one which has fixed the last few issues addressed by @graesslin and @davidedmundson. And i am extremely apologetic for such a long delay.