GNOME Bugzilla – Bug 737798
Toggling media-keys inactive/active results in duplicate key commands
Last modified: 2014-10-17 16:09:19 UTC
Fedora 21 x86_64 + updates, including gnome-settings-daemon-3.14.0-1.fc21.x86_64 This is what I did to reproduce: 1. Assign a shortcut key to open a terminal -- I use Ctrl+Alt+T for this. Hit Ctrl+Alt+T to test. 2. gsettings set org.gnome.settings-daemon.plugins.media-keys active false 3. gsettings set org.gnome.settings-daemon.plugins.media-keys active true 4. Hit Ctrl+Alt+T ==> 2 new terminal windows open. 5. For fun, repeat 2-4 and you'll get 3 new terminals open, and so on.
Created attachment 287681 [details] [review] media-keys: Don't leak the key grabber proxy across plugin restarts In particular, this fixes multiple calls to on_accelerator_activated() after stopping and starting the plugin.
Created attachment 287682 [details] [review] media-keys: Don't leak the screencast proxy across shell restarts
Created attachment 287683 [details] [review] media-keys: Don't leak the idle GSource across plugin restarts
Created attachment 287684 [details] [review] media-keys: Don't leak the shell proxy across plugin restarts
Created attachment 287685 [details] [review] media-keys: Don't leak the logind proxy
Review of attachment 287681 [details] [review]: Yep.
Review of attachment 287682 [details] [review]: Yep.
Review of attachment 287683 [details] [review]: ::: plugins/media-keys/gsd-media-keys-manager.c @@ +2415,3 @@ + if (priv->start_idle_id != 0) + g_source_remove (priv->start_idle_id); missing setting the start_idle_id to 0 too.
Review of attachment 287684 [details] [review]: Instead of doing that, we should just make finalize call _stop(). That would avoid the code duplication.
Review of attachment 287685 [details] [review]: Looks good.
(In reply to comment #9) > Review of attachment 287684 [details] [review]: > > Instead of doing that, we should just make finalize call _stop(). We could though I see some plugins doing it while others don't. But see below > That would avoid the code duplication. Which duplication? The way I see it, the rule is pretty simple: whatever gets instanced in _start() must be unrefed in _stop() and whatever gets instanced in _init() must be unrefed in _finalize(). That's what I did here.
Created attachment 288703 [details] [review] media-keys: Don't leak the idle GSource across plugin restarts -- (In reply to comment #8) > missing setting the start_idle_id to 0 too. Doh, fixed
(In reply to comment #11) > (In reply to comment #9) > > Review of attachment 287684 [details] [review] [details]: > > > > Instead of doing that, we should just make finalize call _stop(). > > We could though I see some plugins doing it while others don't. But see below > > > That would avoid the code duplication. > > Which duplication? The way I see it, the rule is pretty simple: whatever gets > instanced in _start() must be unrefed in _stop() and whatever gets instanced in > _init() must be unrefed in _finalize(). That's what I did here. Right. But you need to do everything that stop() does in finalize, in case stop() wasn't called before finalize().
Pushed everything here and patches to call stop on finalize for all plugins that were missing it (including fixing some cases that weren't cleaning up completely on stop).