GNOME Bugzilla – Bug 559797
GrabMediaPlayerKeys does not respect "time" parameter
Last modified: 2008-11-20 21:39:05 UTC
Please describe the problem: When an application calls GrabMediaPlayerKeys with a non-zero "time" parameter, it overrides applications which have called it with time=0. An example is Evince, which calls GrabMediaPlayerKeys with time=1, which overrides Banshee, which calls GrabMediaPlayerKeys with time=0. Steps to reproduce: 1. Start Banshee 2. Press multimedia keys to confirm that it is working with Banshee 3. Start Evince 4. Press multimedia keys Actual results: Evince responds to the multimedia keys. Expected results: Banshee responds to the multimedia keys. Does this happen every time? Yes Other information: If I quit Banshee and start it again, Banshee has control of the multimedia keys again. Same happens when reloading "Multimedia Keys" extension of Banshee.
Created attachment 122204 [details] [review] Patch against 2.24.0 which fixes the issue
Hm. The code does look inconsistent, indeed. Bastien, what were the intended semantics here?
Okay, I found that the patch I did earlier introduced a new issue where applications with time=0 end up being added to the bottom of the list if it makes the grab after another application with time>0, i.e. if Banshee starts after Evince, Evince retains the grab. I'll upload an updated patch shortly.
Created attachment 122224 [details] [review] Updated one which works
The last app is supposed to get priority, 0 is supposed to mean "current time". Something like that would probably do: - media_player->time = time; + if (time == 0) { + media_player->time = GDK_CURRENT_TIME; + } else { + media_player->time = time; + }
And what does "current time" mean?
What do you mean? "Current time" means now aka GDK_CURRENT_TIME, so that the last app to register gets highest priority (if it uses 0, that is). Does Bastien's proposed change fix your problem? It basically does the same thing your patch tried to do, only a bit simpler (and arguably more correct since it fixes the current timestamp instead of leaving it up in the air like before).
I'm not sure exactly which lines he means. Is this patch supposed to be applied within the gsd_media_keys_manager_grab_media_player_keys function? Another thing I find weird is that GDK_CURRENT_TIME is defined as 0L, so wouldn't that be the same as media_player->time = time? (when time = 0 that is). Anyway I'll try modifying the function I mentioned above and see if that helps.
Okay, I've just tested the patch by Bastien, and it has no effect whatsoever.
Hm, yeah, makes sense, I guess. We need to use g_get_current_time() or some such when GDK_CURRENT_TIME (ie. 0) is passed.
Well, when I was coming up with the patch, I noticed that the original find_by_time function only returns 0, no matter what the input. I believe the flaw lies in find_by_time, not anywhere else.
That's just another angle on the same issue. If we set the time correctly in the first place, find_by_time becomes as simple as: return ((MediaPlayer *)a)->time < ((MediaPlayer *)b)->time;
In that case, the media-player list would be sorted in decreasing order of media_player->time, and so media_player->time has to be set to an extremely large value (MAX_INT?) if the D-Bus call was made with time=0, wouldn't it?
No, it needs to be set to the current system time. That way any new app that registers with time=GDK_CURRENT_TIME automatically moves to the top of the stack.
Then find_by_time would need to be modified accordingly wouldn't it? Something like.. if ((MediaPlayer*)a)->time == GDK_CURRENT_TIME) return 0; return ((MediaPlayer*)a)->time < ((MediaPlayer*)b)->time;
Created attachment 123129 [details] [review] handle GDK_CURRENT_TIME Here's the proposed change.
Looks like it would do the trick.
I have tested the patch, and can testify that it works.
Thanks for testing. 2008-11-20 Jens Granseuer <...> * plugins/media-keys/gsd-media-keys-manager.c: (find_by_time), (gsd_media_keys_manager_grab_media_player_keys): fix handling of time = GDK_CURRENT_TIME. Previously, apps that registered with GDK_CURRENT_TIME would be trumped by any app that registered with time != 0 (bug #559797)