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

Proposed by Emily Strickland on 2011-06-15
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) 2011-07-19 Approve on 2011-08-02
compiz packagers 2011-08-02 Pending
Ubuntu branches 2011-06-15 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.
Didier Roche (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?

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
Sam Spilsbury (smspillaz) wrote :

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

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?

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!

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 on 2011-08-07

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

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 on 2011-08-07

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

232. By Emily Strickland on 2011-06-15

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
1=== added directory 'plugins/bell'
2=== added file 'plugins/bell/CMakeLists.txt'
3--- plugins/bell/CMakeLists.txt 1970-01-01 00:00:00 +0000
4+++ plugins/bell/CMakeLists.txt 2011-08-07 06:46:02 +0000
5@@ -0,0 +1,5 @@
6+find_package (Compiz REQUIRED)
7+
8+include (CompizPlugin)
9+
10+compiz_plugin(bell PKGDEPS libcanberra)
11
12=== added file 'plugins/bell/bell.xml.in'
13--- plugins/bell/bell.xml.in 1970-01-01 00:00:00 +0000
14+++ plugins/bell/bell.xml.in 2011-08-07 06:46:02 +0000
15@@ -0,0 +1,21 @@
16+<compiz>
17+ <plugin name="bell" useBcop="true">
18+ <_short>Audible Bell</_short>
19+ <_long>Plays an audible sound for the system bell</_long>
20+ <category>Utility</category>
21+ <deps>
22+ <relation type="after">
23+ <plugin>composite</plugin>
24+ <plugin>opengl</plugin>
25+ <plugin>decor</plugin>
26+ </relation>
27+ </deps>
28+ <options>
29+ <option name="bell" type="bell">
30+ <_short>Audible Bell</_short>
31+ <_long>Play sound on system bell</_long>
32+ <default>true</default>
33+ </option>
34+ </options>
35+ </plugin>
36+</compiz>
37
38=== added directory 'plugins/bell/src'
39=== added file 'plugins/bell/src/bell.cpp'
40--- plugins/bell/src/bell.cpp 1970-01-01 00:00:00 +0000
41+++ plugins/bell/src/bell.cpp 2011-08-07 06:46:02 +0000
42@@ -0,0 +1,103 @@
43+/**
44+ *
45+ * Compiz bell plugin
46+ *
47+ * bell.c
48+ *
49+ * Copyright (c) 2011 Emily Strickland <emily@zubon.org>
50+ *
51+ * Authors:
52+ * Emily Strickland <emily@zubon.org>
53+ *
54+ * This program is free software; you can redistribute it and/or
55+ * modify it under the terms of the GNU General Public License
56+ * as published by the Free Software Foundation; either version 2
57+ * of the License, or (at your option) any later version.
58+ *
59+ * This program is distributed in the hope that it will be useful,
60+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
61+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
62+ * GNU General Public License for more details.
63+ *
64+ **/
65+
66+#include "bell.h"
67+
68+COMPIZ_PLUGIN_20090315 (bell, BellPluginVTable)
69+
70+bool
71+AudibleBell::bell ()
72+{
73+ int error;
74+
75+ if ((error = ca_context_play (mCanberraContext, 0,
76+ CA_PROP_EVENT_ID, "bell",
77+ CA_PROP_CANBERRA_CACHE_CONTROL, "permanent",
78+ NULL)) < 0);
79+ {
80+ compLogMessage ("bell", CompLogLevelWarn, "couldn't play bell - %s",
81+ ca_strerror (error));
82+ }
83+
84+ /* Allow other plugins to handle bell event */
85+ return false;
86+}
87+
88+AudibleBell::AudibleBell (CompScreen *screen) :
89+ PluginClassHandler <AudibleBell, CompScreen> (screen),
90+ mCanberraContext (NULL)
91+{
92+ int error;
93+ boost::function <bool (CompAction *, CompAction::State, CompOption::Vector &)> bellCallback;
94+
95+ if ((error = ca_context_create (&mCanberraContext)) < 0)
96+ {
97+ compLogMessage ("bell", CompLogLevelWarn, "couldn't initialize canberra - %s",
98+ ca_strerror (error));
99+ setFailed ();
100+ }
101+ else
102+ {
103+ if ((error = ca_context_change_props (mCanberraContext,
104+ CA_PROP_APPLICATION_NAME,
105+ "Compiz bell plugin",
106+ CA_PROP_APPLICATION_ID,
107+ "org.freedesktop.compiz.Bell",
108+ CA_PROP_WINDOW_X11_SCREEN,
109+ screen->displayString (),
110+ NULL)) < 0)
111+ {
112+ compLogMessage ("bell", CompLogLevelWarn, "couldn't register bell handler - %s",
113+ ca_strerror (error));
114+ setFailed ();
115+ }
116+ else
117+ {
118+ if ((error = ca_context_open (mCanberraContext)) < 0)
119+ {
120+ compLogMessage ("bell", CompLogLevelWarn, "couldn't open canberra context - %s",
121+ ca_strerror (error));
122+ setFailed ();
123+ }
124+ }
125+ }
126+
127+ bellCallback =
128+ boost::bind (&AudibleBell::bell, this);
129+
130+ optionSetBellInitiate (bellCallback);
131+}
132+
133+AudibleBell::~AudibleBell ()
134+{
135+ ca_context_destroy (mCanberraContext);
136+}
137+
138+bool
139+BellPluginVTable::init ()
140+{
141+ if (!CompPlugin::checkPluginABI ("core", CORE_ABIVERSION))
142+ return false;
143+
144+ return true;
145+}
146
147=== added file 'plugins/bell/src/bell.h'
148--- plugins/bell/src/bell.h 1970-01-01 00:00:00 +0000
149+++ plugins/bell/src/bell.h 2011-08-07 06:46:02 +0000
150@@ -0,0 +1,54 @@
151+/**
152+ *
153+ * Compiz bell plugin
154+ *
155+ * bell.c
156+ *
157+ * Copyright (c) 2011 Emily Strickland <emily@zubon.org>
158+ *
159+ * Authors:
160+ * Emily Strickland <emily@zubon.org>
161+ *
162+ * This program is free software; you can redistribute it and/or
163+ * modify it under the terms of the GNU General Public License
164+ * as published by the Free Software Foundation; either version 2
165+ * of the License, or (at your option) any later version.
166+ *
167+ * This program is distributed in the hope that it will be useful,
168+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
169+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
170+ * GNU General Public License for more details.
171+ *
172+ **/
173+
174+#include <core/core.h>
175+#include <core/pluginclasshandler.h>
176+
177+#include <canberra.h>
178+
179+#include "bell_options.h"
180+
181+class AudibleBell :
182+ public PluginClassHandler<AudibleBell, CompScreen>,
183+ public ScreenInterface,
184+ public BellOptions
185+{
186+ public:
187+
188+ AudibleBell (CompScreen *screen);
189+ ~AudibleBell ();
190+
191+ bool bell ();
192+
193+ private:
194+
195+ ca_context *mCanberraContext;
196+};
197+
198+class BellPluginVTable :
199+ public CompPlugin::VTableForScreen<AudibleBell>
200+{
201+ public:
202+ bool init ();
203+};
204+

Subscribers

People subscribed via source and target branches