GNOME Bugzilla – Bug 744610
Nicer missing codec installation experience
Last modified: 2015-02-20 16:39:48 UTC
The patches here implement the missing bits on the Totem side to get closer to the mockup in https://raw.githubusercontent.com/gnome-design-team/gnome-mockups/master/software/version2/wire-codec-install.png
Created attachment 296958 [details] [review] missing plugins: Pass in Totem's desktop file ID It's passed down to the PK session service implemention in gnome-software, which needs to know the ID of the application requesting codec installation.
Created attachment 296959 [details] [review] missing plugins: Break out a function
Created attachment 296960 [details] [review] missing plugins: Ask the user before spawning a plugin search Previously, the PackageKit session service implementation would use Totem's window xid to attatch it's own dialogs. This however no longer works in Wayland world. Instead of attaching a dialog to Totem's window, gnome-software by default now pops up a Shell notification asking for confirmation before launching the codec search. To avoid the notification and make the user experience a bit nicer, this commit implements a confirmation dialog within Totem and passes down necessary flags to supress the Shell notification in that case.
Created attachment 296961 [details] [review] missing plugins: Don't hard depend on too new gst-plugins-base Check if we have the required functions before using them, to avoid hard depending on gst-plugins-base git master.
FWIW, you could also check for #if GST_PLUGINS_BASE_VERSION_MAJOR == 1 && GST_PLUGINS_BASE_VERSION_MINOR >= 5 in the code instead of doing the configure check (although the configure check is probably more correct, and this version check won't catch people with old post-1.4 git versions).
Review of attachment 296958 [details] [review]: That's fine.
Review of attachment 296959 [details] [review]: As long as there are no other changes in the patch, fine.
Review of attachment 296960 [details] [review]: > Totem's window xid to attatch it's own dialogs Attach. Its. > Instead of attaching a dialog to Totem's window, gnome-software by > default now pops up a Shell notification asking for confirmation before > launching the codec search. Huh, what? Why would you spawn a notification instead of showing a dialogue on top of gnome-software's window? ::: src/backend/bacon-video-widget-gst-missing-plugins.c @@ +315,3 @@ + if (len == 2) { + /* TRANSLATORS: separator for a list of items */ + return g_strjoinv (_(" and "), items); This is awful. I really don't want to see that in Totem. @@ +401,3 @@ + /* if gnome-software exists, show a gnome-software specific + * dialog, asking for confirmation before plugin installation */ + if (does_gnome_software_exist ()) This code doesn't check if the GStreamer installation helper *is* gnome-software, just if gnome-software is installed. You probably don't want to do that.
Review of attachment 296961 [details] [review]: Just bump the dependencies when you've landed the patches, and the necessary GStreamer packages have been released instead.
(In reply to Tim-Philipp Müller from comment #5) > FWIW, you could also check for #if GST_PLUGINS_BASE_VERSION_MAJOR == 1 && > GST_PLUGINS_BASE_VERSION_MINOR >= 5 in the code instead of doing the > configure check (although the configure check is probably more correct, and > this version check won't catch people with old post-1.4 git versions). Yep, that would definitely nicer. I forgot to ask earlier: what's the release schedule for gstreamer 1.6, it's not planned to be released with GNOME 3.16, is it? If it's planned for this spring, then I'd say it definitely makes sense to bump the minimum required version in Totem and just hard depend on the new gstreamer.
Thanks for the quick review, Bastien! (In reply to Bastien Nocera from comment #8) > ::: src/backend/bacon-video-widget-gst-missing-plugins.c > @@ +315,3 @@ > + if (len == 2) { > + /* TRANSLATORS: separator for a list of items */ > + return g_strjoinv (_(" and "), items); > > This is awful. I really don't want to see that in Totem. Fair enough. What should I do here, just concatenate the codec names with ', ' without marking them translatable? > @@ +401,3 @@ > + /* if gnome-software exists, show a gnome-software specific > + * dialog, asking for confirmation before plugin installation */ > + if (does_gnome_software_exist ()) > > This code doesn't check if the GStreamer installation helper *is* > gnome-software, just if gnome-software is installed. You probably don't want > to do that. Yes, but I don't think we have a good way right now to do a better check. I wonder if we should maybe have a property on the org.freedesktop.PackageKit.Modify2 interface that says what's the program implementing the interface. Hard to do a check like that without it. Hughsie, what do you think?
(In reply to Kalev Lember from comment #11) > Thanks for the quick review, Bastien! > > > (In reply to Bastien Nocera from comment #8) > > ::: src/backend/bacon-video-widget-gst-missing-plugins.c > > @@ +315,3 @@ > > + if (len == 2) { > > + /* TRANSLATORS: separator for a list of items */ > > + return g_strjoinv (_(" and "), items); > > > > This is awful. I really don't want to see that in Totem. > > Fair enough. What should I do here, just concatenate the codec names with ', > ' without marking them translatable? "The following codecs are missing: foo, bar, baz" > > @@ +401,3 @@ > > + /* if gnome-software exists, show a gnome-software specific > > + * dialog, asking for confirmation before plugin installation */ > > + if (does_gnome_software_exist ()) > > > > This code doesn't check if the GStreamer installation helper *is* > > gnome-software, just if gnome-software is installed. You probably don't want > > to do that. > > Yes, but I don't think we have a good way right now to do a better check. I > wonder if we should maybe have a property on the > org.freedesktop.PackageKit.Modify2 interface that says what's the program > implementing the interface. Hard to do a check like that without it. > > Hughsie, what do you think? You can check what the helper is: $ ls -l /usr/libexec/gst-install-plugins-helper 0 lrwxrwxrwx. 1 root root 20 Jan 19 19:26 /usr/libexec/gst-install-plugins-helper -> pk-gstreamer-install
> I forgot to ask earlier: what's the release schedule > for gstreamer 1.6, it's not planned to be released > with GNOME 3.16, is it? > > If it's planned for this spring, then I'd say it definitely makes sense to > bump the minimum required version in Totem and just hard depend on the new > gstreamer. Well, a 1.5.1 pre-release is planned "real soon now", and hopefully we'll have 1.6 out in mid-March with 3.16, but there's also a real possibility we might not make it in time. Best to not rely on anything and bump the deps as stuff is released.
OK, here's a new patch, making use of the DisplayName property as discussed on IRC, and getting rid of the translatable " and " and ", " strings. Matching gnome-software commit is https://git.gnome.org/browse/gnome-software/commit/?id=a9fd0d669b784426302f4b213f58a6b12c921f34
Created attachment 297027 [details] [review] missing plugins: Ask the user before spawning a plugin search Previously, the PackageKit session service implementation would use Totem's window xid to attach its own dialogs. This however no longer works in Wayland world. Instead of attaching a dialog to Totem's window, gnome-software by default now pops up a Shell notification asking for confirmation before launching the codec search. This is however just a fallback and the preferred way is for an app to pop up its own dialog / in-app notification before starting the codec installation. This commit implements the confirmation dialog within Totem and passes down necessary flags to supress the Shell notification in that case.
Review of attachment 297027 [details] [review]: ::: src/backend/bacon-video-widget-gst-missing-plugins.c @@ +387,3 @@ + dbus_interface = pk_modify2_proxy_new_for_bus_finish (res, &error); + if (error != NULL) { + g_debug ("Failed to connect to PackageKit Modify2 session service: %s", error->message); No, it should fallback to the "this isn't gnome-software" case. ::: src/backend/org.freedesktop.PackageKit.Modify2.xml @@ +1,1 @@ +<!DOCTYPE node PUBLIC Please remove everything that's not relevant. I wonder whether this is even necessary for a single property... g_dbus_proxy_get_cached_property() after creating the proxy should be enough.
(In reply to Bastien Nocera from comment #16) > Review of attachment 297027 [details] [review] [review]: > > ::: src/backend/bacon-video-widget-gst-missing-plugins.c > @@ +387,3 @@ > + dbus_interface = pk_modify2_proxy_new_for_bus_finish (res, &error); > + if (error != NULL) { > + g_debug ("Failed to connect to PackageKit Modify2 session service: %s", > error->message); > > No, it should fallback to the "this isn't gnome-software" case. Right. It actually already fell back to the "this isn't gnome-software" case here since display_name would be NULL down below, but it might not have been obvious when reading the code. I've updated it to be more explicit here. > ::: src/backend/org.freedesktop.PackageKit.Modify2.xml > @@ +1,1 @@ > +<!DOCTYPE node PUBLIC > > Please remove everything that's not relevant. I wonder whether this is even > necessary for a single property... > > g_dbus_proxy_get_cached_property() after creating the proxy should be enough. OK, I got rid of the gdbus-generated code and using raw gdbus now. Hope it's simpler now.
Created attachment 297294 [details] [review] missing plugins: Ask the user before spawning a plugin search Previously, the PackageKit session service implementation would use Totem's window xid to attach its own dialogs. This however no longer works in Wayland world. Instead of attaching a dialog to Totem's window, gnome-software by default now pops up a Shell notification asking for confirmation before launching the codec search. This is however just a fallback and the preferred way is for an app to pop up its own dialog / in-app notification before starting the codec installation. This commit implements the confirmation dialog within Totem and passes down necessary flags to supress the Shell notification in that case.
Review of attachment 297294 [details] [review]: ::: src/backend/bacon-video-widget-gst-missing-plugins.c @@ +36,3 @@ +#include <gio/gdesktopappinfo.h> +#include <glib/gi18n-lib.h> glib/gi18n.h not the lib variant. @@ +381,3 @@ + TotemCodecInstallContext *ctx = (TotemCodecInstallContext *) user_data; + GDBusProxy *packagekit_proxy; + GVariant *property; This should be init'ed to NULL, otherwise you might get into trouble at the "out" label. @@ +389,3 @@ + if (property != NULL) { + display_name = g_variant_get_string (property, NULL); + if (display_name != NULL) { You forgot to check what the DisplayName actually is.
Created attachment 297340 [details] [review] missing plugins: Ask the user before spawning a plugin search Previously, the PackageKit session service implementation would use Totem's window xid to attach its own dialogs. This however no longer works in Wayland world. Instead of attaching a dialog to Totem's window, gnome-software by default now pops up a Shell notification asking for confirmation before launching the codec search. This is however just a fallback and the preferred way is for an app to pop up its own dialog / in-app notification before starting the codec installation. This commit implements the confirmation dialog within Totem and passes down necessary flags to supress the Shell notification in that case.
Created attachment 297395 [details] [review] missing plugins: Ask the user before spawning a plugin search Previously, the PackageKit session service implementation would use Totem's window xid to attach its own dialogs. This however no longer works in Wayland world. Instead of attaching a dialog to Totem's window, gnome-software by default now pops up a Shell notification asking for confirmation before launching the codec search. This is however just a fallback and the preferred way is for an app to pop up its own dialog / in-app notification before starting the codec installation. This commit implements the confirmation dialog within Totem and passes down necessary flags to supress the Shell notification in that case.
Review of attachment 297395 [details] [review]: ::: src/backend/bacon-video-widget.c @@ +641,3 @@ G_CALLBACK (leave_notify_cb), bvw); + bvw->priv->missing_plugins_cancellable = g_cancellable_new (); Do we need to cancel and unref a possibly existing missing_plugins_cancellable here? Something like, if (bvw->priv->missing_plugins_cancellable != NULL) { g_cancellable_cancel (bvw->priv->missing_plugins_cancellable); g_object_unref (bvw->priv->missing_plugins_cancellable); } bvw->priv->missing_plugins_cancellable = g_cancellable_new (); @@ +2908,3 @@ GST_DEBUG ("finalizing"); + g_cancellable_cancel (bvw->priv->missing_plugins_cancellable); Does it need a NULL check here, in case the bvw was never realized?
(In reply to Kalev Lember from comment #22) > Review of attachment 297395 [details] [review] [review]: > > ::: src/backend/bacon-video-widget.c > @@ +641,3 @@ > G_CALLBACK (leave_notify_cb), bvw); > > + bvw->priv->missing_plugins_cancellable = g_cancellable_new (); > > Do we need to cancel and unref a possibly existing > missing_plugins_cancellable here? > > Something like, > > if (bvw->priv->missing_plugins_cancellable != NULL) { > g_cancellable_cancel > (bvw->priv->missing_plugins_cancellable); > g_object_unref (bvw->priv->missing_plugins_cancellable); > } > bvw->priv->missing_plugins_cancellable = g_cancellable_new (); I didn't realize that I added that to realize. The opposite should be done in unrealize() instead of finalize() then. > @@ +2908,3 @@ > GST_DEBUG ("finalizing"); > > + g_cancellable_cancel (bvw->priv->missing_plugins_cancellable); > > Does it need a NULL check here, in case the bvw was never realized? As mentioned above, should be done in unrealize() instead.
Review of attachment 297395 [details] [review]: Two more things I noticed while adding version checks: ::: src/backend/bacon-video-widget-gst-missing-plugins.c @@ +57,3 @@ + gchar **details; + BaconVideoWidget *bvw; + GCancellable *cancellable; The new ctx->cancellable isn't used anywhere. @@ +389,3 @@ + if (packagekit_proxy == NULL && + g_error_matches (error, G_IO_ERROR, G_IO_ERROR_CANCELLED)) { + goto out; This leaks the GError on the error path.
Created attachment 297425 [details] [review] missing plugins: Pass in Totem's desktop file ID It's passed down to the PK session service implemention in gnome-software, which needs to know the ID of the application requesting codec installation.
Created attachment 297426 [details] [review] missing plugins: Break out a function
Created attachment 297427 [details] [review] missing plugins: Ask the user before spawning a plugin search Previously, the PackageKit session service implementation would use Totem's window xid to attach its own dialogs. This however no longer works in Wayland world. Instead of attaching a dialog to Totem's window, gnome-software by default now pops up a Shell notification asking for confirmation before launching the codec search. This is however just a fallback and the preferred way is for an app to pop up its own dialog / in-app notification before starting the codec installation. This commit implements the confirmation dialog within Totem and passes down necessary flags to supress the Shell notification in that case.
Created attachment 297428 [details] [review] downstream patch: Reduce gstreamer version requirement We carry backported gstreamer1-plugins-base patches that add the required new symbols.
Review of attachment 297425 [details] [review]: Yes.
Review of attachment 297426 [details] [review]: Yep.
Review of attachment 297427 [details] [review]: ::: src/backend/bacon-video-widget-gst-missing-plugins.c @@ +281,3 @@ +#if GST_CHECK_VERSION (1, 5, 0) + startup_id = make_startup_notification_id (); A bit spaghetti. Can you move all that into make_startup_notification() and rename to set_startup_notification()? @@ +397,3 @@ + GError *error = NULL; + + packagekit_proxy = g_dbus_proxy_new_for_bus_finish (res, &error); Seeing as you always run g_dbus_proxy_new_for_bus(), you'll need to always run _finish() as well. @@ +421,3 @@ +out: + if (error != NULL) + g_error_free (error); g_clear_error(); @@ +423,3 @@ + g_error_free (error); + if (property != NULL) + g_variant_unref (property); g_clear_pointer(); @@ +425,3 @@ + g_variant_unref (property); + if (packagekit_proxy != NULL) + g_object_unref (packagekit_proxy); g_clear_object();
Review of attachment 297428 [details] [review]: Sure for F22. Thanks.
Created attachment 297429 [details] [review] missing plugins: Ask the user before spawning a plugin search Previously, the PackageKit session service implementation would use Totem's window xid to attach its own dialogs. This however no longer works in Wayland world. Instead of attaching a dialog to Totem's window, gnome-software by default now pops up a Shell notification asking for confirmation before launching the codec search. This is however just a fallback and the preferred way is for an app to pop up its own dialog / in-app notification before starting the codec installation. This commit implements the confirmation dialog within Totem and passes down necessary flags to supress the Shell notification in that case.
Review of attachment 297429 [details] [review]: ++
Thanks for the reviews and help, Bastien! Attachment 297425 [details] pushed as 628c254 - missing plugins: Pass in Totem's desktop file ID Attachment 297426 [details] pushed as da91464 - missing plugins: Break out a function Attachment 297429 [details] pushed as dd42ebd - missing plugins: Ask the user before spawning a plugin search