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

Daemon: Use GLib.Notification #337

Closed
wants to merge 21 commits into from
Closed

Daemon: Use GLib.Notification #337

wants to merge 21 commits into from

Conversation

danirabbit
Copy link
Member

Fixes #17

@cassidyjames cassidyjames requested a review from a team December 13, 2018 19:34
donadigo
donadigo previously approved these changes Dec 13, 2018
Copy link

@donadigo donadigo left a comment

Choose a reason for hiding this comment

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

Code looks good.

Copy link
Collaborator

@jeremypw jeremypw left a comment

Choose a reason for hiding this comment

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

I think you need to refactor Daemon.vala as a GLib.Application. Also, for me, the daemon does not work properly unless it is installed in /usr/bin rather than /usr/libexec (not sure why yet)

warning ("Could not show notification: %s", err.message);
}
#endif
GLib.Application.get_default ().send_notification (uid, notification);
Copy link
Collaborator

Choose a reason for hiding this comment

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

This does not work because Daemon.vala is not a GLib.Application (at the moment).

@jeremypw
Copy link
Collaborator

Ignore comment about libexec. Binaries here are not expected to be in the user's path (so I had trouble launching it from the terminal for testing). The daemon should be installed here.

@danirabbit danirabbit closed this Jan 9, 2019
@danirabbit danirabbit deleted the glib-notification branch January 9, 2019 20:40
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.

None yet

5 participants