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

notification: Implement version2 of the interface #147

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

jsparber
Copy link
Contributor

@jsparber jsparber commented Mar 8, 2024

Implement the new portal frontend in libportal flatpak/xdg-desktop-portal#1298

@jsparber jsparber force-pushed the implement_v2_notification_spec branch 2 times, most recently from bbe8883 to 57d6202 Compare April 11, 2024 14:38
@jsparber jsparber changed the title Implement v2 notification spec notification v2 specs: Allow passing GFileIcon, sound and add additional properties Apr 11, 2024
@jsparber jsparber marked this pull request as ready for review April 16, 2024 13:31
libportal/notification.c Outdated Show resolved Hide resolved
libportal/notification.c Outdated Show resolved Hide resolved
libportal/notification.c Outdated Show resolved Hide resolved
libportal/notification.c Outdated Show resolved Hide resolved
libportal/notification.c Outdated Show resolved Hide resolved
@jsparber jsparber force-pushed the implement_v2_notification_spec branch 3 times, most recently from 050fe36 to 03dd58c Compare April 23, 2024 14:41
libportal/notification.c Outdated Show resolved Hide resolved
@jsparber jsparber force-pushed the implement_v2_notification_spec branch 2 times, most recently from 595a948 to ff872a1 Compare April 23, 2024 15:48
@swick
Copy link

swick commented Apr 23, 2024

Might want to put the autofd stuff into a new commit but otherwise this LGTM.

@jsparber jsparber force-pushed the implement_v2_notification_spec branch from ff872a1 to 7c39eb4 Compare April 25, 2024 11:10
@jsparber jsparber force-pushed the implement_v2_notification_spec branch 2 times, most recently from f919856 to 7a202b4 Compare July 3, 2024 16:46
libportal/notification.c Outdated Show resolved Hide resolved
@jsparber jsparber force-pushed the implement_v2_notification_spec branch 3 times, most recently from e0fa651 to 5a84acf Compare July 15, 2024 08:38
@swick
Copy link

swick commented Oct 18, 2024

xdg-desktop-portal now depends on this PR

libportal/notification.c Outdated Show resolved Hide resolved
libportal/portal-private.h Outdated Show resolved Hide resolved
libportal/notification.c Outdated Show resolved Hide resolved
libportal/notification.c Show resolved Hide resolved
libportal/notification.c Outdated Show resolved Hide resolved
@jsparber jsparber force-pushed the implement_v2_notification_spec branch from 5a84acf to 14c0ec6 Compare October 25, 2024 16:02
@jsparber jsparber changed the title notification v2 specs: Allow passing GFileIcon, sound and add additional properties notification: Implement version2 of the interface Oct 25, 2024
@jsparber
Copy link
Contributor Author

I reworked this MR to keep backwards compatibility. I didn't fully test it yet, therefore i'm marking it as a draft.

@jsparber jsparber marked this pull request as draft October 25, 2024 16:07
@jsparber jsparber force-pushed the implement_v2_notification_spec branch from 14c0ec6 to 938e92f Compare October 28, 2024 12:30
@jsparber jsparber force-pushed the implement_v2_notification_spec branch 3 times, most recently from e3dd306 to 4c6b996 Compare October 29, 2024 15:29
@jsparber jsparber marked this pull request as ready for review October 29, 2024 15:30
@jsparber
Copy link
Contributor Author

Should be ready for review. I changed the MR to keep backwards compatibility and some tests to make sure it actually works.

@jsparber jsparber force-pushed the implement_v2_notification_spec branch 4 times, most recently from fe0465b to ae9ac48 Compare October 29, 2024 16:45
libportal/notification.c Outdated Show resolved Hide resolved
libportal/notification.c Outdated Show resolved Hide resolved
libportal/notification.c Outdated Show resolved Hide resolved
libportal/notification.c Outdated Show resolved Hide resolved
}

static void
parse_notification (GTask *task)
Copy link

Choose a reason for hiding this comment

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

It's a bit unexpected that a function named parse_notification does a dbus call. Maybe split the actual "parsing" out of here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i can rename the function to handle_notification()

libportal/notification.c Outdated Show resolved Hide resolved
libportal/notification.c Outdated Show resolved Hide resolved
libportal/notification.c Outdated Show resolved Hide resolved
libportal/notification.c Outdated Show resolved Hide resolved
libportal/notification.c Outdated Show resolved Hide resolved
@jsparber jsparber force-pushed the implement_v2_notification_spec branch from ae9ac48 to 6e5941f Compare October 30, 2024 11:27
Calls to the notification interface version 1 fail if the vardict
of `AddNotification` contains properties that aren't supported therefore
check the version and filter the vardict. This will be even more relevant
once features for version 2 are added in a future commit. The parse
function is made async because it's needed in a future commit.
@jsparber jsparber force-pushed the implement_v2_notification_spec branch from 6e5941f to d3c9817 Compare November 5, 2024 10:42
This introduces all new properties introduced in version 2 of the
notification interface. It still keeps compatibility with older versions
of the interface by adjusting and stripping properties that
were not supported in version 1.
@jsparber jsparber force-pushed the implement_v2_notification_spec branch from d3c9817 to 70365d8 Compare November 5, 2024 10:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants