Linux: add reply field support #226

ghost · 2025-05-07T20:18:27Z

This adds support for the reply field on Linux, using the inline-reply capability. It might not exist everywhere but seems to be standard as it doesn't have a x-vendor prefix.

Support for the button_text field could be achieved, but it doesn't seem to be standardized (yet?), so unsure if this is in the scope of this lib.

This adds support for the reply field on Linux, using the `inline-reply` capability. It might not exist everywhere but seems to be standard as it doesn't have a `x-vendor` prefix.

codecov-commenter · 2025-05-07T20:19:33Z

Codecov Report

Attention: Patch coverage is 43.75000% with 9 lines in your changes missing coverage. Please review.

Project coverage is 85.23%. Comparing base (5dab588) to head (e8fa20d).
Report is 29 commits behind head on main.

Files with missing lines	Patch %	Lines
src/desktop_notifier/backends/dbus.py	43.75%	9 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #226      +/-   ##
==========================================
+ Coverage   85.06%   85.23%   +0.17%     
==========================================
  Files          10       10              
  Lines         951      962      +11     
==========================================
+ Hits          809      820      +11     
  Misses        142      142

Flag	Coverage Δ
pytest	`85.23% <43.75%> (+0.17%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

samschott

Thanks for the contribution! It looks like inline replies are not part of the spec at https://specifications.freedesktop.org/notification-spec/latest/protocol.html#command-get-capabilities, even though the capability is not prefixed with "x-vendor". Do you know on which desktop environments you expect this to be supported?

Two requests:

This is currently not tested because. The notification daemon used in our tests does not seem to support reply fields (see setup here) but you can probably still "simulate" a reply here and remove the capability check here from the test case.
Could you add a change log entry?

src/desktop_notifier/backends/dbus.py

ghost · 2025-05-13T16:05:06Z

1. [...] you can probably still "simulate" a reply here and remove the capability check here from the test case.

2. Could you add a change log entry?

Yes, I will do both of those things as well. For the change log entry, shall I bump the version?

samschott · 2025-05-13T16:20:56Z

For the change log entry, shall I bump the version?

Yes! 6.2.0 would be appropriate since its a "new" feature for the dbus backend.

ghost · 2025-05-13T16:30:10Z

It looks like inline replies are not part of the spec [...] Do you know on which desktop environments you expect this to be supported?

Forgot to add that part in my other reply, oops. After looking a bit more, it seems like inline-reply was stalled, and that would explain why it's not part of the spec (despite its name indicating otherwise), nor mentioned in KDE's own documentation (despite them supporting it).

On the other hand useful additions to org.freedesktop notifications ( !24 (closed)) got stalled because of vague portal things.

If you still wish to merge that PR, I'd be happy to still bring the requested changed.

samschott · 2025-05-14T18:19:29Z

Given that KDE is a sufficiently popular desktop environment, I'd be happy to proceed with this PR.

samschott · 2025-05-14T18:26:18Z

Support for the button_text field could be achieved, but it doesn't seem to be standardized (yet?), so unsure if this is in the scope of this lib.

Since "inline-reply" is non-standard either, I'd be happy also include button_text support for KDE:

https://github.com/KDE/knotifications/blob/aa0083308a6e547c40f138df53109a04fb1f83b6/src/notifybypopup.cpp#L243

The approach for this lib is to support features where possible and expose notification server capabilities through the API. An app should generally not rely on notifications for key interaction patterns since users may disable them or limit their capabilities on a per-app basis, so a "your milage will vary" approach sounds good to me.

Added check to ignore reply field if the notification server doesn't support `inline-reply`, as well as adding support for `x-kde-reply-submit-button-text`

samschott

Thanks a lot! Only a few minor requests for changes.

samschott · 2025-05-16T19:13:53Z

src/desktop_notifier/backends/dbus.py

-        hints_v: dict[str, Variant] = dict()
+        if notification.reply_field and self.supports_inline_reply:
+            actions += [INLINE_REPLY_ACTION_KEY, notification.reply_field.title]
+            hints_v["x-kde-reply-submit-button-text"] = Variant(


nit: Could you move "x-kde-reply-submit-button-text" also to a constant?

samschott · 2025-05-16T19:16:59Z

src/desktop_notifier/backends/dbus.py

            actions += [button.identifier, button.title]

-        hints_v: dict[str, Variant] = dict()
+        if notification.reply_field and self.supports_inline_reply:


Suggested change

if notification.reply_field and self.supports_inline_reply:

if notification.reply_field and Capability.REPLY_FIELD in await self.get_capabilities():

samschott · 2025-05-16T19:19:55Z

CHANGES.md

@@ -1,3 +1,10 @@
+# v6.2.0
+
+## Added :


Suggested change

## Added :

## Added:

samschott · 2025-05-16T19:26:20Z

tests/test_callbacks.py


 @pytest.mark.asyncio
 async def test_replied_fallback_handler_called(notifier: DesktopNotifier) -> None:
-    await check_supported(notifier, Capability.REPLY_FIELD)


Darn, it looks like this check is also required for older MacOS versions which now have failing tests.

For now, I'd recommend adding this back again, and accepting that we have a bit less coverage on Linux. I can look later into setting up a notification daemon with inline-reply support on GitHub Actions.

Linux: add reply field support

2349fe0

This adds support for the reply field on Linux, using the `inline-reply` capability. It might not exist everywhere but seems to be standard as it doesn't have a `x-vendor` prefix.

samschott requested changes May 13, 2025

View reviewed changes

src/desktop_notifier/backends/dbus.py Show resolved Hide resolved

val_int1 added 3 commits May 14, 2025 20:54

Update dbus.py

83cbb53

Added check to ignore reply field if the notification server doesn't support `inline-reply`, as well as adding support for `x-kde-reply-submit-button-text`

Added notification reply test for dbus

1ef1c4c

Updated changelog

5845c2f

samschott requested changes May 16, 2025

View reviewed changes

Implemented requested changes

e8fa20d

samschott approved these changes May 17, 2025

View reviewed changes

samschott merged commit 2f0016c into samschott:main May 17, 2025
30 checks passed

samschott mentioned this pull request Aug 8, 2025

Capability.REPLY_FIELD absent on KDE Plasma-6.4.3 #230

Open

	if notification.reply_field and self.supports_inline_reply:
	if notification.reply_field and Capability.REPLY_FIELD in await self.get_capabilities():

Linux: add reply field support #226

Linux: add reply field support #226

Uh oh!

Conversation

ghost commented May 7, 2025

Uh oh!

codecov-commenter commented May 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

samschott left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ghost commented May 13, 2025 • edited by ghost Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

samschott commented May 13, 2025

Uh oh!

ghost commented May 13, 2025

Uh oh!

samschott commented May 14, 2025

Uh oh!

samschott commented May 14, 2025

Uh oh!

samschott left a comment

Choose a reason for hiding this comment

Uh oh!

samschott May 16, 2025

Choose a reason for hiding this comment

Uh oh!

samschott May 16, 2025

Choose a reason for hiding this comment

Uh oh!

samschott May 16, 2025

Choose a reason for hiding this comment

Uh oh!

samschott May 16, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

codecov-commenter commented May 7, 2025 •

edited

Loading

ghost commented May 13, 2025 •

edited by ghost

Loading