Merge lp:~alan-griffiths/compiz-core/rework-updatePlugins into lp:compiz-core

Proposed by Alan Griffiths on 2012-03-29
Status: Merged
Merged at revision: 3075
Proposed branch: lp:~alan-griffiths/compiz-core/rework-updatePlugins
Merge into: lp:compiz-core
Diff against target: 686 lines (+439/-107) (has conflicts)
4 files modified
include/core/plugin.h (+1/-1)
src/privatescreen.h (+6/-0)
src/privatescreen/tests/test-privatescreen.cpp (+352/-13)
src/screen.cpp (+80/-93)
Text conflict in src/privatescreen/tests/test-privatescreen.cpp
To merge this branch: bzr merge lp:~alan-griffiths/compiz-core/rework-updatePlugins
Reviewer Review Type Date Requested Status
Daniel van Vugt 2012-03-29 Approve on 2012-03-30
Review via email: mp+99981@code.launchpad.net

This proposal supersedes a proposal from 2012-03-28.

Description of the change

Avoid graphics corruption and hangs on compiz start-up by ensuring that
plugins don't get initialized, de-initialized and re-initialzed during
start-up. (LP: #963093) (LP: #963633)

The multiple init/fini/init calls occured when compiz was asked to load an
invalid plugin name. This occurred most recently as plugins "bailer" and
"detection" were left in some peoples' configs while the plugins themselves
no longer exist in the current compiz release.

The source of the graphics corruption and hangs has been found to be a
problem in the composite and/or opengl plugins. One or both of them are unsafe
to init/fini multiple times without a full compiz restart. So the root cause
is not exactly known yet. However composite and opengl are not alone; many
plugins have bugs with init/fini/init sequences, so it is valuable to fix
the start-up plugin ordering as this does.

Essentially this fix works by remembering which plugins don't exist at all
and putting them on a black list. Then subsequent updates check the blacklist
and know they should never include those failed plugins in testing whether
the active plugin lists have changed.

The final part of the fix is to remove a rendundant call to updatePlugins from
EventManager::init. It is not required as main tells PluginManager when to
load plugins on startup.

IMPORTANT NOTE FOR UBUNTU PACKAGING
In downstream ubuntu branches the DEFAULT_PLUGINS list in "debian/rules" also causes multiple plugin loads on start-up and prevents this fix from working! The solution is to change DEFAULT_PLUGINS to just "ccp".

ORIGINAL DESCRIPTION:
Some test cases to cover what we need.

Combined Daniel's "blacklist" with some cleanup of the code and ignoring any non-existent plugins entirely.

To post a comment you must log in.
Daniel van Vugt (vanvugt) wrote : Posted in a previous version of this proposal

Re: "Just because some plugins are "popped" to insert a new one, doesn't require them to be finalized."
This is untrue. Some plugins have loose dependencies on others and change their behaviour/features depending on what other plugins are loaded. Hence popped plugins must be fini'd and reinitialized.

Also, even if this branch works perfectly I would still be unhappy with the risk associated with such a large change in critical logic. Please don't hate me...

I'm looking at a much smaller/simpler solution. In fact I was designing it in my head as you designed this code. Will let you know if/when I think I have something that works.

In other news, the two critical bugs we're aiming to fix have had ZERO duplicates in the past two days. It sounds like the workarounds are working.

review: Abstain
Daniel van Vugt (vanvugt) wrote : Posted in a previous version of this proposal

I've tested it now. The big startup bugs are avoided, however it causes a regression: Disabling plugins in ccsm has no effect. Those plugins remain active and running even after they are removed from the active_plugins list. That's one reason why I proposed the new logging last night. It's useful :)

Arguably, it's an acceptable compromise to fix the critical issues and introduce a less severe bug. However the the size of the new code in updatePlugins still makes me a little scared. Also, I managed an equally successful hack yesterday with just a few lines changed, but similar regressions.

Daniel van Vugt (vanvugt) wrote : Posted in a previous version of this proposal

Replaced by: https://code.launchpad.net/~vanvugt/compiz-core/fix-startup-plugins/+merge/99886
Which has no such regressions. And is smaller, simpler.

review: Disapprove
Daniel van Vugt (vanvugt) wrote :

Manual testing in precise is all good. The bugs are fixed.

But...
1. Please don't change formatting unnecessarily from func (x) to func(x). I agree with the latter but it's not what compiz uses.
2. You alternative between both styles inconsistently: func (x) and func(x)
3. The critical bugs this branch fixes are not mentioned anywhere and we don't have a usable commit message. But I can add those.

review: Approve
Daniel van Vugt (vanvugt) wrote :

Also, please try to avoid renaming variables (even though you're right) this late in a stable release cycle. It increases the sizes of diffs and makes critical bug fixes harder to review.

Daniel van Vugt (vanvugt) wrote :

Updated the description and linked relevant bugs.

Daniel van Vugt (vanvugt) wrote :

Also, repeatedly calling: desiredPlugins[desireIndex].s ()
is inefficient and harder to read.

We should just do: const CompString &name = desiredPlugins[desireIndex].s ();
and then use "name".

Daniel van Vugt (vanvugt) wrote :

Damn. It did contain real conflicts, but I've fixed them.

3075. By Alan Griffiths on 2012-03-30

Avoid graphics corruption and hangs on compiz start-up by ensuring that
plugins don't get initialized, de-initialized and re-initialzed during
start-up. (LP: #963093) (LP: #963633)

The multiple init/fini/init calls occured when compiz was asked to load an
invalid plugin name. This occurred most recently as plugins "bailer" and
"detection" were left in some peoples' configs while the plugins themselves
no longer exist in the current compiz release.

The source of the graphics corruption and hangs has been found to be a
problem in the composite and/or opengl plugins. One or both of them are unsafe
to init/fini multiple times without a full compiz restart. So the root cause
is not exactly known yet. However composite and opengl are not alone; many
plugins have bugs with init/fini/init sequences, so it is valuable to fix
the start-up plugin ordering as this does.

Essentially this fix works by remembering which plugins don't exist at all
and putting them on a black list. Then subsequent updates check the blacklist
and know they should never include those failed plugins in testing whether
the active plugin lists have changed.

The final part of the fix is to remove a rendundant call to updatePlugins from
EventManager::init. It is not required as main tells PluginManager when to
load plugins on startup.

IMPORTANT NOTE FOR UBUNTU PACKAGING
In downstream ubuntu branches the DEFAULT_PLUGINS list in "debian/rules" also
causes multiple plugin loads on start-up and prevents this fix from working!
The solution is to change DEFAULT_PLUGINS to just "ccp".

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'include/core/plugin.h'
2--- include/core/plugin.h 2012-02-16 05:31:28 +0000
3+++ include/core/plugin.h 2012-03-29 17:16:19 +0000
4@@ -207,7 +207,7 @@
5 * CompPlugin::pop()'d.
6 */
7 static List & getPlugins ();
8-
9+
10 /**
11 * Gets a list of the names of all the known plugins, including plugins that may
12 * have already been loaded.
13
14=== modified file 'src/privatescreen.h'
15--- src/privatescreen.h 2012-03-24 09:49:30 +0000
16+++ src/privatescreen.h 2012-03-29 17:16:19 +0000
17@@ -45,6 +45,8 @@
18
19 #include "core_options.h"
20
21+#include <set>
22+
23 /**
24 * A wrapping of the X display screen. This takes care of communication to the
25 * X server.
26@@ -574,6 +576,10 @@
27 private:
28 CompOption::Value plugin;
29 bool dirtyPluginList;
30+ typedef std::set<CompString> CompStringSet;
31+ CompStringSet blacklist;
32+
33+ CompOption::Value::Vector mergedPluginList();
34 };
35
36 class GrabList
37
38=== modified file 'src/privatescreen/tests/test-privatescreen.cpp'
39--- src/privatescreen/tests/test-privatescreen.cpp 2012-03-29 13:33:39 +0000
40+++ src/privatescreen/tests/test-privatescreen.cpp 2012-03-29 17:16:19 +0000
41@@ -175,6 +175,29 @@
42 MOCK_CONST_METHOD0(createFailed, bool ());
43 };
44
45+class StubActivePluginsOption
46+{
47+public:
48+ StubActivePluginsOption(CoreOptions& co) : co(co)
49+ {
50+ CompOption::Vector& mOptions = co.getOptions ();
51+ CompOption::Value::Vector list;
52+ CompOption::Value value;
53+
54+ // active_plugins
55+ mOptions[CoreOptions::ActivePlugins].setName ("active_plugins", CompOption::TypeList);
56+ list.clear ();
57+ value.set(CompString ("core"));
58+ list.push_back (value);
59+ }
60+
61+ bool setActivePlugins(const char*, const char* key, CompOption::Value & value)
62+ {
63+ return co.setOption(key, value);
64+ }
65+private:
66+ CoreOptions& co;
67+};
68 } // (anon) namespace
69
70 namespace {
71@@ -254,6 +277,17 @@
72 }
73 return true;
74 }
75+
76+ CompStringList mockListPlugins (const char *path)
77+ {
78+ CompStringList list;
79+ list.push_back("one");
80+ list.push_back("two");
81+ list.push_back("three");
82+ list.push_back("four");
83+
84+ return list;
85+ }
86 };
87
88
89@@ -349,19 +383,320 @@
90 WillOnce(Invoke(&mockfs, &MockPluginFilesystem::DummyLoader));
91 EXPECT_CALL(mockfs, LoadPlugin(Ne((void*)0), EndsWith(HOME_PLUGINDIR), StrEq("two"))).
92 WillOnce(Invoke(&mockfs, &MockPluginFilesystem::DummyLoader));
93- EXPECT_CALL(mockfs, LoadPlugin(Ne((void*)0), EndsWith(HOME_PLUGINDIR), StrEq("three"))).
94- WillOnce(Invoke(&mockfs, &MockPluginFilesystem::DummyLoader));
95-
96- EXPECT_CALL(mockfs, UnloadPlugin(_)).Times(1); // Once for "three" which doesn't load
97-
98- EXPECT_CALL(comp_screen, _setOptionForPlugin(StrEq("core"), StrEq("active_plugins"), _)).
99- WillOnce(Return(false));
100-
101- ps.updatePlugins();
102-
103- // TODO Some cleanup that probably ought to be automatic.
104- EXPECT_CALL(mockfs, UnloadPlugin(_)).Times(2);
105- EXPECT_CALL(comp_screen, _finiPluginForScreen(Ne((void*)0))).Times(2);
106+<<<<<<< TREE
107+ EXPECT_CALL(mockfs, LoadPlugin(Ne((void*)0), EndsWith(HOME_PLUGINDIR), StrEq("three"))).
108+ WillOnce(Invoke(&mockfs, &MockPluginFilesystem::DummyLoader));
109+
110+ EXPECT_CALL(mockfs, UnloadPlugin(_)).Times(1); // Once for "three" which doesn't load
111+
112+ EXPECT_CALL(comp_screen, _setOptionForPlugin(StrEq("core"), StrEq("active_plugins"), _)).
113+ WillOnce(Return(false));
114+
115+ ps.updatePlugins();
116+
117+ // TODO Some cleanup that probably ought to be automatic.
118+ EXPECT_CALL(mockfs, UnloadPlugin(_)).Times(2);
119+ EXPECT_CALL(comp_screen, _finiPluginForScreen(Ne((void*)0))).Times(2);
120+=======
121+ EXPECT_CALL(mockfs.mockVtableTwo, init()).WillOnce(Return(true));
122+
123+ EXPECT_CALL(mockfs, LoadPlugin(Ne((void*)0), EndsWith(HOME_PLUGINDIR), StrEq("three"))).
124+ WillOnce(Invoke(&mockfs, &MockPluginFilesystem::DummyLoader));
125+ EXPECT_CALL(mockfs.mockVtableThree, init()).WillOnce(Return(false));
126+
127+ EXPECT_CALL(mockfs, UnloadPlugin(_)).Times(1); // Once for "three" which doesn't load
128+
129+ EXPECT_CALL(comp_screen, _setOptionForPlugin(StrEq("core"), StrEq("active_plugins"), _)).
130+ WillOnce(Return(false));
131+
132+ EXPECT_CALL(mockfs, ListPlugins(_)).
133+ WillRepeatedly(Invoke(&mockfs, &MockPluginFilesystem::mockListPlugins));
134+
135+ ps.updatePlugins();
136+
137+ Mock::VerifyAndClearExpectations(&mockfs);
138+ Mock::VerifyAndClearExpectations(&mockfs.mockVtableOne);
139+ Mock::VerifyAndClearExpectations(&mockfs.mockVtableTwo);
140+ Mock::VerifyAndClearExpectations(&mockfs.mockVtableThree);
141+ Mock::VerifyAndClearExpectations(&mockfs.mockVtableFour);
142+
143+ // TODO Some cleanup that probably ought to be automatic.
144+ EXPECT_CALL(mockfs, UnloadPlugin(_)).Times(2);
145+ EXPECT_CALL(comp_screen, _finiPluginForScreen(Ne((void*)0))).Times(2);
146+ EXPECT_CALL(mockfs.mockVtableOne, finiScreen(Ne((void*)0))).Times(1);
147+ EXPECT_CALL(mockfs.mockVtableOne, fini()).Times(1);
148+ EXPECT_CALL(mockfs.mockVtableTwo, finiScreen(Ne((void*)0))).Times(1);
149+ EXPECT_CALL(mockfs.mockVtableTwo, fini()).Times(1);
150+
151+ for (CompPlugin* p; (p = CompPlugin::pop ()) != 0; CompPlugin::unload (p));
152+}
153+
154+TEST(privatescreen_PluginManagerTest, updating_when_failing_to_load_plugin_in_middle_of_list)
155+{
156+ using namespace testing;
157+
158+ MockCompScreen comp_screen;
159+
160+ cps::PluginManager ps(&comp_screen);
161+ StubActivePluginsOption sapo(ps);
162+
163+ CompOption::Value::Vector values;
164+ values.push_back ("core");
165+ ps.setPlugins (values);
166+ ps.setDirtyPluginList ();
167+
168+ std::list <CompString> plugins;
169+ plugins.push_back ("one");
170+ plugins.push_back ("three");
171+ plugins.push_back ("four");
172+ initialPlugins = plugins;
173+
174+ MockPluginFilesystem mockfs;
175+
176+ EXPECT_CALL(mockfs, LoadPlugin(Ne((void*)0), EndsWith(HOME_PLUGINDIR), StrEq("one"))).
177+ WillOnce(Invoke(&mockfs, &MockPluginFilesystem::DummyLoader));
178+ EXPECT_CALL(mockfs.mockVtableOne, init()).WillOnce(Return(true));
179+
180+ EXPECT_CALL(mockfs, LoadPlugin(Ne((void*)0), EndsWith(HOME_PLUGINDIR), StrEq("three"))).
181+ WillOnce(Invoke(&mockfs, &MockPluginFilesystem::DummyLoader));
182+ EXPECT_CALL(mockfs.mockVtableThree, init()).WillOnce(Return(false));
183+
184+ EXPECT_CALL(mockfs, LoadPlugin(Ne((void*)0), EndsWith(HOME_PLUGINDIR), StrEq("four"))).
185+ WillOnce(Invoke(&mockfs, &MockPluginFilesystem::DummyLoader));
186+ EXPECT_CALL(mockfs.mockVtableFour, init()).Times(1).WillRepeatedly(Return(true));
187+
188+ EXPECT_CALL(mockfs, UnloadPlugin(_)).Times(1); // Once for "three" which doesn't load
189+
190+ EXPECT_CALL(comp_screen, _setOptionForPlugin(StrEq("core"), StrEq("active_plugins"), _)).
191+ WillOnce(Return(true));
192+
193+ EXPECT_CALL(mockfs, ListPlugins(_)).
194+ WillRepeatedly(Invoke(&mockfs, &MockPluginFilesystem::mockListPlugins));
195+
196+ ps.updatePlugins();
197+
198+ Mock::VerifyAndClearExpectations(&mockfs);
199+ Mock::VerifyAndClearExpectations(&mockfs.mockVtableOne);
200+ Mock::VerifyAndClearExpectations(&mockfs.mockVtableTwo);
201+ Mock::VerifyAndClearExpectations(&mockfs.mockVtableThree);
202+ Mock::VerifyAndClearExpectations(&mockfs.mockVtableFour);
203+
204+ EXPECT_CALL(comp_screen, _setOptionForPlugin(StrEq("core"), StrEq("active_plugins"), _)).
205+ WillOnce(Invoke(&sapo, &StubActivePluginsOption::setActivePlugins));
206+
207+ EXPECT_CALL(mockfs, ListPlugins(_)).
208+ WillRepeatedly(Invoke(&mockfs, &MockPluginFilesystem::mockListPlugins));
209+
210+ ps.updatePlugins();
211+
212+ Mock::VerifyAndClearExpectations(&mockfs);
213+ Mock::VerifyAndClearExpectations(&mockfs.mockVtableOne);
214+ Mock::VerifyAndClearExpectations(&mockfs.mockVtableTwo);
215+ Mock::VerifyAndClearExpectations(&mockfs.mockVtableThree);
216+ Mock::VerifyAndClearExpectations(&mockfs.mockVtableFour);
217+
218+ // TODO Some cleanup that probably ought to be automatic.
219+ EXPECT_CALL(mockfs, UnloadPlugin(_)).Times(2);
220+ EXPECT_CALL(comp_screen, _finiPluginForScreen(Ne((void*)0))).Times(2);
221+ EXPECT_CALL(mockfs.mockVtableOne, finiScreen(Ne((void*)0))).Times(1);
222+ EXPECT_CALL(mockfs.mockVtableOne, fini()).Times(1);
223+ EXPECT_CALL(mockfs.mockVtableFour, finiScreen(Ne((void*)0))).Times(1);
224+ EXPECT_CALL(mockfs.mockVtableFour, fini()).Times(1);
225+ for (CompPlugin* p; (p = CompPlugin::pop ()) != 0; CompPlugin::unload (p));
226+}
227+
228+TEST(privatescreen_PluginManagerTest, calling_updatePlugins_with_fewer_plugins)
229+{
230+ using namespace testing;
231+
232+ MockCompScreen comp_screen;
233+
234+ cps::PluginManager ps(&comp_screen);
235+
236+ StubActivePluginsOption sapo(ps);
237+
238+ // Stuff that has to be done before calling updatePlugins()
239+ initialPlugins = std::list <CompString>();
240+ CompOption::Value::Vector values;
241+ values.push_back ("core");
242+ ps.setPlugins (values);
243+ ps.setDirtyPluginList ();
244+
245+ {
246+ CompOption::Value::Vector plugins;
247+ plugins.push_back ("one");
248+ plugins.push_back ("two");
249+ plugins.push_back ("three");
250+ CompOption::Value v(plugins);
251+ sapo.setActivePlugins("core", "active_plugins", v);
252+ }
253+
254+ MockPluginFilesystem mockfs;
255+
256+ EXPECT_CALL(mockfs, LoadPlugin(Ne((void*)0), EndsWith(HOME_PLUGINDIR), StrEq("one"))).
257+ WillOnce(Invoke(&mockfs, &MockPluginFilesystem::DummyLoader));
258+ EXPECT_CALL(mockfs.mockVtableOne, init()).WillOnce(Return(true));
259+
260+ EXPECT_CALL(mockfs, LoadPlugin(Ne((void*)0), EndsWith(HOME_PLUGINDIR), StrEq("two"))).
261+ WillOnce(Invoke(&mockfs, &MockPluginFilesystem::DummyLoader));
262+ EXPECT_CALL(mockfs.mockVtableTwo, init()).WillOnce(Return(true));
263+
264+ EXPECT_CALL(mockfs, LoadPlugin(Ne((void*)0), EndsWith(HOME_PLUGINDIR), StrEq("three"))).
265+ WillOnce(Invoke(&mockfs, &MockPluginFilesystem::DummyLoader));
266+ EXPECT_CALL(mockfs.mockVtableThree, init()).WillOnce(Return(true));
267+
268+ EXPECT_CALL(comp_screen, _setOptionForPlugin(StrEq("core"), StrEq("active_plugins"), _)).
269+ WillOnce(Invoke(&sapo, &StubActivePluginsOption::setActivePlugins));
270+
271+ EXPECT_CALL(mockfs, ListPlugins(_)).
272+ WillRepeatedly(Invoke(&mockfs, &MockPluginFilesystem::mockListPlugins));
273+
274+ ps.updatePlugins();
275+
276+ Mock::VerifyAndClearExpectations(&mockfs);
277+ Mock::VerifyAndClearExpectations(&mockfs.mockVtableOne);
278+ Mock::VerifyAndClearExpectations(&mockfs.mockVtableTwo);
279+ Mock::VerifyAndClearExpectations(&mockfs.mockVtableThree);
280+ Mock::VerifyAndClearExpectations(&mockfs.mockVtableFour);
281+
282+ EXPECT_CALL(comp_screen, _finiPluginForScreen(Ne((void*)0))).Times(2);
283+ EXPECT_CALL(mockfs, UnloadPlugin(_)).Times(1);
284+ EXPECT_CALL(mockfs.mockVtableTwo, finiScreen(Ne((void*)0))).Times(1);
285+ EXPECT_CALL(mockfs.mockVtableTwo, fini()).Times(1);
286+ EXPECT_CALL(mockfs.mockVtableThree, fini()).Times(1);
287+ EXPECT_CALL(mockfs.mockVtableThree, finiScreen(Ne((void*)0))).Times(1);
288+ EXPECT_CALL(mockfs.mockVtableThree, init()).WillOnce(Return(true));
289+ EXPECT_CALL(comp_screen, _setOptionForPlugin(StrEq("core"), StrEq("active_plugins"), _)).
290+ WillOnce(Invoke(&sapo, &StubActivePluginsOption::setActivePlugins));
291+
292+ {
293+ CompOption::Value::Vector plugins;
294+ plugins.push_back ("one");
295+ plugins.push_back ("three");
296+ CompOption::Value v(plugins);
297+ sapo.setActivePlugins("core", "active_plugins", v);
298+ }
299+
300+ EXPECT_CALL(mockfs, ListPlugins(_)).
301+ WillRepeatedly(Invoke(&mockfs, &MockPluginFilesystem::mockListPlugins));
302+
303+ ps.updatePlugins();
304+
305+ Mock::VerifyAndClearExpectations(&mockfs);
306+ Mock::VerifyAndClearExpectations(&mockfs.mockVtableOne);
307+ Mock::VerifyAndClearExpectations(&mockfs.mockVtableTwo);
308+ Mock::VerifyAndClearExpectations(&mockfs.mockVtableThree);
309+ Mock::VerifyAndClearExpectations(&mockfs.mockVtableFour);
310+
311+ // TODO Some cleanup that probably ought to be automatic.
312+ EXPECT_CALL(mockfs, UnloadPlugin(_)).Times(2);
313+ EXPECT_CALL(comp_screen, _finiPluginForScreen(Ne((void*)0))).Times(2);
314+ EXPECT_CALL(mockfs.mockVtableOne, finiScreen(Ne((void*)0))).Times(1);
315+ EXPECT_CALL(mockfs.mockVtableOne, fini()).Times(1);
316+ EXPECT_CALL(mockfs.mockVtableThree, finiScreen(Ne((void*)0))).Times(1);
317+ EXPECT_CALL(mockfs.mockVtableThree, fini()).Times(1);
318+
319+ for (CompPlugin* p; (p = CompPlugin::pop ()) != 0; CompPlugin::unload (p));
320+}
321+
322+TEST(privatescreen_PluginManagerTest, calling_updatePlugins_with_additional_plugins)
323+{
324+ using namespace testing;
325+
326+ MockCompScreen comp_screen;
327+
328+ cps::PluginManager ps(&comp_screen);
329+
330+ StubActivePluginsOption sapo(ps);
331+
332+ // Stuff that has to be done before calling updatePlugins()
333+ initialPlugins = std::list <CompString>();
334+ CompOption::Value::Vector values;
335+ values.push_back ("core");
336+ ps.setPlugins (values);
337+ ps.setDirtyPluginList ();
338+
339+ {
340+ CompOption::Value::Vector plugins;
341+ plugins.push_back ("one");
342+ plugins.push_back ("two");
343+ plugins.push_back ("four");
344+ CompOption::Value v(plugins);
345+ sapo.setActivePlugins("core", "active_plugins", v);
346+ }
347+
348+ MockPluginFilesystem mockfs;
349+
350+ EXPECT_CALL(mockfs, LoadPlugin(Ne((void*)0), EndsWith(HOME_PLUGINDIR), StrEq("one"))).
351+ WillOnce(Invoke(&mockfs, &MockPluginFilesystem::DummyLoader));
352+ EXPECT_CALL(mockfs.mockVtableOne, init()).WillOnce(Return(true));
353+
354+ EXPECT_CALL(mockfs, LoadPlugin(Ne((void*)0), EndsWith(HOME_PLUGINDIR), StrEq("two"))).
355+ WillOnce(Invoke(&mockfs, &MockPluginFilesystem::DummyLoader));
356+ EXPECT_CALL(mockfs.mockVtableTwo, init()).WillOnce(Return(true));
357+
358+ EXPECT_CALL(mockfs, LoadPlugin(Ne((void*)0), EndsWith(HOME_PLUGINDIR), StrEq("four"))).
359+ WillOnce(Invoke(&mockfs, &MockPluginFilesystem::DummyLoader));
360+ EXPECT_CALL(mockfs.mockVtableFour, init()).WillOnce(Return(true));
361+
362+ EXPECT_CALL(comp_screen, _setOptionForPlugin(StrEq("core"), StrEq("active_plugins"), _)).
363+ WillOnce(Invoke(&sapo, &StubActivePluginsOption::setActivePlugins));
364+
365+ EXPECT_CALL(mockfs, ListPlugins(_)).
366+ WillRepeatedly(Invoke(&mockfs, &MockPluginFilesystem::mockListPlugins));
367+
368+ ps.updatePlugins();
369+
370+ Mock::VerifyAndClearExpectations(&mockfs);
371+ Mock::VerifyAndClearExpectations(&mockfs.mockVtableOne);
372+ Mock::VerifyAndClearExpectations(&mockfs.mockVtableTwo);
373+ Mock::VerifyAndClearExpectations(&mockfs.mockVtableThree);
374+ Mock::VerifyAndClearExpectations(&mockfs.mockVtableFour);
375+
376+ EXPECT_CALL(comp_screen, _finiPluginForScreen(Ne((void*)0))).Times(1);
377+ EXPECT_CALL(mockfs.mockVtableFour, fini()).Times(1);
378+ EXPECT_CALL(mockfs.mockVtableFour, finiScreen(Ne((void*)0))).Times(1);
379+ EXPECT_CALL(mockfs, LoadPlugin(Ne((void*)0), EndsWith(HOME_PLUGINDIR), StrEq("three"))).
380+ WillOnce(Invoke(&mockfs, &MockPluginFilesystem::DummyLoader));
381+ EXPECT_CALL(mockfs.mockVtableThree, init()).WillOnce(Return(true));
382+ EXPECT_CALL(mockfs.mockVtableFour, init()).WillOnce(Return(true));
383+ EXPECT_CALL(comp_screen, _setOptionForPlugin(StrEq("core"), StrEq("active_plugins"), _)).
384+ WillOnce(Invoke(&sapo, &StubActivePluginsOption::setActivePlugins));
385+
386+ {
387+ CompOption::Value::Vector plugins;
388+ plugins.push_back ("one");
389+ plugins.push_back ("two");
390+ plugins.push_back ("three");
391+ plugins.push_back ("four");
392+ CompOption::Value v(plugins);
393+ sapo.setActivePlugins("core", "active_plugins", v);
394+ }
395+
396+ EXPECT_CALL(mockfs, ListPlugins(_)).
397+ WillRepeatedly(Invoke(&mockfs, &MockPluginFilesystem::mockListPlugins));
398+
399+ ps.updatePlugins();
400+
401+ Mock::VerifyAndClearExpectations(&mockfs);
402+ Mock::VerifyAndClearExpectations(&mockfs.mockVtableOne);
403+ Mock::VerifyAndClearExpectations(&mockfs.mockVtableTwo);
404+ Mock::VerifyAndClearExpectations(&mockfs.mockVtableThree);
405+ Mock::VerifyAndClearExpectations(&mockfs.mockVtableFour);
406+
407+ // TODO Some cleanup that probably ought to be automatic.
408+ EXPECT_CALL(mockfs, UnloadPlugin(_)).Times(4);
409+ EXPECT_CALL(comp_screen, _finiPluginForScreen(Ne((void*)0))).Times(4);
410+ EXPECT_CALL(mockfs.mockVtableFour, finiScreen(Ne((void*)0))).Times(1);
411+ EXPECT_CALL(mockfs.mockVtableFour, fini()).Times(1);
412+ EXPECT_CALL(mockfs.mockVtableThree, finiScreen(Ne((void*)0))).Times(1);
413+ EXPECT_CALL(mockfs.mockVtableThree, fini()).Times(1);
414+ EXPECT_CALL(mockfs.mockVtableTwo, finiScreen(Ne((void*)0))).Times(1);
415+ EXPECT_CALL(mockfs.mockVtableTwo, fini()).Times(1);
416+ EXPECT_CALL(mockfs.mockVtableOne, finiScreen(Ne((void*)0))).Times(1);
417+ EXPECT_CALL(mockfs.mockVtableOne, fini()).Times(1);
418+
419+>>>>>>> MERGE-SOURCE
420 for (CompPlugin* p; (p = CompPlugin::pop ()) != 0; CompPlugin::unload (p));
421 }
422
423@@ -398,5 +733,9 @@
424 comp_screen.priv.reset(new PrivateScreen(&comp_screen));
425 cps::EventManager& em(*comp_screen.priv.get());
426
427+<<<<<<< TREE
428+=======
429+ em.setPlugins (values);
430+>>>>>>> MERGE-SOURCE
431 em.init(0);
432 }
433
434=== modified file 'src/screen.cpp'
435--- src/screen.cpp 2012-03-24 09:49:30 +0000
436+++ src/screen.cpp 2012-03-29 17:16:19 +0000
437@@ -893,83 +893,45 @@
438 }
439 }
440
441-void
442-cps::PluginManager::updatePlugins ()
443+CompOption::Value::Vector
444+cps::PluginManager::mergedPluginList ()
445 {
446- unsigned int pListCount = 1;
447-
448- possibleTap = NULL;
449- dirtyPluginList = false;
450-
451- CompOption::Value::Vector &list = optionGetActivePlugins ();
452-
453- /* Determine the number of plugins, which is core +
454- * initial plugins + plugins in option list in addition
455- * to initial plugins */
456- foreach (CompString &pn, initialPlugins)
457- {
458- if (pn != "core")
459- pListCount++;
460- }
461-
462- CompString lpName;
463-
464- foreach (CompOption::Value &lp, list)
465- {
466- bool skip = false;
467-
468- lpName = lp.s();
469-
470- if (lpName == "core")
471- continue;
472-
473- foreach (CompString &p, initialPlugins)
474- {
475- if (p == lpName)
476- {
477- skip = true;
478- break;
479- }
480- }
481-
482- /* plugin not in initial list */
483- if (!skip)
484- pListCount++;
485- }
486-
487- /* pListCount is now the number of plugisn contained in both the
488- * initial and new plugins list */
489- CompOption::Value::Vector pList(pListCount);
490-
491- if (pList.empty ())
492- {
493- screen->setOptionForPlugin ("core", "active_plugins", plugin);
494- return;
495- }
496+ std::list<CompString> availablePlugins(CompPlugin::availablePlugins ());
497+
498+ CompOption::Value::Vector result;
499+
500+ CompOption::Value::Vector const& extraPluginsRequested = optionGetActivePlugins();
501
502 /* Must have core as first plugin */
503- pList.at (0) = "core";
504- unsigned int j = 1;
505+ result.push_back("core");
506
507 /* Add initial plugins */
508- foreach (CompString &p, initialPlugins)
509+ foreach(CompString & p, initialPlugins)
510 {
511 if (p == "core")
512 continue;
513- pList.at (j).set (p);
514- j++;
515+
516+ if (blacklist.find (p) != blacklist.end ())
517+ continue;
518+
519+ if (availablePlugins.end() != std::find(availablePlugins.begin(), availablePlugins.end(), p))
520+ result.push_back(p);
521 }
522
523 /* Add plugins not in the initial list */
524- foreach (CompOption::Value &opt, list)
525+ foreach(CompOption::Value const& opt, extraPluginsRequested)
526 {
527- std::list <CompString>::iterator it = initialPlugins.begin ();
528- bool skip = false;
529-
530- if (opt.s () == "core")
531- continue;
532-
533- for (; it != initialPlugins.end (); it++)
534+ if (opt.s() == "core")
535+ continue;
536+
537+ if (blacklist.find (opt.s()) != blacklist.end ())
538+ continue;
539+
540+ typedef std::list<CompString>::iterator iterator;
541+ bool skip = false;
542+
543+ for (iterator it = initialPlugins.begin(); it != initialPlugins.end();
544+ it++)
545 {
546 if ((*it) == opt.s())
547 {
548@@ -980,50 +942,71 @@
549
550 if (!skip)
551 {
552- pList.at (j++).set (opt.s ());
553+ if (availablePlugins.end() != std::find(availablePlugins.begin(), availablePlugins.end(), opt.s()))
554+ result.push_back(opt.s());
555 }
556 }
557-
558- assert (j == pList.size ());
559-
560- /* j is initialized to 1 to make sure we never pop the core plugin */
561- unsigned int i;
562- for (i = j = 1; j < plugin.list ().size () && i < pList.size (); i++, j++)
563+ return result;
564+}
565+
566+
567+void
568+cps::PluginManager::updatePlugins ()
569+{
570+ possibleTap = NULL;
571+ dirtyPluginList = false;
572+
573+ CompOption::Value::Vector const desiredPlugins(mergedPluginList());
574+
575+ unsigned int pluginIndex;
576+ for (pluginIndex = 1;
577+ pluginIndex < plugin.list ().size () && pluginIndex < desiredPlugins.size ();
578+ pluginIndex++)
579 {
580- if (plugin.list ().at (j).s () != pList.at (i).s ())
581+ if (plugin.list ().at (pluginIndex).s () != desiredPlugins.at (pluginIndex).s ())
582 break;
583 }
584
585- CompPlugin::List pop;
586+ unsigned int desireIndex = pluginIndex;
587
588- if (unsigned int const nPop = plugin.list ().size () - j)
589+ // We have pluginIndex pointing at first difference (or end).
590+ // Now pop plugins off stack to this point, but keep track that they are loaded
591+ CompPlugin::List alreadyLoaded;
592+ if (const unsigned int nPop = plugin.list().size() - pluginIndex)
593 {
594- for (j = 0; j < nPop; j++)
595+ for (pluginIndex = 0; pluginIndex < nPop; pluginIndex++)
596 {
597- pop.push_back (CompPlugin::pop ());
598- plugin.list ().pop_back ();
599+ alreadyLoaded.push_back(CompPlugin::pop());
600+ plugin.list().pop_back();
601 }
602 }
603
604- for (; i < pList.size (); i++)
605+ // Now work forward through requested plugins
606+ for (; desireIndex < desiredPlugins.size(); desireIndex++)
607 {
608 CompPlugin *p = NULL;
609 bool failedPush = false;
610
611- foreach (CompPlugin *pp, pop)
612+ // If already loaded, just try to push it...
613+ foreach(CompPlugin * pp, alreadyLoaded)
614 {
615- if (pList[i]. s () == pp->vTable->name ())
616+ if (desiredPlugins[desireIndex].s() == pp->vTable->name())
617 {
618 if (CompPlugin::push (pp))
619 {
620 p = pp;
621- pop.erase (std::find (pop.begin (), pop.end (), pp));
622+ alreadyLoaded.erase(
623+ std::find(alreadyLoaded.begin(),
624+ alreadyLoaded.end(), pp));
625 break;
626 }
627 else
628 {
629- pop.erase (std::find (pop.begin (), pop.end (), pp));
630- CompPlugin::unload (pp);
631+ alreadyLoaded.erase(
632+ std::find(alreadyLoaded.begin(),
633+ alreadyLoaded.end(), pp));
634+ blacklist.insert (desiredPlugins[desireIndex].s ());
635+ CompPlugin::unload(pp);
636 p = NULL;
637 failedPush = true;
638 break;
639@@ -1031,25 +1014,32 @@
640 }
641 }
642
643+ // ...otherwise, try to load and push
644 if (p == 0 && !failedPush)
645 {
646- p = CompPlugin::load (pList[i].s ().c_str ());
647-
648+ p = CompPlugin::load(desiredPlugins[desireIndex].s ().c_str ());
649+
650 if (p)
651 {
652- if (!CompPlugin::push (p))
653+ if (!CompPlugin::push(p))
654 {
655- CompPlugin::unload (p);
656+ blacklist.insert (desiredPlugins[desireIndex].s ());
657+ CompPlugin::unload(p);
658 p = 0;
659 }
660 }
661+ else
662+ {
663+ blacklist.insert (desiredPlugins[desireIndex].s ());
664+ }
665 }
666
667 if (p)
668- plugin.list ().push_back (p->vTable->name ());
669+ plugin.list().push_back(p->vTable->name());
670 }
671
672- foreach (CompPlugin *pp, pop)
673+ // Any plugins that are loaded, but were not re-initialized can be unloaded.
674+ foreach(CompPlugin * pp, alreadyLoaded)
675 CompPlugin::unload (pp);
676
677 if (!dirtyPluginList)
678@@ -4626,9 +4616,6 @@
679
680 optionSetToggleWindowShadedKeyInitiate (CompScreenImpl::shadeWin);
681
682- if (isDirtyPluginList ())
683- updatePlugins ();
684-
685 return true;
686 }
687

Subscribers

People subscribed via source and target branches