GNOME Bugzilla – Bug 692702
enable-upnp does not do what you think it does
Last modified: 2013-02-14 15:13:35 UTC
The enable-upnp option in Rygel only disables/enables the so-called streaming-only mode in Rygel: https://live.gnome.org/Rygel/StreamingOnly Enabling/disabling Rygel should happen through adding/removing the desktop file to autostart, see http://git.gnome.org/browse/rygel/tree/src/ui/rygel-writable-user-config.vala#n148 The naming in rygel-preferences is slightly misleading, sorry.
Created attachment 234918 [details] [review] sharing: add functions to enable and disable the auto-starting of Rygel
Review of attachment 234918 [details] [review]: ::: panels/sharing/cc-media-sharing.c @@ +57,3 @@ + + /* find the rygel desktop file */ + system_data_dirs = g_get_system_data_dirs (); app = g_desktop_app_info_new(); filename = g_desktop_app_info_get_filename (app); @@ +75,3 @@ + /* create a symbolic link in the autostart directory */ + destination = g_build_filename (g_get_user_config_dir (), "autostart", + "rygel.desktop", NULL); Use a constant for the rygel.desktop name. @@ +118,3 @@ + + /* stop rygel */ + g_dbus_proxy_new_for_bus (G_BUS_TYPE_SESSION, No new proxy for a single call. Use g_dbus_connection_call().
Created attachment 235140 [details] [review] sharing: add functions to enable and disable the auto-starting of Rygel
Review of attachment 235140 [details] [review]: ::: panels/sharing/cc-media-sharing.c @@ +62,3 @@ + return; + + source = g_desktop_app_info_get_filename (info); unref info here, it's not used afterwards. @@ +74,3 @@ + RYGEL_DESKTOP_ID, NULL); + + file = g_file_new_for_path (destination); g_free (destination); here, it's not used afterwards @@ +82,3 @@ + + /* start rygel */ + g_bus_watch_name (G_BUS_TYPE_SESSION, BUS_NAME, What's the use? gnome-session should be starting it. Also, we'll keep on watching that name until gnome-control-center exits. If you want to start it, g_app_info_launch(info) would do just fine. @@ +112,3 @@ + RYGEL_DESKTOP_ID, NULL); + + if (g_file_test (path, G_FILE_TEST_IS_SYMLINK)) In which case wouldn't it be a symlink? Does it matter if it's not a symlink? @@ +118,3 @@ + + /* stop rygel */ + g_bus_get (G_BUS_TYPE_SESSION, NULL, cc_media_sharing_bus_ready_callback, Missing passing a GCancellable here.
(In reply to comment #4) > Review of attachment 235140 [details] [review]: > > @@ +74,3 @@ > + RYGEL_DESKTOP_ID, NULL); > + > + file = g_file_new_for_path (destination); > > g_free (destination); here, it's not used afterwards g_free (destination) is called a few lines later, but can be moved up if you prefer. > > @@ +82,3 @@ > + > + /* start rygel */ > + g_bus_watch_name (G_BUS_TYPE_SESSION, BUS_NAME, > > What's the use? gnome-session should be starting it. > Also, we'll keep on watching that name until gnome-control-center exits. > > If you want to start it, g_app_info_launch(info) would do just fine. The purpose is to start Rygel as soon as the media sharing option is enabled. > > @@ +112,3 @@ > + RYGEL_DESKTOP_ID, NULL); > + > + if (g_file_test (path, G_FILE_TEST_IS_SYMLINK)) > > In which case wouldn't it be a symlink? Does it matter if it's not a symlink? This was to ensure the enable and disable operations were symmetrical. In the case where someone has added their own custom rygel.desktop to startup, this should prevent it from being removed. If this isn't a concern, the test can be removed. > > @@ +118,3 @@ > + > + /* stop rygel */ > + g_bus_get (G_BUS_TYPE_SESSION, NULL, cc_media_sharing_bus_ready_callback, > > Missing passing a GCancellable here. What situation should it be cancelled? The callback doesn't rely on any UI code and Rygel should definitely be stopped if the media sharing option is disabled.
(In reply to comment #4) > Review of attachment 235140 [details] [review]: > > ::: panels/sharing/cc-media-sharing.c > @@ +62,3 @@ > + return; > + > + source = g_desktop_app_info_get_filename (info); > > unref info here, it's not used afterwards. info is not used, but source is, and source belongs to the info object (it is const gchar*).
(In reply to comment #5) > (In reply to comment #4) > > Review of attachment 235140 [details] [review] [details]: > > > > > @@ +74,3 @@ > > + RYGEL_DESKTOP_ID, NULL); > > + > > + file = g_file_new_for_path (destination); > > > > g_free (destination); here, it's not used afterwards > > g_free (destination) is called a few lines later, but can be moved up if you > prefer. freed as soon as unused, especially in the case of variable being assigned and freed in lockstep. > > @@ +82,3 @@ > > + > > + /* start rygel */ > > + g_bus_watch_name (G_BUS_TYPE_SESSION, BUS_NAME, > > > > What's the use? gnome-session should be starting it. > > Also, we'll keep on watching that name until gnome-control-center exits. > > > > If you want to start it, g_app_info_launch(info) would do just fine. > > The purpose is to start Rygel as soon as the media sharing option is enabled. g_app_info_launch() should do then. > > @@ +112,3 @@ > > + RYGEL_DESKTOP_ID, NULL); > > + > > + if (g_file_test (path, G_FILE_TEST_IS_SYMLINK)) > > > > In which case wouldn't it be a symlink? Does it matter if it's not a symlink? > > This was to ensure the enable and disable operations were symmetrical. In the > case where someone has added their own custom rygel.desktop to startup, this > should prevent it from being removed. If this isn't a concern, the test can be > removed. Remove the test please. > > @@ +118,3 @@ > > + > > + /* stop rygel */ > > + g_bus_get (G_BUS_TYPE_SESSION, NULL, cc_media_sharing_bus_ready_callback, > > > > Missing passing a GCancellable here. > > What situation should it be cancelled? The callback doesn't rely on any UI code > and Rygel should definitely be stopped if the media sharing option is disabled. It should be cancelled when the panel exits, really, or when cc_media_sharing_enable_autostart() is called. But I guess that'll do for now. (In reply to comment #6) > (In reply to comment #4) > > Review of attachment 235140 [details] [review] [details]: > > > > ::: panels/sharing/cc-media-sharing.c > > @@ +62,3 @@ > > + return; > > + > > + source = g_desktop_app_info_get_filename (info); > > > > unref info here, it's not used afterwards. > > info is not used, but source is, and source belongs to the info object (it is > const gchar*). Right. Make sure that info is unref'ed as soon as source and info are unused then.
Patch pushed with changes suggested.