Created attachment 163184 [details] [review]
call plugin->switch_workspace, when screen doesn't have any window

I'd rather see a more general cleanup:

 * Remove the actors parameter to plugin->switch_workspace, it's just the same list of actors you could get from mutter_plugin_get_windows()

 * Remove the events parameter to plugin->kill_effect() and rename it to kill_window_effects()

 * Handle the fixme im mutter-plugin.h:

/*
 * FIXME -- move these to a private include
 * Required by plugin manager.
 */

 * Add plugin->kill_switch_workspace()

The current thing where we either call:

 ->kill_effect(plugin, window,  MUTTER_PLUGIN_ALL_EFFECTS &   ~MUTTER_PLUGIN_SWITCH_WORKSPACE)

Or:

 ->kill_effect(plugin, <random window>, MUTTER_PLUGIN_SWITCH_WORKSPACE)

Is pretty confusing and deceptive.

Created attachment 163305 [details] [review]
Refactor MutterPlugin

Remove the actors parameter to plugin->switch_workspace
Remove the events parameter to plugin->kill_effect and rename it to
kill_window_effects
Add plugin->kill_switch_workspace

Remove mutter_plugin_manager_kill_effect
Add mutter_plugin_manager_kill_window_effects
Add mutter_plugin_manager_kill_switch_workspace

Remove mutter_plugin_effect_completed
Add mutter_plugin_[minimize/map/destroy/maximize/unmaximize/]_completed

(In reply to comment #3)

Refactoring this makes good sense to me.

> Remove mutter_plugin_effect_completed
> Add mutter_plugin_[minimize/map/destroy/maximize/unmaximize/]_completed

Do we need a separate public function for each of these, could we not just make mutter_plugin_window_effect_completed() not static like the original function it replaces ?

(In reply to comment #4)
> Do we need a separate public function for each of these, could we not just make
> mutter_plugin_window_effect_completed() not static like the original function
> it replaces ?

mutter_plugin_window_effect_completed can handle only one effect, but it take unsigned long as argument.
So this will require create enum(map/destroy...) for replace this argument.

I think, set of function is better in this case.

(In reply to comment #5)
> (In reply to comment #4)
> > Do we need a separate public function for each of these, could we not just make
> > mutter_plugin_window_effect_completed() not static like the original function
> > it replaces ?
> 
> mutter_plugin_window_effect_completed can handle only one effect, but it take
> unsigned long as argument.
> So this will require create enum(map/destroy...) for replace this argument.
> 
> I think, set of function is better in this case.

I like the separate functions better too. It felt awkward to me before that there was a separate virtual function per effect, but then you had to translate the function name into an enumeration for effect_completed().

Review of attachment 163305 [details] [review]:

Patch looks good to me.

Better Subject would be:

 Clean up MutterPlugin effect interface

And I'd start the commit message body with:

 The current effect API passes an unnecessary list of windows to 
 switch_workspace() and forces a window to be passed in when killing
 the switch_workspace() effect.

 We can simplify the interface to correspond more closely to how 
 it is actually used and fix these problems: