Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Build wayland support optionally #9051

Merged
merged 5 commits into from Nov 17, 2020
Merged

Build wayland support optionally #9051

merged 5 commits into from Nov 17, 2020

Conversation

mid-kid
Copy link
Contributor

@mid-kid mid-kid commented Nov 11, 2020

Add DESKTOP_APP_DISABLE_WAYLAND_INTEGRATION to control the dependency on QtWayland.
Related feature request: #9031
Submodule pull request: desktop-app/cmake_helpers#67

@CLAassistant
Copy link

CLAassistant commented Nov 11, 2020

CLA assistant check
All committers have signed the CLA.

@mid-kid
Copy link
Contributor Author

mid-kid commented Nov 11, 2020

The ubuntu machine failed due to a completely unrelated issue...
https://github.com/telegramdesktop/tdesktop/pull/9051/checks?check_run_id=1387827052#step:26:131

@ilya-fedin
Copy link
Contributor

ilya-fedin commented Nov 11, 2020

window_title_linux.h should be changed accrodingly

diff --git a/Telegram/CMakeLists.txt b/Telegram/CMakeLists.txt
index 85f68e378..666a20a4d 100644
--- a/Telegram/CMakeLists.txt
+++ b/Telegram/CMakeLists.txt
@@ -808,6 +808,7 @@ PRIVATE
     platform/linux/notifications_manager_linux.h
     platform/linux/specific_linux.cpp
     platform/linux/specific_linux.h
+    platform/linux/window_title_linux.cpp
     platform/linux/window_title_linux.h
     platform/mac/file_utilities_mac.mm
     platform/mac/file_utilities_mac.h
diff --git a/Telegram/SourceFiles/platform/linux/window_title_linux.cpp b/Telegram/SourceFiles/platform/linux/window_title_linux.cpp
new file mode 100644
index 000000000..95e8b6449
--- /dev/null
+++ b/Telegram/SourceFiles/platform/linux/window_title_linux.cpp
@@ -0,0 +1,38 @@
+/*
+This file is part of Telegram Desktop,
+the official desktop application for the Telegram messaging service.
+
+For license and copyright information please follow this link:
+https://github.com/telegramdesktop/tdesktop/blob/master/LEGAL
+*/
+#include "platform/linux/window_title_linux.h"
+
+#include "platform/linux/linux_desktop_environment.h"
+#include "base/platform/base_platform_info.h"
+
+namespace Platform {
+namespace {
+
+bool SystemMoveResizeSupported() {
+#if !defined DESKTOP_APP_DISABLE_WAYLAND_INTEGRATION || QT_VERSION >= QT_VERSION_CHECK(5, 15, 0) || defined DESKTOP_APP_QT_PATCHED
+	return true;
+#else // !DESKTOP_APP_DISABLE_WAYLAND_INTEGRATION || Qt >= 5.15 || DESKTOP_APP_QT_PATCHED
+	return !IsWayland();
+#endif // !DESKTOP_APP_DISABLE_WAYLAND_INTEGRATION || Qt >= 5.15 || DESKTOP_APP_QT_PATCHED
+}
+
+} // namespace
+
+bool AllowNativeWindowFrameToggle() {
+	return SystemMoveResizeSupported()
+		// https://gitlab.gnome.org/GNOME/mutter/-/issues/217
+		&& !(DesktopEnvironment::IsGnome() && IsWayland());
+}
+
+object_ptr<Window::TitleWidget> CreateTitleWidget(QWidget *parent) {
+	return SystemMoveResizeSupported()
+		? object_ptr<Window::TitleWidgetQt>(parent)
+		: object_ptr<Window::TitleWidgetQt>{ nullptr };
+}
+
+} // namespace Platform
diff --git a/Telegram/SourceFiles/platform/linux/window_title_linux.h b/Telegram/SourceFiles/platform/linux/window_title_linux.h
index 4e5f68e02..c493976c2 100644
--- a/Telegram/SourceFiles/platform/linux/window_title_linux.h
+++ b/Telegram/SourceFiles/platform/linux/window_title_linux.h
@@ -8,8 +8,6 @@ https://github.com/telegramdesktop/tdesktop/blob/master/LEGAL
 #pragma once
 
 #include "platform/platform_window_title.h"
-#include "platform/linux/linux_desktop_environment.h"
-#include "base/platform/base_platform_info.h"
 #include "base/object_ptr.h"
 
 namespace Window {
@@ -23,14 +21,8 @@ void DefaultPreviewWindowFramePaint(QImage &preview, const style::palette &palet
 
 namespace Platform {
 
-inline bool AllowNativeWindowFrameToggle() {
-	// https://gitlab.gnome.org/GNOME/mutter/-/issues/217
-	return !(DesktopEnvironment::IsGnome() && IsWayland());
-}
-
-inline object_ptr<Window::TitleWidget> CreateTitleWidget(QWidget *parent) {
-	return object_ptr<Window::TitleWidgetQt>(parent);
-}
+bool AllowNativeWindowFrameToggle();
+object_ptr<Window::TitleWidget> CreateTitleWidget(QWidget *parent);
 
 inline bool NativeTitleRequiresShadow() {
 	return false;

PS: edited multiple times

@mid-kid
Copy link
Contributor Author

mid-kid commented Nov 11, 2020

Ah, that explains why the telegram window title was suddently popping up...

Comment on lines +15 to +20
class WaylandIntegration {
public:
static WaylandIntegration *Instance();
bool startMove(QWindow *window);
bool startResize(QWindow *window, Qt::Edges edges);
bool showWindowMenu(QWindow *window);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just observed that you get that forward quitly wrong, WaylandIntegration should contain virtual methods, while the non-dummy implementation should be WaylandIntegrationImpl and have WaylandIntegration as a parent.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure what preston meant by WaylandIntegrationInterface, my C++ knowledge is not so advanced

Copy link
Contributor Author

@mid-kid mid-kid Nov 12, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pardon my lack of C++ knowledge, but I'm not sure how that would work? You'd have a WaylandIntegrationImpl with no non-wayland implementation, which would fail building without wayland (as no WaylandIntegrationImpl would exist).
EDIT: Nevermind, I think I know what you mean.

@ilya-fedin
Copy link
Contributor

Just remembered about qt_static_plugins.cpp

diff --git a/Telegram/SourceFiles/qt_static_plugins.cpp b/Telegram/SourceFiles/qt_static_plugins.cpp
index 5dffd6a7a..21c0980f2 100644
--- a/Telegram/SourceFiles/qt_static_plugins.cpp
+++ b/Telegram/SourceFiles/qt_static_plugins.cpp
@@ -21,6 +21,18 @@ Q_IMPORT_PLUGIN(QWindowsIntegrationPlugin)
 Q_IMPORT_PLUGIN(QCocoaIntegrationPlugin)
 Q_IMPORT_PLUGIN(QGenericEnginePlugin)
 #elif defined Q_OS_UNIX // Q_OS_WIN | Q_OS_MAC
+Q_IMPORT_PLUGIN(QXcbIntegrationPlugin)
+Q_IMPORT_PLUGIN(QGenericEnginePlugin)
+Q_IMPORT_PLUGIN(QComposePlatformInputContextPlugin)
+Q_IMPORT_PLUGIN(QSvgPlugin)
+Q_IMPORT_PLUGIN(QSvgIconPlugin)
+#ifndef DESKTOP_APP_DISABLE_DBUS_INTEGRATION
+Q_IMPORT_PLUGIN(QConnmanEnginePlugin)
+Q_IMPORT_PLUGIN(QNetworkManagerEnginePlugin)
+Q_IMPORT_PLUGIN(QIbusPlatformInputContextPlugin)
+Q_IMPORT_PLUGIN(QXdgDesktopPortalThemePlugin)
+#endif // !DESKTOP_APP_DISABLE_DBUS_INTEGRATION
+#ifndef DESKTOP_APP_DISABLE_WAYLAND_INTEGRATION
 Q_IMPORT_PLUGIN(ShmServerBufferPlugin)
 Q_IMPORT_PLUGIN(DmaBufServerBufferPlugin)
 Q_IMPORT_PLUGIN(DrmEglServerBufferPlugin)
@@ -31,31 +43,23 @@ Q_IMPORT_PLUGIN(QWaylandXdgShellV5IntegrationPlugin)
 Q_IMPORT_PLUGIN(QWaylandXdgShellV6IntegrationPlugin)
 Q_IMPORT_PLUGIN(QWaylandXdgShellIntegrationPlugin)
 Q_IMPORT_PLUGIN(QWaylandBradientDecorationPlugin)
-Q_IMPORT_PLUGIN(QXcbIntegrationPlugin)
 Q_IMPORT_PLUGIN(QWaylandIntegrationPlugin)
 Q_IMPORT_PLUGIN(QWaylandEglPlatformIntegrationPlugin)
-Q_IMPORT_PLUGIN(QGenericEnginePlugin)
-Q_IMPORT_PLUGIN(QComposePlatformInputContextPlugin)
-Q_IMPORT_PLUGIN(QSvgPlugin)
-Q_IMPORT_PLUGIN(QSvgIconPlugin)
-#ifndef DESKTOP_APP_DISABLE_DBUS_INTEGRATION
-Q_IMPORT_PLUGIN(QConnmanEnginePlugin)
-Q_IMPORT_PLUGIN(QNetworkManagerEnginePlugin)
-Q_IMPORT_PLUGIN(QIbusPlatformInputContextPlugin)
-Q_IMPORT_PLUGIN(QXdgDesktopPortalThemePlugin)
-#endif // !DESKTOP_APP_DISABLE_DBUS_INTEGRATION
+#endif // !DESKTOP_APP_DISABLE_WAYLAND_INTEGRATION
 #endif // Q_OS_WIN | Q_OS_MAC | Q_OS_UNIX
 #endif // !DESKTOP_APP_USE_PACKAGED
 
 #if defined Q_OS_UNIX && !defined Q_OS_MAC
 #if !defined DESKTOP_APP_USE_PACKAGED || defined DESKTOP_APP_USE_PACKAGED_LAZY
-Q_IMPORT_PLUGIN(QWaylandMaterialDecorationPlugin)
 Q_IMPORT_PLUGIN(NimfInputContextPlugin)
 #ifndef DESKTOP_APP_DISABLE_DBUS_INTEGRATION
 Q_IMPORT_PLUGIN(QFcitxPlatformInputContextPlugin)
 Q_IMPORT_PLUGIN(QFcitx5PlatformInputContextPlugin)
 Q_IMPORT_PLUGIN(QHimePlatformInputContextPlugin)
 #endif // !DESKTOP_APP_DISABLE_DBUS_INTEGRATION
+#ifndef DESKTOP_APP_DISABLE_WAYLAND_INTEGRATION
+Q_IMPORT_PLUGIN(QWaylandMaterialDecorationPlugin)
+#endif // !DESKTOP_APP_DISABLE_WAYLAND_INTEGRATION
 #endif // !DESKTOP_APP_USE_PACKAGED || DESKTOP_APP_USE_PACKAGED_LAZY
 
 #if !defined DESKTOP_APP_USE_PACKAGED || defined DESKTOP_APP_USE_PACKAGED_LAZY_PLATFORMTHEMES

@ilya-fedin
Copy link
Contributor

The ubuntu machine failed due to a completely unrelated issue...

Snap action works and looks like you forgot to move a method:

[ 88%] Building CXX object Telegram/CMakeFiles/Telegram.dir/SourceFiles/platform/linux/linux_wayland_integration.cpp.o
/root/parts/telegram/src/Telegram/SourceFiles/platform/linux/linux_wayland_integration.cpp: In member function ‘bool Platform::internal::WaylandIntegration::startResize(QWindow*, Qt::Edges)’:
/root/parts/telegram/src/Telegram/SourceFiles/platform/linux/linux_wayland_integration.cpp:62:32: error: ‘WlResizeFromEdges’ was not declared in this scope
   62 |     shellSurface->resize(seat, WlResizeFromEdges(edges));
      |                                ^~~~~~~~~~~~~~~~~

I guess it could be moved to a private namespace in linux_wayland_integration.cpp

@ilya-fedin
Copy link
Contributor

Wow, it builds!

@mid-kid
Copy link
Contributor Author

mid-kid commented Nov 13, 2020

Heck yes it does! I also had to fix linking with the material decoration here, aside from the things you mentioned.

I've been looking at the thing you mentioned with virtuals, but I'm having a bit of trouble with it, and I don't know if there's any way to avoid having to exclude the dummy version from the build because ::Instance() is static and has to be overridden to get the instance of WaylandIntegrationImpl instead.

@ilya-fedin
Copy link
Contributor

I've been looking at the thing you mentioned with virtuals, but I'm having a bit of trouble with it, and I don't know if there's any way to avoid having to exclude the dummy version from the build because ::Instance() is static and has to be overridden to get the instance of WaylandIntegrationImpl instead.

Well, I don't know what to say, it looks good for me as is, maybe @john-preston can help, it's he who written that post from the forward

@john-preston
Copy link
Member

@mid-kid Thanks. I've merged the submodule PR, can you please force-push this PR so that it uses only the commit that was created in desktop-app/cmake_helpers.

@mid-kid
Copy link
Contributor Author

mid-kid commented Nov 17, 2020

Done

@mid-kid
Copy link
Contributor Author

mid-kid commented Nov 17, 2020

I misread your comment, now it's done.

@john-preston
Copy link
Member

Thanks.

@john-preston john-preston merged commit 4948000 into telegramdesktop:dev Nov 17, 2020
@github-actions
Copy link

github-actions bot commented Mar 3, 2021

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

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 3, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants