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

Embedded daemon in one unified binary #702

Merged
merged 11 commits into from
Sep 13, 2021
Merged

Embedded daemon in one unified binary #702

merged 11 commits into from
Sep 13, 2021

Conversation

marbetschar
Copy link
Member

@marbetschar marbetschar commented Sep 8, 2021

This PR fixes the notification icon issue from #677 by embedding the daemonized background process into the same application binary (in the screenshot: "Other" is before this change, the correct notification is after this change):

Screenshot from 2021-09-08 08-52-28

This way, we gain the following benefits:

1. The notification icon is correct

The notification icon is read from the Icon key stored in /usr/share/applications/{application_id}.desktop. Since the background process shares the same application_id as the main application, the notification server uses the correct icon to display the alert.

2. The background process starts automatically

Autostart files are read from /etc/xdg/autostart/{application_id}.desktop. We can easily place a second desktop file with the EXEC line io.elementary.calendar --background in this directory.

3. When clicking on the notification, the corresponding app opens

When clicking on the notification, the notification server executes the Exec key of the /usr/share/applications/{application_id}.desktop file. Here it is crucial we embedded the background process within the same binary - because this way, the regular binary starts and the user is presented with the app itself. Just like we want it.

4. System Settings > Notifications work as expected

As it turned out when working on this in Mail, if we don't embed the background process within the same binary we either end up with two application entries there - or even worse, with one which does not have any effect at all.

Copy link
Collaborator

@mcclurgm mcclurgm left a comment

Choose a reason for hiding this comment

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

I have some very small changes I noted. There's a bigger issue here that I noted for on_events_updated, but I'll come back with a second review when I can figure out what's going on there.

src/TodayEventMonitor.vala Outdated Show resolved Hide resolved
src/TodayEventMonitor.vala Outdated Show resolved Hide resolved
src/TodayEventMonitor.vala Outdated Show resolved Hide resolved
src/TodayEventMonitor.vala Show resolved Hide resolved
@mcclurgm
Copy link
Collaborator

mcclurgm commented Sep 12, 2021

Never mind my comment about on_events_updated, that's out of scope for this.

Seems to work as expected. As I said in my last comment there are a few minor style things, but I think this pretty much works! I'll approve it once we get those sorted.

@marbetschar
Copy link
Member Author

@mcclurgm thanks for your review! I addressed the proposed changes and I think we should be good to go here now.

@mcclurgm
Copy link
Collaborator

Alright, seems good to me.

@marbetschar marbetschar merged commit 558716e into master Sep 13, 2021
@marbetschar marbetschar deleted the single-binary branch September 13, 2021 14:22
@quienn
Copy link

quienn commented Sep 25, 2021

Latest Calendar version comes with the autostart entry for the daemon (at /etc/xdg/autostart/io.elementary.calendar-daemon.desktop) though it never really starts the daemon. 😕 Was this intended?

@marbetschar
Copy link
Member Author

@ghsttwn no, this was not the intention :)

The daemon should be autostarted as soon as you end your session and start a new one (read "logout and login again"). Doesn't this work on your end? If not, can you please open a new issue with a link to this PR, so we can investigate there?

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.

3 participants