D18824: Implement wl_eglstream_controller Server Interface

[Roman Gilg]

romangg added inline comments.

INLINE COMMENTS

> CMakeLists.txt:349
>    xdgoutput_interface.h
> +  eglstream_controller_interface.h
>  )

alphabetical sorted

> display.cpp:529
> +    } else {
> +        qCWarning(KWAYLAND_SERVER) << "Unable to load libnvidia-egl-wayland.so.1";
> +    }

There should be always a non-null object returned. Instead check somewhere before the create call of EglStreamControllerInterface if the library can be loaded. Also this would put the logic and the QLibrary include in the class file and not in display.cpp, what I would prefer.

One could overwrite the `Global::create` method in the `EglStreamControllerInterface` child class. From there then call QLibrary::resolve and only if it succeeds the parent create method. Afterwards compositor needs to check `Global::isValid`.

The control flow would then be in the compositor:

  auto *e = display->createEglStreamControllerInterface();
  // other initial setup
  e->create();
  if (!e->isValid()) {
      // error handling
  }

It's unfortunate that the create method is not virtual in Global. We can change this in KF6.

> display.h:310
> +     * @return the created EGL Stream controller, or nullptr on failure
> +     * @since 5.55
> +     */

5.56

> eglstream_controller_interface.cpp:53
> +    Q_UNUSED(client);
> +    Q_UNUSED(attribs);
> +    Private *p = static_cast<Private *>(wl_resource_get_user_data(resource));

Why is attribs unused in this implementation?

> eglstream_controller_interface.cpp:54
> +    Q_UNUSED(attribs);
> +    Private *p = static_cast<Private *>(wl_resource_get_user_data(resource));
> +    emit p->m_controller->streamConsumerAttached(SurfaceInterface::get(surface), (void *)eglStream);

Use `Global::Private::cast`.

> eglstream_controller_interface.cpp:58
> +
> +EglStreamControllerInterface::Private::Private(EglStreamControllerInterface *controller,
> +                                               Display *display,

Rename `controller` and `m_controller` below to `q` for enhanced pimpl street cred.

> eglstream_controller_interface.h:28
> +#include "surface_interface.h"
> +
> +namespace KWayland

Cleanup includes

REPOSITORY
  R127 KWayland

REVISION DETAIL
  https://phabricator.kde.org/D18824

To: ekurzinger, romangg, davidedmundson, zzag, #kwin
Cc: kde-frameworks-devel, michaelh, ngraham, bruns
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-frameworks-devel/attachments/20190209/7c94053f/attachment.html>