Merge lp:~hypodermia/ubuntu/oneiric/compiz/fix-for-bug-301174 into lp:ubuntu/oneiric/compiz

Proposed by Emily Strickland
Status: Work in progress
Proposed branch: lp:~hypodermia/ubuntu/oneiric/compiz/fix-for-bug-301174
Merge into: lp:ubuntu/oneiric/compiz
Diff against target: 204 lines (+183/-0)
4 files modified
plugins/bell/CMakeLists.txt (+5/-0)
plugins/bell/bell.xml.in (+21/-0)
plugins/bell/src/bell.cpp (+103/-0)
plugins/bell/src/bell.h (+54/-0)
To merge this branch: bzr merge lp:~hypodermia/ubuntu/oneiric/compiz/fix-for-bug-301174
Reviewer Review Type Date Requested Status
Sam Spilsbury (community) Approve
compiz packagers Pending
Ubuntu branches Pending
Review via email: mp+64632@code.launchpad.net

Description of the change

This branch creates a new plugin which uses libcanberra to play an audible bell when the system bell action event is triggered, along with a small typo fix incorporated to make sure that action actually happens. For the typo fix to compiz core, see this commit log entry: http://cgit.compiz.org/compiz/core/commit/?id=e6afcfd735db5a9f507fb9cd810bdb139ab8480f

The original source code can be found in my Github repository here, along with smspillaz' merged changes: https://github.com/hypodermia/compiz-bell-plugin

This should fix this bug: https://bugs.launchpad.net/ubuntu/+source/compiz/+bug/301174

As well as this bug: https://bugs.launchpad.net/ubuntu/+source/unity/+bug/769314

To test this, build and install this plugin, install fixed compiz, replace compiz, load the plugin, and then run this to trigger the bell: echo -n -e "\a"

Should cause a mild bell sound, provided the alert sounds in sound preferences are turned on and up and the sound file exists (configurable).

Adds a dependency on libcanberra. May require one further small change to make sure CMake finds and builds it, and may also add one dependency to ensure the default sound file is actually present on the system.

To post a comment you must log in.
Revision history for this message
Didier Roche-Tolomelli (didrocks) wrote :

Thanks for this patch and help to make ubuntu better.

The review should be done by smspillaz, Sam can you have a look please?

Revision history for this message
Sam Spilsbury (smspillaz) wrote :

Approve +1

We discussed having this a long time ago but never actually got around to implementing it. Thanks.

review: Approve
Revision history for this message
Sam Spilsbury (smspillaz) wrote :

Oh, one thing, could you retarget this against lp:compiz-core ?

Revision history for this message
Chris Halse Rogers (raof) wrote :

I'm not a compiz dev, so my input may be overruled by someone who is, but I think this should use a named event sound rather than manually specifying a filename - that way, (a) depending on sound-theme-freedesktop (which libcanberra does) guarantees that the sound file exists, and (b) allows the sound-theme to be configured in the “Sound” capplet, with the rest of the sounds.

http://0pointer.de/public/sound-naming-spec.html suggests “bell-window-system” (which should fallback to “bell” in the freedesktop theme) would be the right choice.

This looks like it should go upstream first - Sam, do you have any comments on this?

Revision history for this message
Chris Halse Rogers (raof) wrote :

Discussion with Sam in #ayatana:

16:07 <RAOF> It should use an event sound from http://0pointer.de/public/sound-naming-spec.html rather than manually specifying a filename. That way, sound themes will actually work.
16:07 <RAOF> Also, it becomes simpler. Bonus!
16:07 <RAOF> (I'd suggest bell-window-system)
16:10 <RAOF> Ah. Alternatively, that spec could be a pack of lies, and there not actually _be_ a bell-window-system. Superlative. There is, however, a bell. So the not-manually-specifying-a-filename thing stands.
16:17 <RAOF> Ah, again no. bell-window-system should alledgedly fall back to just “bell”
16:18 <RAOF> smspillaz: Anyway, the summary is: don't specify a filename, so that it respects the system sound theme :)
16:19 <smspillaz> RAOF: ah, ok
16:19 <RAOF> Note: this advice may need actual, you know, _testing_
16:19 <smspillaz> that doesn't exist in compizland
16:19 <RAOF> But if everything works as advertised, that's what should happen!
16:21 <smspillaz> RAOF: so we need to specify bell-window-system
16:21 <smspillaz> and instead of CA_PROP_MEDIA_FILENAME ...
16:21 <RAOF> Right.
16:22 <RAOF> And instead of CA_PROP_MEDIA_FILENAME, you *don't* specify CA_PROP_MEDIA_FILENAME, and libcanberra looks it up in the sound theme.
16:22 <smspillaz> I hope canberra uses gtkdoc ...
16:22 <RAOF> Why, yes it does.
16:22 <RAOF> http://0pointer.de/lennart/projects/libcanberra/gtkdoc/libcanberra-canberra.html#CA-PROP-EVENT-ID:CAPS
16:22 <smspillaz> oh I see
16:23 <smspillaz> I think that the filename is actually inteded to be an override
16:23 <smspillaz> so I might just check for filename size
16:24 <RAOF> Is there a desperate need for compiz's sounds to be configured differently to the sound theme?
16:25 <RAOF> Hm. Actually, the gtkdoc doesn't make it clear whether or CA_PROP_EVENT_ID *overrides* CA_PROP_MEDIA_FILENAME…
16:26 <RAOF> I guess empirical testing may be in order. Worse than a superfluous configuration option is a superfluous configuration option that *doesn't even work* :)
16:28 <RAOF> smspillaz: So, it's possible my concern is actually ‘the option to set the bell sound filename doesn't work’ :)
16:30 <RAOF> Either it works, and compiz won't follow the sound theme, or it doesn't, and compiz gains a non-functional option. Score!
16:32 <smspillaz> RAOF: we just don't se CA_PROP_MEDIA_FILENAME if mFilename is empty
16:33 <RAOF> smspillaz: And you may need to *not* set CA_PROP_EVENT_ID if mFilename is not empty. http://0pointer.de/lennart/projects/libcanberra/gtkdoc/libcanberra-canberra.html#ca-context-play suggests that it'll override MEDIA_FILENAME.
16:34 <RAOF> But relying on this crazy documentation thing is probably a bit ambitious. There should be actual testing to confirm that libcanberra works that way.
16:35 <RAOF> And I suspect that this testing could be asked of our intrepid branch submitterc.
16:36 <RAOF> Shall I paste this IRC snippet in as a review comment?
16:37 <smspillaz> sure
16:37 <smspillaz> I'll probably just fix it myself though

So - feel free to either resolve my concerns there, or wait for Sam to fix it!

Revision history for this message
Emily Strickland (hypodermia) wrote :

Finally got a chance to look at this.

The documentation was ambiguous when I originally made the plugin, so as
suggested here, I decided simply to test out all the options. I can confirm
that if you set CA_PROP_EVENT_ID as "bell" and don't specify a file name
that it uses the correct bell sound. Furthermore, 'bell-window-system'
actually doesn't work. Since 'bell' itself does, there's no need to specify
the file name, as far as I can tell (it seemed to be overriding the
CA_PROP_EVENT_ID setting).

I'm making the change and pushing it with the filename business entirely
removed.

On Mon, Aug 1, 2011 at 23:44, Chris Halse Rogers <email address hidden> wrote:

> The proposal to merge
> lp:~hypodermia/ubuntu/oneiric/compiz/fix-for-bug-301174 into
> lp:ubuntu/compiz has been updated.
>
> Status: Needs review => Work in progress
>
> For more details, see:
>
> https://code.launchpad.net/~hypodermia/ubuntu/oneiric/compiz/fix-for-bug-301174/+merge/64632
> --
>
> https://code.launchpad.net/~hypodermia/ubuntu/oneiric/compiz/fix-for-bug-301174/+merge/64632
> You are the owner of
> lp:~hypodermia/ubuntu/oneiric/compiz/fix-for-bug-301174.
>

--
Emily Strickland
<email address hidden>
706-363-0950

233. By Emily Strickland

Removed option to specify the file name. Should follow user's sound scheme.

Revision history for this message
Emily Strickland (hypodermia) wrote :

Actually, this appears to break on my system. I must've had the old bell file cached for that event. I don't see how to make this work as it's documented to supposed to work. We may need to roll back this latest change and, as RAOF said, go with the option where "it works, and compiz won't follow the sound theme".

Libcanberra bug?

Unmerged revisions

233. By Emily Strickland

Removed option to specify the file name. Should follow user's sound scheme.

232. By Emily Strickland

Add 'bell' plugin and a small fix to bcop.xslt to properly trigger bell events (see http://cgit.compiz.org/compiz/core/commit/?id=e6afcfd735db5a9f507fb9cd810bdb139ab8480f); fixes https://bugs.launchpad.net/ubuntu/+source/compiz/+bug/301174 and https://bugs.launchpad.net/ubuntu/+source/unity/+bug/769314

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
=== added directory 'plugins/bell'
=== added file 'plugins/bell/CMakeLists.txt'
--- plugins/bell/CMakeLists.txt 1970-01-01 00:00:00 +0000
+++ plugins/bell/CMakeLists.txt 2011-08-07 06:46:02 +0000
@@ -0,0 +1,5 @@
1find_package (Compiz REQUIRED)
2
3include (CompizPlugin)
4
5compiz_plugin(bell PKGDEPS libcanberra)
06
=== added file 'plugins/bell/bell.xml.in'
--- plugins/bell/bell.xml.in 1970-01-01 00:00:00 +0000
+++ plugins/bell/bell.xml.in 2011-08-07 06:46:02 +0000
@@ -0,0 +1,21 @@
1<compiz>
2 <plugin name="bell" useBcop="true">
3 <_short>Audible Bell</_short>
4 <_long>Plays an audible sound for the system bell</_long>
5 <category>Utility</category>
6 <deps>
7 <relation type="after">
8 <plugin>composite</plugin>
9 <plugin>opengl</plugin>
10 <plugin>decor</plugin>
11 </relation>
12 </deps>
13 <options>
14 <option name="bell" type="bell">
15 <_short>Audible Bell</_short>
16 <_long>Play sound on system bell</_long>
17 <default>true</default>
18 </option>
19 </options>
20 </plugin>
21</compiz>
022
=== added directory 'plugins/bell/src'
=== added file 'plugins/bell/src/bell.cpp'
--- plugins/bell/src/bell.cpp 1970-01-01 00:00:00 +0000
+++ plugins/bell/src/bell.cpp 2011-08-07 06:46:02 +0000
@@ -0,0 +1,103 @@
1/**
2 *
3 * Compiz bell plugin
4 *
5 * bell.c
6 *
7 * Copyright (c) 2011 Emily Strickland <emily@zubon.org>
8 *
9 * Authors:
10 * Emily Strickland <emily@zubon.org>
11 *
12 * This program is free software; you can redistribute it and/or
13 * modify it under the terms of the GNU General Public License
14 * as published by the Free Software Foundation; either version 2
15 * of the License, or (at your option) any later version.
16 *
17 * This program is distributed in the hope that it will be useful,
18 * but WITHOUT ANY WARRANTY; without even the implied warranty of
19 * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
20 * GNU General Public License for more details.
21 *
22 **/
23
24#include "bell.h"
25
26COMPIZ_PLUGIN_20090315 (bell, BellPluginVTable)
27
28bool
29AudibleBell::bell ()
30{
31 int error;
32
33 if ((error = ca_context_play (mCanberraContext, 0,
34 CA_PROP_EVENT_ID, "bell",
35 CA_PROP_CANBERRA_CACHE_CONTROL, "permanent",
36 NULL)) < 0);
37 {
38 compLogMessage ("bell", CompLogLevelWarn, "couldn't play bell - %s",
39 ca_strerror (error));
40 }
41
42 /* Allow other plugins to handle bell event */
43 return false;
44}
45
46AudibleBell::AudibleBell (CompScreen *screen) :
47 PluginClassHandler <AudibleBell, CompScreen> (screen),
48 mCanberraContext (NULL)
49{
50 int error;
51 boost::function <bool (CompAction *, CompAction::State, CompOption::Vector &)> bellCallback;
52
53 if ((error = ca_context_create (&mCanberraContext)) < 0)
54 {
55 compLogMessage ("bell", CompLogLevelWarn, "couldn't initialize canberra - %s",
56 ca_strerror (error));
57 setFailed ();
58 }
59 else
60 {
61 if ((error = ca_context_change_props (mCanberraContext,
62 CA_PROP_APPLICATION_NAME,
63 "Compiz bell plugin",
64 CA_PROP_APPLICATION_ID,
65 "org.freedesktop.compiz.Bell",
66 CA_PROP_WINDOW_X11_SCREEN,
67 screen->displayString (),
68 NULL)) < 0)
69 {
70 compLogMessage ("bell", CompLogLevelWarn, "couldn't register bell handler - %s",
71 ca_strerror (error));
72 setFailed ();
73 }
74 else
75 {
76 if ((error = ca_context_open (mCanberraContext)) < 0)
77 {
78 compLogMessage ("bell", CompLogLevelWarn, "couldn't open canberra context - %s",
79 ca_strerror (error));
80 setFailed ();
81 }
82 }
83 }
84
85 bellCallback =
86 boost::bind (&AudibleBell::bell, this);
87
88 optionSetBellInitiate (bellCallback);
89}
90
91AudibleBell::~AudibleBell ()
92{
93 ca_context_destroy (mCanberraContext);
94}
95
96bool
97BellPluginVTable::init ()
98{
99 if (!CompPlugin::checkPluginABI ("core", CORE_ABIVERSION))
100 return false;
101
102 return true;
103}
0104
=== added file 'plugins/bell/src/bell.h'
--- plugins/bell/src/bell.h 1970-01-01 00:00:00 +0000
+++ plugins/bell/src/bell.h 2011-08-07 06:46:02 +0000
@@ -0,0 +1,54 @@
1/**
2 *
3 * Compiz bell plugin
4 *
5 * bell.c
6 *
7 * Copyright (c) 2011 Emily Strickland <emily@zubon.org>
8 *
9 * Authors:
10 * Emily Strickland <emily@zubon.org>
11 *
12 * This program is free software; you can redistribute it and/or
13 * modify it under the terms of the GNU General Public License
14 * as published by the Free Software Foundation; either version 2
15 * of the License, or (at your option) any later version.
16 *
17 * This program is distributed in the hope that it will be useful,
18 * but WITHOUT ANY WARRANTY; without even the implied warranty of
19 * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
20 * GNU General Public License for more details.
21 *
22 **/
23
24#include <core/core.h>
25#include <core/pluginclasshandler.h>
26
27#include <canberra.h>
28
29#include "bell_options.h"
30
31class AudibleBell :
32 public PluginClassHandler<AudibleBell, CompScreen>,
33 public ScreenInterface,
34 public BellOptions
35{
36 public:
37
38 AudibleBell (CompScreen *screen);
39 ~AudibleBell ();
40
41 bool bell ();
42
43 private:
44
45 ca_context *mCanberraContext;
46};
47
48class BellPluginVTable :
49 public CompPlugin::VTableForScreen<AudibleBell>
50{
51 public:
52 bool init ();
53};
54

Subscribers

People subscribed via source and target branches