[Feature Request] Build wayland suppport optionally #9031

mid-kid · 2020-11-09T14:26:36Z

Is your feature request related to a problem?

My issue is mostly that I don't want to install qtgui with wayland support to build telegram, since I don't use wayland.

Describe the solution you'd like

A build flag, like the one for GTK+ (TDESKTOP_DISABLE_GTK_INTEGRATION)

Describe alternatives you've considered

Uhh putting up with it, but I think it's trivial enough to add a flag for it instead.

Additional context

I'm aware most distributions that support wayland probably ship wayland at this point, even if you don't use it, because it's a runtime dependency of gtk for some reason, but there's still minimalistic distributions that don't.
I've patched out wayland support manually, for version 2.4.5, and it seems to build and work correctly. Here's the patch:
EDIT: Snipped - see comment below.

What needs to be done is simply convert this ad-hoc patch into an actual CMake flag, but I have no clue how to do that. Might figure it out later and submit a proper PR.

The text was updated successfully, but these errors were encountered:

mid-kid · 2020-11-09T16:28:22Z

Since the patch spans across 3 repositories, and it has to be merged all at the same time, and I have no clue how to do it properly for this repo, I'll paste my proposed patch here:

--- a/Telegram/CMakeLists.txt
+++ b/Telegram/CMakeLists.txt
@@ -89,7 +89,7 @@ if (LINUX)
         )
     endif()
 
-    if (DESKTOP_APP_USE_PACKAGED AND Qt5WaylandClient_VERSION VERSION_LESS 5.13.0)
+    if (NOT DESKTOP_APP_DISABLE_WAYLAND_INTEGRATION AND DESKTOP_APP_USE_PACKAGED AND Qt5WaylandClient_VERSION VERSION_LESS 5.13.0)
         find_package(PkgConfig REQUIRED)
         pkg_check_modules(WAYLAND_CLIENT REQUIRED wayland-client)
 
--- a/Telegram/SourceFiles/platform/linux/specific_linux.cpp
+++ b/Telegram/SourceFiles/platform/linux/specific_linux.cpp
@@ -31,10 +31,16 @@ https://github.com/telegramdesktop/tdesktop/blob/master/LEGAL
 #include <QtCore/QLibraryInfo>
 #include <QtGui/QWindow>
 
+#ifndef DESKTOP_APP_DISABLE_WAYLAND_INTEGRATION
 #include <private/qwaylanddisplay_p.h>
 #include <private/qwaylandwindow_p.h>
 #include <private/qwaylandshellsurface_p.h>
 
+#if QT_VERSION < QT_VERSION_CHECK(5, 13, 0) && !defined DESKTOP_APP_QT_PATCHED
+#include <wayland-client.h>
+#endif // Qt < 5.13 && !DESKTOP_APP_QT_PATCHED
+#endif // !DESKTOP_APP_DISABLE_WAYLAND_INTEGRATION
+
 #ifndef DESKTOP_APP_DISABLE_DBUS_INTEGRATION
 #include <QtDBus/QDBusInterface>
 #include <QtDBus/QDBusConnection>
@@ -46,10 +52,6 @@ https://github.com/telegramdesktop/tdesktop/blob/master/LEGAL
 #include <xcb/xcb.h>
 #include <xcb/screensaver.h>
 
-#if QT_VERSION < QT_VERSION_CHECK(5, 13, 0) && !defined DESKTOP_APP_QT_PATCHED
-#include <wayland-client.h>
-#endif // Qt < 5.13 && !DESKTOP_APP_QT_PATCHED
-
 #include <glib.h>
 
 extern "C" {
@@ -69,7 +71,9 @@ extern "C" {
 
 using namespace Platform;
 using Platform::File::internal::EscapeShell;
+#ifndef DESKTOP_APP_DISABLE_WAYLAND_INTEGRATION
 using QtWaylandClient::QWaylandWindow;
+#endif
 
 namespace Platform {
 namespace {
@@ -450,6 +454,7 @@ bool ShowXCBWindowMenu(QWindow *window) {
 }
 
 bool StartWaylandMove(QWindow *window) {
+#ifndef DESKTOP_APP_DISABLE_WAYLAND_INTEGRATION
 	// There are startSystemMove on Qt 5.15
 #if QT_VERSION < QT_VERSION_CHECK(5, 15, 0) && !defined DESKTOP_APP_QT_PATCHED
 	if (const auto waylandWindow = static_cast<QWaylandWindow*>(
@@ -461,11 +466,13 @@ bool StartWaylandMove(QWindow *window) {
 		}
 	}
 #endif // Qt < 5.15 && !DESKTOP_APP_QT_PATCHED
+#endif // !DESKTOP_APP_DISABLE_WAYLAND_INTEGRATION
 
 	return false;
 }
 
 bool StartWaylandResize(QWindow *window, Qt::Edges edges) {
+#ifndef DESKTOP_APP_DISABLE_WAYLAND_INTEGRATION
 	// There are startSystemResize on Qt 5.15
 #if QT_VERSION < QT_VERSION_CHECK(5, 15, 0) && !defined DESKTOP_APP_QT_PATCHED
 	if (const auto waylandWindow = static_cast<QWaylandWindow*>(
@@ -483,11 +490,13 @@ bool StartWaylandResize(QWindow *window, Qt::Edges edges) {
 		}
 	}
 #endif // Qt < 5.15 && !DESKTOP_APP_QT_PATCHED
+#endif // !DESKTOP_APP_DISABLE_WAYLAND_INTEGRATION
 
 	return false;
 }
 
 bool ShowWaylandWindowMenu(QWindow *window) {
+#ifndef DESKTOP_APP_DISABLE_WAYLAND_INTEGRATION
 #if QT_VERSION >= QT_VERSION_CHECK(5, 13, 0) || defined DESKTOP_APP_QT_PATCHED
 	if (const auto waylandWindow = static_cast<QWaylandWindow*>(
 		window->handle())) {
@@ -498,6 +507,7 @@ bool ShowWaylandWindowMenu(QWindow *window) {
 		}
 	}
 #endif // Qt >= 5.13 || DESKTOP_APP_QT_PATCHED
+#endif // DESKTOP_APP_DISABLE_WAYLAND_INTEGRATION
 
 	return false;
 }
--- a/cmake/external/qt/package.cmake
+++ b/cmake/external/qt/package.cmake
@@ -24,7 +24,11 @@ find_package(Qt5 COMPONENTS Core Gui Widgets Network REQUIRED)
 find_package(Qt5Gui COMPONENTS QWebpPlugin REQUIRED)
 
 if (LINUX)
-    find_package(Qt5 COMPONENTS WaylandClient REQUIRED)
+    if (DESKTOP_APP_DISABLE_WAYLAND_INTEGRATION)
+        find_package(Qt5 OPTIONAL_COMPONENTS WaylandClient QUIET)
+    else()
+        find_package(Qt5 COMPONENTS WaylandClient REQUIRED)
+    endif()
     find_package(Qt5 OPTIONAL_COMPONENTS XkbCommonSupport QUIET)
 
     if (NOT DESKTOP_APP_USE_PACKAGED OR DESKTOP_APP_USE_PACKAGED_LAZY_PLATFORMTHEMES)
--- a/cmake/options.cmake
+++ b/cmake/options.cmake
@@ -26,6 +26,13 @@ if (DESKTOP_APP_DISABLE_DBUS_INTEGRATION)
     )
 endif()
 
+if (DESKTOP_APP_DISABLE_WAYLAND_INTEGRATION)
+    target_compile_definitions(common_options
+    INTERFACE
+        DESKTOP_APP_DISABLE_WAYLAND_INTEGRATION
+    )
+endif()
+
 if (DESKTOP_APP_DISABLE_WEBRTC_INTEGRATION)
     target_compile_definitions(common_options
     INTERFACE
--- a/cmake/variables.cmake
+++ b/cmake/variables.cmake
@@ -29,6 +29,7 @@ endif()
 
 option(DESKTOP_APP_LOTTIE_USE_CACHE "Use caching in lottie animations." ON)
 option(DESKTOP_APP_DISABLE_DBUS_INTEGRATION "Disable all code for D-Bus integration (Linux only)." OFF)
+option(DESKTOP_APP_DISABLE_WAYLAND_INTEGRATION "Disable all code for Wayland integration (Linux only)." OFF)
 option(DESKTOP_APP_DISABLE_WEBRTC_INTEGRATION "Disable all code for WebRTC integration." OFF)
 option(DESKTOP_APP_USE_GLIBC_WRAPS "Use wraps for new GLIBC features." OFF)
 option(DESKTOP_APP_USE_PACKAGED "Find libraries using CMake instead of exact paths." ${no_special_target})
--- a/Telegram/lib_base/base/platform/linux/base_info_linux.cpp
+++ b/Telegram/lib_base/base/platform/linux/base_info_linux.cpp
@@ -143,7 +143,11 @@ QString GetLibcVersion() {
 }
 
 bool IsWayland() {
+#ifndef DESKTOP_APP_DISABLE_WAYLAND_INTEGRATION
 	return QGuiApplication::platformName().startsWith("wayland", Qt::CaseInsensitive);
+#else
+	return false;
+#endif
 }
 
 void Start(QJsonObject options) {

ilya-fedin · 2020-11-09T19:04:35Z

#8282

mid-kid · 2020-11-09T20:18:29Z

Ah, I hadn't seen pull request #8351, I wonder if this patch can be considered an alternative to that?

ilya-fedin · 2020-11-09T20:22:08Z

Ah, I hadn't seen pull request #8351, I wonder if this patch can be considered an alternative to that?

No, as been discussed privately, adding a lot of ifdefs is not a way to go and @mymedia2 answered that it is too hard for him to implement this in the proper way

mid-kid · 2020-11-09T20:32:44Z

If "adding a lot of ifdefs" is the issue I'm sure there's a (possibly hacky) way to avoid the ifdefs in the StartWayland<x> functions. Ultimately all this is doing is shorting out checks that would otherwise be performed at runtime - the checks (specifically, every call to IsWayland()) are already there.

mid-kid · 2020-11-09T20:35:55Z

Heck, in an earlier draft of this patch I did:

#ifndef DESKTOP_APP_DISABLE_WAYLAND_INTEGRATION
bool StartWaylandMove(QWindow *window) {
    [...]
}
bool StartWaylandEtc...(QWindow *window) {
    [...]
}
#else
bool StartWaylandMove(QWindow *window) { return false; }
bool StartWaylandEtc...(QWindow *window) { return false; }
#endif

Would this be preferable?

If not, what about defining QWaylandWindow as some dummy type if the flag is enabled?

ilya-fedin · 2020-11-09T20:41:36Z

Would this be preferable?

I can forward preston's explanations on how this should be implemented to you, but they are in Russian

mid-kid · 2020-11-09T20:43:12Z

Why not? I have a friend who knows Russian.
I'm genuinely curious at this point, given the simplicity of the issue I don't really see a different way out.

ilya-fedin · 2020-11-09T20:44:35Z

John Preston, [31.07.20 16:21]
@mymedia Как минимум надо действительно сделать через DESKTOP_APP_DISABLE_WAYLAND_INTEGRATION, задаваемый в cmake и чтобы работало с -Werror=dev.

После этого про оба DESKTOP_APP_DISABLE_DBUS_INTEGRATION и DESKTOP_APP_DISABLE_WAYLAND_INTEGRATION можно будет рассуждать в контексте кода — как (и нужно ли) его реорганизовать таким образом, чтобы было не так непродираемо в #ifdef-ах в коде. Но это можно отложить.

John Preston, [31.07.20 16:26]
Скажем, это мог бы быть какой-нибудь синглтон Platform::WaylandIntegration() интерфейса Platform::WaylandIntegrationInterface.

В нём были бы чисто-виртуальные методы startMove, startResize.

При этом по дефайну DESKTOP_APP_DISABLE_WAYLAND_INTEGRATION мы бы возвращали там nullptr, а иначе компилировали бы реальный WaylandIntegrationImpl и возвращали его экземпляр. И получили бы простой

bool Platform::IsWayland() {
  return WaylandIntegration() != nullptr;
}

И в коде что-то типа:

bool StartSystemMove(QWindow *window) {
  if (const auto integration = WaylandIntegration()) {
    return integration->startMove(window);
  } else {
    return StartXCBMoveResize(window, 16);
  }
}

mid-kid · 2020-11-09T21:00:42Z

So, if I gather correctly, the proper way is to have two singletons, conditionally compiled depending on the flag, to both replace IsWayland in the checks as well as the StartWayland* functions?
I can try to do that, but I fail to see the difference with #9031 (comment). You'll need two versions of the functions no matter what you do, since they have to be defined somewhere, and the Wayland functions can't be compiled without the headers.

If I understood correctly and this is what you want, I'll look into doing this.

ilya-fedin · 2020-11-09T21:07:45Z

So, if I gather correctly, the proper way is to have two singletons, conditionally compiled depending on the flag, to both replace IsWayland in the checks as well as the StartWayland* functions?

Yeah, there should be Telegram/SourceFiles/platform/linux/linux_wayland_integration.cpp and Telegram/SourceFiles/platform/linux/linux_wayland_integration_dummy.cpp and all the wayland includes will be in the first, that way the only thing is needed to disable wayland dependency is to add the right file in CMakeLists.txt (like webrtc cmake option done)

What about IsWayland, I don't think it should be changed at all, since it will possible to build tdesktop without wayland and link to Qt with wayland and tdesktop should be able to do the platformName check to behave correctly.

mid-kid · 2020-11-09T21:18:44Z

I see, that makes sense now. Thanks!

mymedia2 · 2020-11-10T05:51:15Z

On Mon, 2020-11-09 at 13:08 -0800, ilya-fedin wrote: What about IsWayland, I don't think it should be changed at all, since it is possible to build tdesktop without wayland, while Qt with wayland and tdesktop should be able to do the platformName check to behave correctly.

Alas, at the moment, it is not possible to build telegram-desktop without Wayland. This is required dependency. https://github.com/telegramdesktop/tdesktop/blob/8a9317f9e11d3dca6a019bcf477bcc1a5927de1f/Telegram/CMakeLists.txt#L94 https://github.com/desktop-app/cmake_helpers/blob/d9e8a608c21ca175ed118955d83010580fb46e65/external/qt/package.cmake#L27

ilya-fedin · 2020-11-10T05:52:43Z

Alas, at the moment, it is not possible to build telegram-desktop without Wayland. This is required dependency.

Thank you, cap.

mymedia2 · 2020-11-10T05:59:12Z

ilya-fedin, is that news to you? You wrote Telegram works without Wayland, didn't you? But it is not correct, Wayland is required dependency.

mid-kid · 2020-11-11T21:09:30Z

Alright, so I've defined the behavior as follows:
If you're on Wayland, even without the wayland support baked in, the XCB calls shouldn't be executed. To that extent, IsWayland() is kept untouched. This means that the if (integration is built && is wayland) { wayland; } else { XCB; } isn't a good solution, as the only check should be if (is wayland).

I've written it with the singleton pattern as explained in the russian comment, and according to the IsWayland() rule specified by @ilya-fedin, in the following cross-repo diff:
https://hastebin.com/raw/zisupefawe

However, with the checks becoming if (is wayland) { if (integration is built) { wayland; } else { return false; } } else { XCB; }, I think that's ugly, so I'm proposing two other options:

Keep the singleton pattern, but return a valid instance regardless of whether support is builtin: https://hastebin.com/raw/kalixogihe
Just use static methods: https://hastebin.com/raw/pasewinari

All of these methods work and have been tested.
If any of these is to your liking, I'll make a proper pull request.

ilya-fedin · 2020-11-11T21:52:45Z

I think it is better to make this check multi-line

if (NOT DESKTOP_APP_DISABLE_WAYLAND_INTEGRATION
    AND DESKTOP_APP_USE_PACKAGED
    AND Qt5WaylandClient_VERSION VERSION_LESS 5.13.0)

PascalCase is only for static methods, generic class members should use camelCase.
It is better to rename GetInstance to Instance I guess
https://github.com/desktop-app/lib_ui/blob/f46a1be77c3ea7c23f016dd2b3375a39eb0a36cf/ui/integration.cpp#L23-L27
Why not just do the check in Instance?

WaylandIntegration *WaylandIntegration::Instance() {
    if (IsWayland()) {
        static WaylandIntegration instance;
        return &instance;
    }
    return nullptr;
}

Platform::internal namespace is a more right place for this class
You didn't adapt static build for the switch. There are already checks for Qt5DBus_FOUND in cmake/external/qt/CMakeLists.txt, Wayland checks should be like that.
There are

if (LINUX)
    add_checked_subdirectory(materialdecoration)
endif()

in cmake/external/CMakeLists.txt, it should become

if (LINUX AND NOT DESKTOP_APP_DISABLE_WAYLAND_INTEGRATION)
    add_checked_subdirectory(materialdecoration)
endif()

PS: line limit length for C++ code is 78 characters per line AFAIK

mid-kid · 2020-11-11T22:01:15Z

Because then any checks involving it will use the XCB branch instead, when not compiled, even when in wayland, which doesn't reflect the behavior of IsWayland() in https://github.com/telegramdesktop/tdesktop/blob/dev/Telegram/SourceFiles/platform/linux/specific_linux.cpp#L886 and similar checks.

The rest are stylistical things, I'll go through them and create a pull request.

ilya-fedin · 2020-11-11T22:10:33Z

The dummy variant of the class has these methods as well. Why not return an instance in the dummy variant as well?

mid-kid · 2020-11-11T22:11:36Z

Yeah, that's a good idea. I'll do that.

ilya-fedin · 2020-11-11T22:30:59Z

Is it possible to use a forward declaration of QWindow in linux_wayland_integration.h? 🤔

mid-kid · 2020-11-11T22:43:02Z

I think so, though I'm not sure what to do about Qt::Edges.

ilya-fedin · 2020-11-11T22:46:48Z

It should be included in PCH 🤔

github-actions · 2021-03-04T10:08:10Z

This issue has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

Aokromes closed this as completed Nov 9, 2020

mid-kid mentioned this issue Nov 9, 2020

Make Wayland free build possible #8351

Closed

This was referenced Nov 11, 2020

Build wayland support optionally #9051

Merged

Build wayland suppport optionally desktop-app/cmake_helpers#67

Merged

github-actions bot locked as resolved and limited conversation to collaborators Mar 4, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Feature Request] Build wayland suppport optionally #9031

[Feature Request] Build wayland suppport optionally #9031

mid-kid commented Nov 9, 2020 •

edited

mid-kid commented Nov 9, 2020 •

edited

ilya-fedin commented Nov 9, 2020

mid-kid commented Nov 9, 2020

ilya-fedin commented Nov 9, 2020 •

edited

mid-kid commented Nov 9, 2020

mid-kid commented Nov 9, 2020 •

edited

ilya-fedin commented Nov 9, 2020

mid-kid commented Nov 9, 2020

ilya-fedin commented Nov 9, 2020

mid-kid commented Nov 9, 2020

ilya-fedin commented Nov 9, 2020 •

edited

mid-kid commented Nov 9, 2020

mymedia2 commented Nov 10, 2020 via email

ilya-fedin commented Nov 10, 2020

mymedia2 commented Nov 10, 2020 via email

mid-kid commented Nov 11, 2020

ilya-fedin commented Nov 11, 2020

mid-kid commented Nov 11, 2020

ilya-fedin commented Nov 11, 2020

mid-kid commented Nov 11, 2020

ilya-fedin commented Nov 11, 2020

mid-kid commented Nov 11, 2020

ilya-fedin commented Nov 11, 2020

github-actions bot commented Mar 4, 2021

[Feature Request] Build wayland suppport optionally #9031

[Feature Request] Build wayland suppport optionally #9031

Comments

mid-kid commented Nov 9, 2020 • edited

Is your feature request related to a problem?

Describe the solution you'd like

Describe alternatives you've considered

Additional context

mid-kid commented Nov 9, 2020 • edited

ilya-fedin commented Nov 9, 2020

mid-kid commented Nov 9, 2020

ilya-fedin commented Nov 9, 2020 • edited

mid-kid commented Nov 9, 2020

mid-kid commented Nov 9, 2020 • edited

ilya-fedin commented Nov 9, 2020

mid-kid commented Nov 9, 2020

ilya-fedin commented Nov 9, 2020

mid-kid commented Nov 9, 2020

ilya-fedin commented Nov 9, 2020 • edited

mid-kid commented Nov 9, 2020

mymedia2 commented Nov 10, 2020 via email

ilya-fedin commented Nov 10, 2020

mymedia2 commented Nov 10, 2020 via email

mid-kid commented Nov 11, 2020

ilya-fedin commented Nov 11, 2020

mid-kid commented Nov 11, 2020

ilya-fedin commented Nov 11, 2020

mid-kid commented Nov 11, 2020

ilya-fedin commented Nov 11, 2020

mid-kid commented Nov 11, 2020

ilya-fedin commented Nov 11, 2020

github-actions bot commented Mar 4, 2021

mid-kid commented Nov 9, 2020 •

edited

mid-kid commented Nov 9, 2020 •

edited

ilya-fedin commented Nov 9, 2020 •

edited

mid-kid commented Nov 9, 2020 •

edited

ilya-fedin commented Nov 9, 2020 •

edited