GNOME Bugzilla – Bug 695021
media-keys: Delegate popping up OSDs to GNOME Shell
Last modified: 2013-03-04 12:35:08 UTC
See bug 613543 for the shell part. In statistics: g-s changes: 218 insertions(+) g-s-d changes: 160 insertions(+), 1547 deletions(-) Totally worth it!
Created attachment 237839 [details] [review] media-keys: Copy get_image_name_for_volume() from gsd-osd-window.c Just as we now refer key grabbing to the shell, we will do the same for OSD popups. Determining the correct volume icon is something we'll still need to do, while the rest of GsdOsdWindow will become obsolete and removed, so copy the function over.
Created attachment 237840 [details] [review] media-keys: Generalize ensure_drag_cancellable() We will add another proxy for OSDs, changing the function to take the address of a GCancellable* will make it reusable for that case.
Created attachment 237841 [details] [review] media-keys: Use the shell's ShowOSD() method for OSDs Now that fallback mode has been dropped and we rely on the shell, there's no longer any good reason to implement OSDs in g-s-d (a background service!) while trying to mimick the shell style as close as possible. Instead, delegate showing the actual UI to the shell.
Created attachment 237842 [details] [review] media-keys: Remove the old OSD implementation
Oh, I just noticed that there's another OSD implementation in the wacom plugin :(
(In reply to comment #5) > Oh, I just noticed that there's another OSD implementation in the wacom plugin > :( Which has nothing to do with what media-keys might show.
Review of attachment 237839 [details] [review]: Looks good.
Review of attachment 237840 [details] [review]: ++
Review of attachment 237841 [details] [review]: Looks good otherwise. ::: plugins/media-keys/gsd-media-keys-manager.c @@ +415,3 @@ +static GVariant * +build_osd_params (const char *icon_name, + GIcon *gicon, The code you just added in the first patch returns a GIcon, but could just as easily return a string, and would avoid this bizarre icon or gicon code below. I also don't really like this separate "build_osd_params" seeing as we'll always call it as one of show_osd's parameters.
Review of attachment 237842 [details] [review]: Yep.
Created attachment 237870 [details] [review] media-keys: Adjust get_image_name_for_volume() to return a string The shell's OSD DBus interface expects serialized GIcons for the icons, e.g. the result of g_icon_to_string(). For GThemedIcons with exactly one name, this is guaranteed to be the icon name itself, so return that instead of a GIcon.
Created attachment 237871 [details] [review] media-keys: Use the shell's ShowOSD() method for OSDs (In reply to comment #10) > The code you just added in the first patch returns a GIcon, but could just as > easily return a string, and would avoid this bizarre icon or gicon code below. Yeah, it makes sense to use the icon name directly here. Actually I noticed that GThemedIcon with a single icon name guarantees that g_icon_to_string() returns the icon name itself, so I adjusted the DBus API to always require a serialized GIcon, so the icon/gicon code is gone for good! > I also don't really like this separate "build_osd_params" seeing as we'll > always call it as one of show_osd's parameters. The idea was to have build_osd_params() as the general method, and then add special cases for volume etc. like the existing osd_$foo_action() stuff. But as it turned out, I just ended up using the same method everywhere, so yeah, makes sense to just merge this with show_osd() itself.
Created attachment 237873 [details] [review] media-keys: Use the shell's ShowOSD() method for OSDs Accidentally dropped the "level" parameter of the battery action in the last version ...
Review of attachment 237870 [details] [review]: Fine.
Review of attachment 237873 [details] [review]: Looks good. ::: plugins/media-keys/gsd-media-keys-manager.c @@ +1114,3 @@ + gvc_mixer_ui_device_get_description (device), vol); + } else { + show_osd (manager, icon, NULL, vol); Indentation.
Attachment 237839 [details] pushed as 2db01ed - media-keys: Copy get_image_name_for_volume() from gsd-osd-window.c Attachment 237840 [details] pushed as dfeeca1 - media-keys: Generalize ensure_drag_cancellable() Attachment 237842 [details] pushed as 476781c - media-keys: Remove the old OSD implementation Attachment 237870 [details] pushed as 32e29c7 - media-keys: Adjust get_image_name_for_volume() to return a string Attachment 237873 [details] pushed as 50e3b5b - media-keys: Use the shell's ShowOSD() method for OSDs (In reply to comment #16) > @@ +1114,3 @@ > + gvc_mixer_ui_device_get_description (device), vol); > + } else { > + show_osd (manager, icon, NULL, vol); > > Indentation. Ugh, sorry - I was paying really close attention to coding style, but nowadays I'm almost exclusively writing GNU-style ...