Code review comment for lp:~ted/indicator-sound/synchronous-notification

Revision history for this message
Charles Kerr (charlesk) wrote :

Short & sweet, I like it.

A couple of changes that would be nice-to-have:

1. The new set_volume() notification code overlaps a lot with IndicatorSound.Service's existing activate_scroll_action() method, but with some differences, e.g. icon naming of "notification-audio-volume-off" vs "audio-volume-muted". Should these blocks be using the same icon names? If so, should the similar code be extracted into its own method where it can be shared?

2. Trivial change, "/usr/share/sounds/ubuntu/stereo/message.ogg" should be a symbolic somewhere, rather than hardcoded in the middle of the source file. Yawn.

« Back to merge proposal