GNOME Bugzilla – Bug 730890
Add a provider for locally shared media
Last modified: 2014-06-24 15:08:05 UTC
This is an enhancement to incorporate support (add a provider) for adding locally shared media devices like DLNA Media servers in GOA.
Created attachment 277481 [details] [review] stub code for media server provider
maintaining a separate branch at : https://github.com/pranavk/gnome-online-accounts/tree/mediaprovider
Created attachment 277752 [details] [review] added dbus interface for mediaserver provider
Created attachment 277753 [details] [review] updated docs for MediaServer interface
Created attachment 277754 [details] [review] added xml files for calling dleyna-server's DBus API
Review of attachment 277752 [details] [review]: Thanks for the patch, Pranav. Mostly looks good. Parts of the documentation patch should actually be a part of this one. ::: data/dbus-interfaces.xml @@ +705,3 @@ + An account object implements this interface if it provides + media server like capabilities. + --> Please add a @since: 3.14 @@ +721,3 @@ + Human understandable name of the media server. + --> + <property name="friendly_name" type="s" access="read"/> Do we really need friendly_name? I don't think we do because Udn should be enough to identify the device. Given the UDN applications can query the friendly name.
Review of attachment 277753 [details] [review]: Parts of this patch should be split and merged into others. See below: ::: doc/goa-docs.xml @@ +127,3 @@ <xi:include href="../src/goa/goa-generated-doc-org.gnome.OnlineAccounts.Exchange.xml"/> <xi:include href="../src/goa/goa-generated-doc-org.gnome.OnlineAccounts.Ticketing.xml"/> + <xi:include href="../src/goa/goa-generated-doc-org.gnome.OnlineAccounts.MediaServer.xml"/> This should be part of the patch that adds the DBus interface. @@ +155,3 @@ <xi:include href="xml/GoaExchange.xml"/> <xi:include href="xml/GoaTicketing.xml"/> + <xi:include href="xml/GoaMediaServer.xml"/> Ditto. @@ +173,3 @@ <xi:include href="xml/goaexchangeprovider.xml"/> <xi:include href="xml/goagoogleprovider.xml"/> + <xi:include href="xml/goamediaserverprovider.xml"/> This only makes sense when we have the provider, and so should be part of the patch that adds the provider. ::: doc/goa-sections.txt @@ +550,3 @@ +GOA_TYPE_MEDIA_SERVER_PROVIDER +goa_media_server_provider_get_type +</SECTION> This only makes sense when we have a provider. Hence this should be part of the patch that adds the provider. @@ +1108,3 @@ +goa_media_server_skeleton_get_type +</SECTION> + This should be part of the patch that added the DBus interface. ::: doc/goa.types @@ +33,3 @@ goa_google_provider_get_type goa_facebook_provider_get_type +goa_media_server_provider_get_type Should be part of the patch that adds the provider.
Created attachment 277788 [details] [review] mediaserverprovider main implementation files
Review of attachment 277754 [details] [review]: Thanks for the patch, Pranav. A few comments below: ::: src/goabackend/goa-dleyna-server-device.xml @@ +21,3 @@ + Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA + 02110-1301, USA. +--> This header is wrong. Also, I prefer goadleynaservermediadevice.xml for consistency with the rest of the files. ::: src/goabackend/goa-dleyna-server.xml @@ +21,3 @@ + Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA + 02110-1301, USA. +--> This header is wrong. @@ +24,3 @@ + +<node> + <interface name="com.intel.dLeynaServer.Manager"> Shouldn't this file be goa-dleyna-server-manager.xml or goadleynaservermanager.xml? I prefer not to use the hyphens for consistency with the rest of the files.
Review of attachment 277788 [details] [review]: Mostly looks good. Some comments below: ::: configure.ac @@ +313,3 @@ + [Enable DLNA Media Server provider])], + [], + [enable_media_server=yes]) Lets keep 'enable_media_server=no' till we have finished and tested the feature. ::: src/goabackend/Makefile.am @@ +93,3 @@ + goamediaserverprovider.h goamediaserverprovider.c \ + goa-dlna-server-manager.h goa-dlna-server-manager.c \ + goautils.h goautils.c \ The alignment broke. ::: src/goabackend/goa-dlna-server-manager.c @@ +16,3 @@ + * Public License along with this library; if not, write to the + * Free Software Foundation, Inc., 59 Temple Place, Suite 330, + * Boston, MA 02111-1307, USA. Please use the same header as the rest of the files. @@ +18,3 @@ + * Boston, MA 02111-1307, USA. + * + * Author: Pranav Kant <pranav913@gmail.com> No need for Author. @@ +111,3 @@ +goa_dlna_server_manager_server_lost_cb (GoaDlnaServerManager *self, + const gchar *object_path, + gpointer *data) Broken alignment. @@ +132,3 @@ +goa_dlna_server_manager_proxy_get_servers_cb (GObject *source_object, + GAsyncResult *res, + gpointer user_data) Broken alignment. ::: src/goabackend/goa-dlna-server-manager.h @@ +4,3 @@ + * + * This program is free software; you can redistribute it and/or + * modify it under the terms of the GNU General Public License Wrong license. Please use the same header as the rest of the files. ::: src/goabackend/goamediaserverprovider.c @@ +16,3 @@ + * Public License along with this library; if not, write to the + * Free Software Foundation, Inc., 59 Temple Place, Suite 330, + * Boston, MA 02111-1307, USA. Please use the same header as the rest of the files. @@ +280,3 @@ + g_variant_builder_init (&details, G_VARIANT_TYPE ("a{ss}")); + g_variant_builder_add (&details, "PhotosEnabled", "true"); + I noticed that you don't check for duplicate accounts with goa_utils_check_duplicate. Are you skipping servers that are already added? @@ +418,3 @@ + g_object_set (G_OBJECT (mediaserver), + "friendly_name", friendly_name, + NULL); Do we really need the friendly_name? @@ +505,3 @@ + grid, + right); + */ You forgot to remove this. :) @@ +587,3 @@ + provider_class->build_object = build_object; + provider_class->show_account = show_account; + provider_class->ensure_credentials_sync = ensure_credentials_sync; The last '=' is not aligned anymore. ::: src/goabackend/goamediaserverprovider.h @@ +16,3 @@ + * Public License along with this library; if not, write to the + * Free Software Foundation, Inc., 59 Temple Place, Suite 330, + * Boston, MA 02111-1307, USA. Please use the same header as the rest of the files. ::: src/goabackend/goaprovider.c @@ +803,3 @@ +#ifdef GOA_MEDIA_SERVER_ENABLED + type = GOA_TYPE_MEDIA_SERVER_PROVIDER; +#endif This will need some adjustment for master.
Created attachment 277802 [details] [review] added dbus interface for mediaserver provider
Created attachment 277803 [details] [review] added xml files for calling dleyna-server's DBus API
Created attachment 277805 [details] [review] mediaserverprovider main implementation files
Created attachment 277822 [details] [review] mediaserverprovider main implementation files got rid of a segmentation fault in the earlier version of this patch.
Review of attachment 277802 [details] [review]: Almost there. ::: data/dbus-interfaces.xml @@ +708,3 @@ + <interface name="org.gnome.OnlineAccounts.MediaServer"> + <!-- DlnaSupported: + @since: 3.14 Not just this property, but the whole interface should have the @since. eg., see the Ticketing interface.
Review of attachment 277803 [details] [review]: Almost there. ::: src/goabackend/Makefile.am @@ +62,3 @@ + +nodist_libgoa_backend_1_0_la_SOURCES = \ + $(libgoa_backend_1_0_la_built_sources) \ The backslash is not aligned. ::: src/goabackend/goadleynaservermanager.xml @@ +18,3 @@ + You should have received a copy of the GNU Lesser General + Public License along with this library; if not, see <http://www.gnu.org/licenses/>. +--> Some trailing whitespaces hidden in there. ::: src/goabackend/goadleynaservermediadevice.xml @@ +18,3 @@ + You should have received a copy of the GNU Lesser General + Public License along with this library; if not, see <http://www.gnu.org/licenses/>. +--> Ditto.
Created attachment 277868 [details] [review] Generate stubs for the dleyna-server D-Bus API I removed the trailing whitespaces, fixed the alignment and tweaked the commit message a bit.
Review of attachment 277822 [details] [review]: You have made good progress, Pranav. A few superficial comments before I try out the code. ::: src/goabackend/Makefile.am @@ +123,1 @@ $< Wrong rebase alert. :) ::: src/goabackend/goamediaserverprovider.c @@ +5,3 @@ + * + * This program is free software; you can redistribute it and/or + * modify it under the terms of the GNU General Public License Wrong license. @@ +18,3 @@ + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA + * 02110-1301, USA. + */ Not the same header as the rest of the files. ::: src/goabackend/goamediaserverprovider.h @@ +16,3 @@ + * Public License along with this library; if not, write to the + * Free Software Foundation, Inc., 59 Temple Place, Suite 330, + * Boston, MA 02111-1307, USA. Not the same header as the rest of the files.
Created attachment 277869 [details] [review] added dbus interface for mediaserver provider
Created attachment 277871 [details] [review] mediaserverprovider main implementation files
Review of attachment 277822 [details] [review]: You have made good progress, Pranav. A few comments before I try out the code. ::: src/goabackend/Makefile.am @@ +123,1 @@ $< Wrong rebase alert. :) ::: src/goabackend/goadlnaservermanager.c @@ +28,3 @@ + DleynaServerManager *proxy; + GHashTable *servers; + GError *error; We won't need the GError if we drop _is_available. @@ +85,3 @@ + object_path); + g_hash_table_insert (priv->servers, (gpointer) object_path, server); + g_signal_emit (self, signals[SERVER_FOUND], 0, server); Unref self in out. @@ +100,3 @@ + NULL, /* GCancellable */ + goa_dlna_server_manager_server_new_cb, + self); Same thing (see below) about taking a ref on self before passing it as user_data to a async method. @@ +147,3 @@ + goa_dlna_server_manager_server_found_cb (self, *path, NULL); + + g_strfreev (object_paths); Unref self in out. @@ +164,3 @@ + g_warning ("Unable to connect to the dLeynaServer.Manager DBus object: %s", error->message); + g_propagate_error (&priv->error, error); + return; goto out; instead of return; @@ +175,3 @@ + + dleyna_server_manager_call_get_servers (priv->proxy, NULL, + goa_dlna_server_manager_proxy_get_servers_cb, self); Same thing (see below) about taking a ref on self before passing it as user_data to a async method. We also need a: out: g_object_unref (self); @@ +189,3 @@ + G_OBJECT_CLASS (goa_dlna_server_manager_parent_class)->constructor (type, + n_construct_params, + construct_params); Broken alignment. @@ +211,3 @@ + NULL, /* GCancellable */ + goa_dlna_server_manager_proxy_new_cb, + self); I think it is better to take a ref on 'self' and unref it in the callback. Otherwise it might crash in this case: manager = goa_dlna_server_manager_dup_singleton (); g_object_unref (manager); Because the callback will be invoked after the unref and at that point the manager object (ie. self) would have been destroyed. If the code in gnome-photos is not doing that, then we should double check it. So instead of just passing 'self', pass 'g_object_ref (self)' as user_data. @@ +266,3 @@ + + return self->priv->error == NULL; +} We can drop this because I think we can reasonably make the media server provider have a hard dependency on our DLNA stack. Those who don't want that can turn the provider off. In any case we are not using this method at the moment. ::: src/goabackend/goamediaserverprovider.c @@ +5,3 @@ + * + * This program is free software; you can redistribute it and/or + * modify it under the terms of the GNU General Public License Wrong license. @@ +18,3 @@ + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA + * 02110-1301, USA. + */ Not the same header as the rest of the files. @@ +70,3 @@ + g_io_extension_point_implement (GOA_PROVIDER_EXTENSION_POINT_NAME, + g_define_type_id, + "media_server", Use GOA_MEDIA_SERVER_NAME which you defined in configure.ac. @@ +78,3 @@ +get_provider_type (GoaProvider *provider) +{ + return "media_server"; Ditto. ::: src/goabackend/goamediaserverprovider.h @@ +16,3 @@ + * Public License along with this library; if not, write to the + * Free Software Foundation, Inc., 59 Temple Place, Suite 330, + * Boston, MA 02111-1307, USA. Not the same header as the rest of the files.
Review of attachment 277869 [details] [review]: ::: data/dbus-interfaces.xml @@ +708,3 @@ + <interface name="org.gnome.OnlineAccounts.MediaServer"> + <!-- DlnaSupported: + @since: 3.14 You don't need it on each member. Having it on the interface is enough. eg., see Ticketing. @@ +714,3 @@ + <property name="DlnaSupported" type="b" access="read"/> + <!-- Udn: + @since: 3.14 Ditto.
Review of attachment 277871 [details] [review]: ::: src/goabackend/goamediaserverprovider.c @@ +37,3 @@ +struct _GoaMediaServerProviderPrivate +{ + GoaDlnaServerManager *media_server; This is not a media server. It is a manager, so we should name it accordingly. In any case, do we really need it to be instance wide variable? In the current code, it is only used in add_account so we can just use a local variable, unless the code changes. @@ +269,3 @@ + gtk_grid_set_row_spacing (GTK_GRID (grid), 12); + gtk_container_add (GTK_CONTAINER (grid), listbox); + gtk_container_add (GTK_CONTAINER (vbox), grid); You should use goa_utils_set_dialog_title to change the title of the dialog. Also, can you please use the same structure as the other providers (owncloud, exchange, imapsmtp) and create the UI in a function named create_account_details_ui ? @@ +307,3 @@ + provider_type, + (GoaPeekInterfaceFunc) goa_object_peek_account, + &data.error)) I am wondering if we should hide duplicate servers from the list instead of checking afterwards. @@ +490,3 @@ + { + (*error)->domain = GOA_ERROR; + (*error)->code = GOA_ERROR_FAILED; It should be GOA_ERROR_NOT_AUTHORIZED. @@ +543,3 @@ + + g_clear_object (&priv->media_server); + Trailing whitespaces. @@ +544,3 @@ + g_clear_object (&priv->media_server); + + (G_OBJECT_CLASS (goa_media_server_provider_parent_class)->finalize) (object); Should be dispose, not finalize.
Created attachment 277893 [details] [review] added dbus interface for mediaserver provider
Created attachment 277894 [details] [review] mediaserverprovider main implementation files
Review of attachment 277893 [details] [review]: Committed after tweaking the commit message a little bit. Thanks for the patch, Pranav.
Created attachment 277972 [details] [review] mediaserverprovider main implementation files minor tweaks, rechecked ref/unref pairs, little UI enhancements.
Created attachment 278061 [details] [review] mediaserverprovider main implementation files some UI enhancements
Review of attachment 278061 [details] [review]: Thanks for the fixes, Pranav. The UI looks better than before. Two big things that I noticed are that we are not showing icons for the media servers, and the list box is not framed. ::: src/goabackend/goadlnaservermanager.c @@ +59,3 @@ + if (error != NULL) + { + g_warning ("Unable to load server object: %s", error->message); The GError is being leaked. @@ +108,3 @@ + dleyna_server_media_device_get_udn (server), + object_path); + g_signal_emit (self, signals[SERVER_LOST], 0, server); The server is being leaked. g_hash_table_steal means that the entry is removed from the hash table without invoking the destroy functions. That is OK in this case because we want to remove it from the hash table before we emit the signal so that goa_dlna_server_manager_dup_servers does not return it, but we don't want to lose the object before emitting the signal. @@ +127,3 @@ + if (error != NULL) + { + g_warning ("Unable to fetch the list of available servers: %s", error->message); The GError is being leaked. @@ +132,3 @@ + + for (path = object_paths; *path != NULL; path++) + goa_dlna_server_manager_server_found_cb (g_object_ref (self), *path, NULL); I think this 'g_object_ref (self)' is spurious because the ref is never released. Moreover it is not needed because goa_dlna_server_manager_server_found_cb is not a async operation itself. Minor nitpick: I find the object_paths[i] notation easier to read. @@ +160,3 @@ + { + g_warning ("Unable to connect to the dLeynaServer.Manager DBus object: %s", + error->message); The GError is being leaked. @@ +220,3 @@ + goa_dlna_server_manager_proxy_new_cb, + g_object_ref (self)); + priv->servers = g_hash_table_new_full (g_str_hash, g_str_equal, NULL, g_object_unref); The hash table is never unreffed. ::: src/goabackend/goadlnaservermanager.h @@ +63,3 @@ +GType goa_dlna_server_manager_get_type (void) G_GNUC_CONST; + +GoaDlnaServerManager *goa_dlna_server_manager_dup_singleton (void); Broken alignment. ::: src/goabackend/goamediaserverprovider.c @@ +228,3 @@ + GtkDialog *dialog, + GtkBox *vbox, + gboolean new_account, We are always creating new_accounts here, so we can drop this argument. @@ +233,3 @@ + goa_utils_set_dialog_title (provider, dialog, new_account); + + GtkWidget *label = gtk_label_new (_("Personal content can be added\nto your applications through a media server account.")); Please don't manually format strings with '\n's. Instead set the appropriate size properties of the widget and let GTK+ do it for you. @@ +237,3 @@ + GtkWidget *window = gtk_scrolled_window_new (NULL, NULL); + GtkWidget *grid = gtk_grid_new (); + Variables should be defined at the top of the block and use separate lines for function calls. @@ +317,3 @@ + + GoaMediaServerProvider *self = GOA_MEDIA_SERVER_PROVIDER (provider); + GoaMediaServerProviderPrivate *priv = self->priv; Put this at the top of the block. @@ +324,3 @@ + data.loop = g_main_loop_new (NULL, FALSE); + data.dialog = dialog; + data.error = NULL; Shouldn't we be hooking up a GCancellable so that the user can bail out of the UI while it is busy doing something? @@ +333,3 @@ + create_account_details_ui (provider, dialog, vbox, TRUE, &data); + + goa_media_server_provider_fill_listbox (data.listbox, priv->dlna_mngr); Why can't this and some of the signal connections below be merged into create_account_details_ui ? @@ +463,3 @@ + GoaAccount *account; + GoaMediaServer *mediaserver; + GoaPhotos *photos; These two should be initialized to NULL. @@ +511,3 @@ + g_object_set (G_OBJECT (mediaserver), + "udn", udn, + NULL); You can club them into one g_object_set call. @@ +529,3 @@ + +out: + g_clear_object (&mediaserver); The GoaPhotos instance is not unreffed. @@ +559,3 @@ + + if (credentials == NULL) + goto out; Any reason for keeping these two if branches separate instead of clubbing them together like in the other files? @@ +609,3 @@ + g_clear_object (&priv->dlna_mngr); + + (G_OBJECT_CLASS (goa_media_server_provider_parent_class)->dispose) (object); ^ this layer of parentheses are not required.
Created attachment 278263 [details] [review] MediaServer: added main implementation for mediaserver provider I fixed some of the smaller issues in the previous patch.
Created attachment 278360 [details] [review] mediaserverprovider main implementation files fixed all left issues
Review of attachment 278360 [details] [review]: ::: src/goabackend/goamediaserverprovider.c @@ +37,3 @@ +struct _GoaMediaServerProviderPrivate +{ + GoaDlnaServerManager *dlna_mngr; I forgot to tell you before, but we don't need a separate private structure because this is what you can call a 'final' class. No one is expected to create subclasses of it. So you can put dlna_mngr in the GoaMediaServerProvider structure itself. @@ +259,3 @@ + + row = gtk_list_box_row_new (); + grid = gtk_grid_new (); No need for this grid. The label can be directly added to the row. @@ +274,3 @@ + + gtk_widget_set_margin_top (grid, 15); + gtk_widget_set_margin_bottom (grid, 15); From where did you pick the value 15? It is better to use the values from one of the existing panels. eg., Privacy or Sharing. I can see that Privacy uses 12 for the top and bottom margins, and 20 on the sides. @@ +297,3 @@ + + list = gtk_container_get_children (GTK_CONTAINER (listbox)); + const gchar *udn = dleyna_server_media_device_get_udn (server); Non-C89-ism. @@ +302,3 @@ + { + tmp_server = g_object_get_data (G_OBJECT (list->data), "server"); + const gchar *tmp_udn = dleyna_server_media_device_get_udn (tmp_server); Ditto. @@ +326,3 @@ + DleynaServerMediaDevice *device = g_object_get_data (G_OBJECT (listboxrow), "server"); + const gchar *udn = dleyna_server_media_device_get_udn (DLEYNA_SERVER_MEDIA_DEVICE (device)); + const gchar *friendly_name = dleyna_server_media_device_get_friendly_name (DLEYNA_SERVER_MEDIA_DEVICE (device)); Please put the function calls in separate statements, and why are you typecasting twice? @@ +396,3 @@ + + label = gtk_label_new (_("Personal content can be added " \ + "to your applications through a media server account.")); Better to put the entire string literal on a single line. Helps with grepping and the line isn't that long anyway. @@ +397,3 @@ + label = gtk_label_new (_("Personal content can be added " \ + "to your applications through a media server account.")); + gtk_label_set_justify (GTK_LABEL (label), GTK_JUSTIFY_CENTER); Should be LEFT, not CENTER. @@ +404,3 @@ + label1 = gtk_label_new (NULL); + gtk_widget_set_hexpand (label1, TRUE); + gtk_label_set_justify (GTK_LABEL (label1), GTK_JUSTIFY_CENTER); Ditto. @@ +405,3 @@ + gtk_widget_set_hexpand (label1, TRUE); + gtk_label_set_justify (GTK_LABEL (label1), GTK_JUSTIFY_CENTER); + gtk_label_set_markup (GTK_LABEL (label1), "<b>Available Media Servers</b>"); This string has not been marked for translation. You need to use something like g_strconcat or g_strdup_printf to avoid putting the Pango markup in the translatable string. @@ +409,3 @@ + frame = gtk_frame_new (NULL); + gtk_widget_set_vexpand (frame, TRUE); + gtk_widget_set_hexpand (frame, TRUE); No need to create a frame manually. Just set the shadow type of the scrolled window to GTK_SHADOW_IN. @@ +423,3 @@ + gtk_widget_set_hexpand (window, TRUE); + + placeholder = gtk_box_new (GTK_ORIENTATION_HORIZONTAL, 0); You don't need a separate GtkBox. The label can be directly used as the placeholder. @@ +424,3 @@ + + placeholder = gtk_box_new (GTK_ORIENTATION_HORIZONTAL, 0); + label = gtk_label_new (_("No available Media Servers found")); The capitalisation looks a bit weird. Lets use small m and small s for the moment. @@ +439,3 @@ + "server-found", + G_CALLBACK (goa_media_server_provider_server_found_cb), + data->listbox); Better to use g_signal_connect_object here. The lifetime of the listbox widget and dlna_mngr are not the same. The dlna_mngr can outlive the listbox. If the signal is emitted after the destruction of the widget we might crash. @@ +444,3 @@ + "server-lost", + G_CALLBACK (goa_media_server_provider_server_lost_cb), + data->listbox); Ditto. @@ +496,3 @@ + priv->dlna_mngr = goa_dlna_server_manager_dup_singleton (); + data.dlna_mngr = priv->dlna_mngr; + data.listbox = gtk_list_box_new (); Can't you create the list box in create_account_details_ui? @@ +635,3 @@ +{ + provider->priv = goa_media_server_provider_get_instance_private (provider); + provider->priv->dlna_mngr = goa_dlna_server_manager_dup_singleton (); dlna_mngr is being created in add_account, so we don't need this line.
Created attachment 279119 [details] [review] Add Media Server I went ahead and fixed up the remaining issues in the previous patch. Polished the UI a bit. Lets get this in for 3.13.3.
Created attachment 279120 [details] Screenshot of adding a server
Created attachment 279121 [details] Screenshot without any servers