Native notifications on Linux without libnotify #6825

ilya-fedin · 2019-11-26T22:50:26Z

This change removes the need of gtk for native notifications on Linux
Closes #1026

CLAassistant · 2019-11-26T22:50:40Z

All committers have signed the CLA.

ilya-fedin · 2019-11-29T03:10:25Z

Added possible fix for #1026 and (duplicate) #6507
But I have no idea for which notification daemon there was a hack that caused this problem

Aokromes · 2019-11-29T04:11:32Z

@ilya-fedin edit 1st post and add a note "closes #1026" plz :)

ilya-fedin · 2019-11-30T06:17:44Z

@Aokromes ok

ilya-fedin · 2019-11-30T07:36:38Z

🤔 travis-ci failed with -Werror=enum-compare

/home/travis/build/telegramdesktop/tdesktop/upstream/Telegram/SourceFiles/platform/linux/notifications_manager_linux.cpp: In member function ‘QStringList Platform::Notifications::NotificationData::getServerInformation()’:

/home/travis/build/telegramdesktop/tdesktop/upstream/Telegram/SourceFiles/platform/linux/notifications_manager_linux.cpp:90:67: error: comparison between ‘enum QVariant::Type’ and ‘enum QMetaType::Type’ [-Werror=enum-compare]

    if (serverInformationReply.arguments()[i].type() == QMetaType::QString) {

But Qt documentation recommends comparsion with QMetaType https://doc.qt.io/QT-5/qvariant.html#type

Should I add -Wno-enum-compare to build scripts?

23rd · 2019-11-30T14:28:03Z

Should I add -Wno-enum-compare to build scripts?

You can just replace type() with userType().
So it will be:

if (serverInformationReply.arguments()[i].userType() == QMetaType::QString) {

Telegram/SourceFiles/platform/linux/notifications_manager_linux.cpp

ilya-fedin · 2019-12-01T00:36:18Z

I remembered that this PR also fixes #3750, because now there is no call to GetServerName in Supported(), instead the interface is checking for validity. I don’t know if I need to add this to the first post, because issue is auto-closed.

23rd · 2019-12-01T08:05:17Z

Telegram/SourceFiles/platform/linux/notifications_manager_linux.cpp

-//			}
-//			Libs::notify_notification_clear_actions(_data);
-			Libs::g_object_unref(Libs::g_object_cast(_data));
+	if (std::find(capabilities.begin(), capabilities.end(), qsl("body-markup")) != capabilities.end()) {


All such lines can be replaced with the range-v3.
Something like this.

if (ranges::find(capabilities, qsl("body-markup")) != capabilities.end()) {

Also to shorten the code we can move the capabilities.end() to a new variable. For example.

const auto endCap = end(capabilities); if (ranges::find(capabilities, qsl("body-markup")) != endCap) { ... if (ranges::find(capabilities, qsl("actions")) != endCap) { ... etc

23rd · 2019-12-01T08:08:31Z

Telegram/SourceFiles/platform/linux/notifications_manager_linux.cpp

+QString NotificationData::escapeHtml(const QString &text) {
+	auto result = QString();
+	auto copyFrom = 0, textSize = text.size();
+	auto data = text.constData();
+	for (auto i = 0; i != textSize; ++i) {
+		auto ch = data[i];
+		if (ch == '<' || ch == '>' || ch == '&') {
+			if (!copyFrom) {
+				result.reserve(textSize * 5);
+			}
+			if (i > copyFrom) {
+				result.append(data + copyFrom, i - copyFrom);
+			}
+			switch (ch.unicode()) {
+			case '<': result.append(qstr("&lt;")); break;
+			case '>': result.append(qstr("&gt;")); break;
+			case '&': result.append(qstr("&amp;")); break;
+			}
+			copyFrom = i + 1;
+		}
+	}
+	if (copyFrom > 0) {
+		result.append(data + copyFrom, textSize - copyFrom);
+		return result;
+	}
+	return text;
+}


I'm not sure, but why we can't just use toHtmlEscaped()?

Hmm, this code is from the old implementation, I just reused it

23rd · 2019-12-01T08:15:23Z

Telegram/SourceFiles/platform/linux/notifications_manager_linux.cpp

+		if (!subtitle.isEmpty()) {
+			_body = qstr("<b>") + escapeHtml(subtitle) + qstr("</b>") + '\n' + escapeHtml(msg);
+		} else {
+			_body = escapeHtml(msg);
+		}


This can be simplified.

_body = subtitle.isEmpty() ? escapeHtml(msg) : QString("<b>%1</b>\n%2") .arg(escapeHtml(subtitle)) .arg(escapeHtml(msg));

23rd · 2019-12-01T08:38:19Z

Telegram/SourceFiles/platform/linux/notifications_manager_linux.cpp

-		return _actionsSupported;
-	}
+bool NotificationData::show() {
+	QStringList actionsStringList(QList<QString>::fromVector(QVector<QString>::fromStdVector(_actions)));


Okay, this is insane.
Perhaps I was wrong and it will be better to store _actions as QStringList in this particular case.

23rd · 2019-12-02T20:15:26Z

Telegram/SourceFiles/platform/linux/notifications_manager_linux.cpp

+		if (_specificationVersion.majorVersion() == 1 && _specificationVersion.minorVersion() >= 2 || _specificationVersion.majorVersion() > 1) {
+			imageKey = "image-data";
+		} else if (_specificationVersion.majorVersion() == 1 && _specificationVersion.minorVersion() == 1) {
+			imageKey = "image_data";
+		} else if (_specificationVersion.majorVersion() == 1 && _specificationVersion.minorVersion() < 1 || _specificationVersion.majorVersion() < 1) {
+			imageKey = "icon_data";
+		} else {


These huge conditions can be easily shortened with a new variables.

const auto major = _specificationVersion.majorVersion(); const auto minor = _specificationVersion.minorVersion(); if (major == 1 && minor >= 2) { ...

It would be better to have a clearly if ((a && b) || c) condition instead of unclearly if (a && b || c).

23rd · 2019-12-02T20:20:37Z

Telegram/SourceFiles/platform/linux/notifications_manager_linux.cpp

+		for (int i = 0; i < serverInformationReply.arguments().size(); ++i) {
+			if (static_cast<QMetaType::Type>(serverInformationReply.arguments()[i].type()) == QMetaType::QString) {
+				serverInformation.push_back(serverInformationReply.arguments()[i].toString());


Can be simplified with the for ( a : b ) loop.
For example.

for (const auto &arg : serverInformationReply.arguments()) { if (static_cast<QMetaType::Type>(arg.type()) == QMetaType::QString) { serverInformation.push_back(arg.toString());

23rd · 2019-12-02T21:25:13Z

Telegram/SourceFiles/platform/linux/notifications_manager_linux.cpp

-	return text;
-}
+NotificationData::NotificationData(const std::shared_ptr<Manager*> &guarded, const QString &title, const QString &subtitle, const QString &msg, PeerId peerId, MsgId msgId)
+: _notificationInterface("org.freedesktop.Notifications", "/org/freedesktop/Notifications", "org.freedesktop.Notifications")


I would suggest you to move the repeated constant values to the separate constants.
Something like this.

namespace { const auto kService = QString("org.freedesktop.Notifications"); const auto kPath = QString("/org/freedesktop/Notifications"); } // namespace

Moving such consts into an anonymous namespace is welcome.
An example from the project code.

tdesktop/Telegram/SourceFiles/window/themes/window_theme.cpp

Lines 36 to 42 in 0c0c8f3

namespace {

constexpr auto kThemeFileSizeLimit = 5 * 1024 * 1024;

constexpr auto kBackgroundSizeLimit = 25 * 1024 * 1024;

constexpr auto kNightThemeFile = str_const(":/gui/night.tdesktop-theme");

constexpr auto kMinimumTiledSize = 512;

23rd · 2019-12-11T21:58:29Z

Telegram/SourceFiles/platform/linux/notifications_manager_linux.cpp

+	if (!capabilities.empty()) {
+		QString capabilitiesString;
+
+		for (const auto &capability : capabilities) {
+			if (!capabilitiesString.isEmpty()) {
+				capabilitiesString += qstr(", ");


This is a useless job for the Release build.

Maybe it is better to check if debug logs are enabled than this is a debug build? This should be very helpful if there will be issues with notifications.

This should be very helpful if there will be issues with notifications.

Well, if this is useful for problems with notifications, we will not get this information from the users, as no one uses the Debug version. I think for a one-time log entry this will be fine to use LOG().

23rd · 2019-12-21T22:52:54Z

Telegram/SourceFiles/platform/linux/notifications_manager_linux.cpp

-		Libs::g_list_free_full(capabilities, g_free);
+const QDBusArgument &operator>>(const QDBusArgument &argument, NotificationData::ImageData &imageData) {
+	argument.beginStructure();
+	argument >> imageData.width >> imageData.height >> imageData.rowStride >> imageData.hasAlpha >> imageData.bitsPerSample >> imageData.channels >> imageData.data;;


Suggested change

argument >> imageData.width >> imageData.height >> imageData.rowStride >> imageData.hasAlpha >> imageData.bitsPerSample >> imageData.channels >> imageData.data;;

argument >> imageData.width >> imageData.height >> imageData.rowStride >> imageData.hasAlpha >> imageData.bitsPerSample >> imageData.channels >> imageData.data;

23rd · 2019-12-21T22:57:53Z

Telegram/SourceFiles/platform/linux/notifications_manager_linux.cpp

-		bool hideNameAndPhoto = false;
-	};
+	if (capabilitiesReply.isValid()) {
+		return capabilitiesReply.value().toVector().toStdVector();


In my opinion, we can remove the blank elements from the vector right away.

Suggested change

return capabilitiesReply.value().toVector().toStdVector();

auto capabilities = capabilitiesReply.value();

capabilities.removeAll(QString());

return capabilities | ranges::to_vector;

Which blank elements? o_O

You have the following lines in your code.

tdesktop/Telegram/SourceFiles/platform/linux/notifications_manager_linux.cpp

Lines 43 to 45 in 3a0c597

if (!capabilitiesString.isEmpty()) {

capabilitiesString += qstr(", ");

}

So you are probably assuming that there may be blank elements?

No, it is a list of daemon capabilities for debug purposes.
It is also a port of

tdesktop/Telegram/SourceFiles/platform/linux/notifications_manager_linux.cpp

Line 371 in 0c0c8f3

LOG(("LibNotify capabilities: %1").arg(_capabilities.join(qstr(", "))));

Oops, I misread the condition, sorry.

23rd · 2019-12-21T23:01:27Z

Telegram/SourceFiles/platform/linux/notifications_manager_linux.cpp

+		QString capabilitiesString;
+
+		for (const auto &capability : capabilities) {
+			if (!capabilitiesString.isEmpty()) {
+				capabilitiesString += qstr(", ");
 			}

-			logError(error);
+			capabilitiesString += capability;
 		}


This loop can be replaced with std::accumulate.
For example.

const auto capabilitiesString = std::accumulate( capabilities.begin(), capabilities.end(), QString{}, [](auto &s, auto &p) { return s += (p + qstr(", ")); }).chopped(2);

23rd · 2019-12-21T23:24:12Z

By the way, all new project files follow the style with the column limit of 78.
It's quite difficult to work with long lines. =(
So, I think it would be great to split up them.
(Except for comments with links.)

23rd · 2019-12-21T23:41:02Z

Telegram/SourceFiles/platform/linux/notifications_manager_linux.cpp

+auto NotificationDaemonRunning = false;
+bool Supported() {
+	static auto Checked = false;
+	if (!Checked) {


What is the reason to keep the naked NotificationDaemonRunning variable here? You can move it to the Supported() like you did with the static auto Checked.

As far as I understand, the point here is that the check is performed only once - at startup.
static auto Checked was in Supported() already:

tdesktop/Telegram/SourceFiles/platform/linux/notifications_manager_linux.cpp

Line 267 in 0c0c8f3

static auto Checked = false;

Windows version has similar code:

tdesktop/Telegram/SourceFiles/platform/win/notifications_manager_win.cpp

Lines 306 to 312 in 0c713a9

bool Supported() {

if (!Checked) {

Checked = true;

Check();

}

return InitSucceeded;

}

Yes, and you can see that these variables are at least in the anonymous namespace.

tdesktop/Telegram/SourceFiles/platform/win/notifications_manager_win.cpp

Lines 296 to 313 in 0c713a9

auto Checked = false;

auto InitSucceeded = false;

void Check() {

InitSucceeded = init();

}

} // namespace

bool Supported() {

if (!Checked) {

Checked = true;

Check();

}

return InitSucceeded;

}

Okay, I'll put it in an anonymous namespace

23rd · 2019-12-21T23:49:31Z

Telegram/SourceFiles/platform/linux/notifications_manager_linux.cpp

+QVersionNumber NotificationData::parseSpecificationVersion(const std::vector<QString> &serverInformation) {
+	if (serverInformation.size() >= 4) {
+		return QVersionNumber::fromString(serverInformation[3]);
+	} else {
+		LOG(("Native notification error: server information should have 4 elements"));
+	}

-	using Notifications = QMap<PeerId, QMap<MsgId, Notification>>;
-	Notifications _notifications;
+	return QVersionNumber();
+}


Nitpick: There is no reason to keep this function as a class method and it can be safely moved to anonymous namespace.

namespace { auto ParseSpecificationVersion(const std::vector<QString> &serverInformation) { if (serverInformation.size() >= 4) { return QVersionNumber::fromString(serverInformation[3]); } else { LOG(("Native notification error: server information should have 4 elements")); } return QVersionNumber(); } } // namespace

23rd · 2019-12-22T00:02:33Z

Telegram/SourceFiles/platform/linux/notifications_manager_linux.cpp

+
+	if (!_specificationVersion.isNull()) {
+		const auto majorVersion = _specificationVersion.majorVersion();
+		const auto minorVersion = _specificationVersion.majorVersion();


Suggested change

const auto minorVersion = _specificationVersion.majorVersion();

const auto minorVersion = _specificationVersion.minorVersion();

23rd · 2019-12-22T01:04:39Z

Telegram/SourceFiles/platform/linux/notifications_manager_linux.h

@@ -23,16 +28,62 @@ inline bool SkipToast() {
 inline void FlashBounce() {
 }

-void Finish();
+class NotificationData : public QObject {
+	Q_OBJECT


I think we can safely remove Q_OBJECT to avoid an unnecessary extra MOC.

Suggested change

Q_OBJECT

No, we can't

tdesktop/Telegram/SourceFiles/platform/linux/notifications_manager_linux.cpp

Line 66 in 3a0c597

connect(&_notificationInterface, SIGNAL(ActionInvoked(uint, QString)), this, SLOT(notificationClicked(uint)));

tdesktop/Telegram/SourceFiles/platform/linux/notifications_manager_linux.cpp

Line 90 in 3a0c597

connect(&_notificationInterface, SIGNAL(NotificationClosed(uint, uint)), this, SLOT(notificationClosed(uint)));

Did you try?

Yes, I added Q_OBJECT only after a lot of tries without it

23rd · 2019-12-22T10:43:42Z

Hmm, do we really need to create the QDBusInterface in each new NotificationData? Can we create a single interface and use it for all future notifications? Can the provided information be changed during the tdesktop execution?

…ication implementation

ilya-fedin · 2019-12-24T12:37:42Z

By the way, all new project files follow the style with the column limit of 78.
It's quite difficult to work with long lines. =(
So, I think it would be great to split up them.
(Except for comments with links.)

fixed

Hmm, do we really need to create the QDBusInterface in each new NotificationData? Can we create a single interface and use it for all future notifications? Can the provided information be changed during the tdesktop execution?

I checked - it works without that. But it is still necessary to request capabilities and specification version every time, because user can replace notification daemon (or it can be replaced by system upgrade, because notification daemon doesn't works all the time and only starts upon notification request by dbus activation).

john-preston · 2019-12-29T15:41:52Z

Thanks a lot!

github-actions · 2021-03-04T04:02:39Z

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.

ilya-fedin force-pushed the qtdbus-notifications branch from 95ab1e3 to fef8e63 Compare November 29, 2019 03:04

23rd reviewed Nov 30, 2019

View reviewed changes

Telegram/SourceFiles/platform/linux/notifications_manager_linux.cpp Outdated Show resolved Hide resolved

ilya-fedin force-pushed the qtdbus-notifications branch from 4f2cbe4 to 63c4cc9 Compare December 1, 2019 00:28

23rd reviewed Dec 1, 2019

View reviewed changes

ilya-fedin force-pushed the qtdbus-notifications branch from 63c4cc9 to 5bca9c4 Compare December 2, 2019 19:35

23rd reviewed Dec 2, 2019

View reviewed changes

ilya-fedin force-pushed the qtdbus-notifications branch from 5bca9c4 to 3a0c597 Compare December 3, 2019 01:05

23rd reviewed Dec 11, 2019

View reviewed changes

23rd reviewed Dec 21, 2019

View reviewed changes

23rd reviewed Dec 22, 2019

View reviewed changes

Replace libnotify linux notification implementation with QtDBus notif…

db38f5a

…ication implementation

ilya-fedin force-pushed the qtdbus-notifications branch from 3a0c597 to db38f5a Compare December 24, 2019 12:28

ilya-fedin requested a review from 23rd December 24, 2019 12:53

Merge branch 'dev' into qtdbus-notifications

a210504

john-preston merged commit 3d36b4f into telegramdesktop:dev Dec 29, 2019

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

	namespace {

	constexpr auto kThemeFileSizeLimit = 5 * 1024 * 1024;
	constexpr auto kBackgroundSizeLimit = 25 * 1024 * 1024;
	constexpr auto kNightThemeFile = str_const(":/gui/night.tdesktop-theme");
	constexpr auto kMinimumTiledSize = 512;

	argument >> imageData.width >> imageData.height >> imageData.rowStride >> imageData.hasAlpha >> imageData.bitsPerSample >> imageData.channels >> imageData.data;;
	argument >> imageData.width >> imageData.height >> imageData.rowStride >> imageData.hasAlpha >> imageData.bitsPerSample >> imageData.channels >> imageData.data;

-		return capabilitiesReply.value().toVector().toStdVector();
+		auto capabilities = capabilitiesReply.value();
+		capabilities.removeAll(QString());
+		return capabilities | ranges::to_vector;

	if (!capabilitiesString.isEmpty()) {
	capabilitiesString += qstr(", ");
	}

	bool Supported() {
	if (!Checked) {
	Checked = true;
	Check();
	}
	return InitSucceeded;
	}


	auto Checked = false;
	auto InitSucceeded = false;

	void Check() {
	InitSucceeded = init();
	}

	} // namespace

	bool Supported() {
	if (!Checked) {
	Checked = true;
	Check();
	}
	return InitSucceeded;
	}

	const auto minorVersion = _specificationVersion.majorVersion();
	const auto minorVersion = _specificationVersion.minorVersion();

Native notifications on Linux without libnotify #6825

Native notifications on Linux without libnotify #6825

Conversation

ilya-fedin commented Nov 26, 2019 • edited

CLAassistant commented Nov 26, 2019 • edited

ilya-fedin commented Nov 29, 2019

Aokromes commented Nov 29, 2019

ilya-fedin commented Nov 30, 2019

ilya-fedin commented Nov 30, 2019

23rd commented Nov 30, 2019

ilya-fedin commented Dec 1, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ilya-fedin Dec 1, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ilya-fedin Dec 12, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

23rd commented Dec 21, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

23rd commented Dec 22, 2019

ilya-fedin commented Dec 24, 2019

john-preston commented Dec 29, 2019

github-actions bot commented Mar 4, 2021

ilya-fedin commented Nov 26, 2019 •

edited

CLAassistant commented Nov 26, 2019 •

edited

ilya-fedin commented Dec 1, 2019 •

edited

ilya-fedin Dec 1, 2019 •

edited

ilya-fedin Dec 12, 2019 •

edited

23rd commented Dec 21, 2019 •

edited