After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 728912 - Mine photos from DMSes
Mine photos from DMSes
Status: RESOLVED OBSOLETE
Product: gnome-online-miners
Classification: Applications
Component: general
unspecified
Other All
: Normal enhancement
: ---
Assigned To: GNOME Online Miners maintainer(s)
GNOME Online Miners maintainer(s)
Depends on:
Blocks: 728913
 
 
Reported: 2014-04-24 21:36 UTC by Pranav Kant
Modified: 2021-06-05 16:35 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
src: added stub for mediaserver miner (7.53 KB, patch)
2014-06-08 21:21 UTC, Pranav Kant
needs-work Details | Review
MediaServerMiner: Generate stubs for the dleyna-server D-Bus API (11.42 KB, patch)
2014-06-08 21:22 UTC, Pranav Kant
none Details | Review
MediaServerMiner: Added wrappers over dleyna-server D-Bus API (25.35 KB, patch)
2014-06-08 21:22 UTC, Pranav Kant
none Details | Review
MediaServerMiner: Added a library for common tasks. (6.27 KB, patch)
2014-06-08 21:23 UTC, Pranav Kant
none Details | Review
MediaServerMiner: Implemented basic miner functionality (6.32 KB, patch)
2014-06-08 21:23 UTC, Pranav Kant
none Details | Review
check for gio-unix-2.0 in configure (952 bytes, patch)
2014-06-10 21:03 UTC, Pranav Kant
none Details | Review
MediaServerMiner: Generate stubs for the dleyna-server D-Bus API (11.42 KB, patch)
2014-06-12 19:45 UTC, Pranav Kant
none Details | Review
MediaServerMiner: Added wrappers over dleyna-server D-Bus API (25.34 KB, patch)
2014-06-12 19:46 UTC, Pranav Kant
none Details | Review
MediaServerMiner: Generate stubs for the dleyna-server D-Bus API (11.41 KB, patch)
2014-06-12 19:51 UTC, Pranav Kant
needs-work Details | Review
MediaServerMiner: Added wrappers over dleyna-server D-Bus API (25.34 KB, patch)
2014-06-12 19:51 UTC, Pranav Kant
needs-work Details | Review
MediaServerMiner: Added a library for common tasks. (6.27 KB, patch)
2014-06-12 19:52 UTC, Pranav Kant
needs-work Details | Review
MediaServerMiner: Implemented basic miner functionality (6.25 KB, patch)
2014-06-12 19:52 UTC, Pranav Kant
none Details | Review
MediaServerMiner: implemented mining for non-searchable devices (10.33 KB, patch)
2014-06-12 19:53 UTC, Pranav Kant
none Details | Review
basic miner functionality (6.27 KB, patch)
2014-06-13 19:46 UTC, Pranav Kant
needs-work Details | Review
MediaServerMiner: Generate stubs for the dleyna-server D-Bus API (12.81 KB, patch)
2014-06-17 19:52 UTC, Pranav Kant
needs-work Details | Review
MediaServerMiner: Added wrappers over dleyna-server D-Bus API (24.81 KB, patch)
2014-06-17 19:53 UTC, Pranav Kant
needs-work Details | Review
implemented mining for non searchable devices (9.73 KB, patch)
2014-06-17 19:54 UTC, Pranav Kant
needs-work Details | Review
src: added stub for mediaserver miner (19.55 KB, patch)
2014-06-19 13:43 UTC, Debarshi Ray
none Details | Review
MediaServerMiner: Added wrappers over dleyna-server D-Bus API (27.39 KB, patch)
2014-06-19 21:54 UTC, Pranav Kant
none Details | Review
src: added stub for mediaserver miner (19.63 KB, patch)
2014-06-19 21:55 UTC, Pranav Kant
none Details | Review
MediaServerMiner: Added wrappers over dleyna-server D-Bus API (27.38 KB, patch)
2014-06-19 21:57 UTC, Pranav Kant
none Details | Review
MediaServerMiner: Added basic implementation for mining on searchable devices. (7.36 KB, patch)
2014-06-19 21:57 UTC, Pranav Kant
none Details | Review
MediaServerMiner: Added wrappers over dleyna-server D-Bus API (27.39 KB, patch)
2014-06-20 07:45 UTC, Pranav Kant
none Details | Review
MediaServerMiner: Added basic implementation for mining on searchable devices. (7.39 KB, patch)
2014-06-20 07:46 UTC, Pranav Kant
needs-work Details | Review
MediaServerMiner: Added support for searching non searchable devices. (7.72 KB, patch)
2014-06-20 07:46 UTC, Pranav Kant
none Details | Review
MediaServerMiner: Add support for collections (16.00 KB, patch)
2014-06-20 11:22 UTC, Pranav Kant
none Details | Review
MediaServerMiner: Added basic implementation for mining on searchable devices. (7.29 KB, patch)
2014-06-22 20:32 UTC, Pranav Kant
none Details | Review
MediaServerMiner: Added support for searching non searchable devices. (7.55 KB, patch)
2014-06-22 20:32 UTC, Pranav Kant
none Details | Review
MediaServerMiner: Add support for collections (15.71 KB, patch)
2014-06-22 20:35 UTC, Pranav Kant
none Details | Review
MediaServerMiner: Added wrappers over dleyna-server D-Bus API (27.33 KB, patch)
2014-07-03 10:11 UTC, Pranav Kant
none Details | Review
MediaServerMiner: Add support for collections (15.73 KB, patch)
2014-07-03 10:11 UTC, Pranav Kant
none Details | Review
MediaServerMiner: Add support for collections (15.91 KB, patch)
2014-07-04 09:43 UTC, Pranav Kant
none Details | Review
MediaServerMiner: Add support for collections (16.65 KB, patch)
2014-07-06 15:17 UTC, Pranav Kant
none Details | Review
src: Added stub for media server miner (19.17 KB, patch)
2014-08-18 09:16 UTC, Debarshi Ray
committed Details | Review
src: Added wrappers over dleyna-server D-Bus API (27.79 KB, patch)
2014-08-18 09:17 UTC, Debarshi Ray
committed Details | Review
Added basic implementation for mediaserver miner (7.31 KB, patch)
2014-08-19 19:31 UTC, Pranav Kant
needs-work Details | Review
Added support for non searchable devices (7.54 KB, patch)
2014-08-19 19:32 UTC, Pranav Kant
needs-work Details | Review
Added support for collections (16.66 KB, patch)
2014-08-19 19:32 UTC, Pranav Kant
none Details | Review
media-server: NULL-terminate the filter array (1016 bytes, patch)
2014-08-28 00:33 UTC, Debarshi Ray
committed Details | Review
media-server: Don't take a reference in dlna_servers_manager_get_server (1.02 KB, patch)
2014-08-28 00:34 UTC, Debarshi Ray
committed Details | Review
media-server: Added implementation for searchable servers (7.48 KB, patch)
2014-08-28 00:43 UTC, Debarshi Ray
committed Details | Review
data: add a DBus service file for org.gnome.OnlineMiners.MediaServer (1.90 KB, patch)
2014-09-05 13:12 UTC, Debarshi Ray
committed Details | Review
Added support for non searchable devices (6.28 KB, patch)
2014-09-14 22:49 UTC, Pranav Kant
none Details | Review
Added support for non searchable devices (10.95 KB, patch)
2014-12-14 16:10 UTC, Pranav Kant
none Details | Review
MediaServerMiner: Added support for searching non searchable devices (11.70 KB, patch)
2015-03-10 19:02 UTC, Pranav Kant
none Details | Review
media-server: Shuffle some code around (9.79 KB, patch)
2016-02-14 18:34 UTC, Debarshi Ray
committed Details | Review
media-server: Support non-searchable DLNA servers (4.70 KB, patch)
2016-02-14 18:35 UTC, Debarshi Ray
committed Details | Review

Description Pranav Kant 2014-04-24 21:36:15 UTC
It would be great to add a miner that browse all the DMSes on a local network and mine photos from them. The miner can then be used in gnome-photos to show the contents from all the DMSes around.
Comment 1 Pranav Kant 2014-06-08 21:21:16 UTC
Created attachment 278106 [details] [review]
src: added stub for mediaserver miner

http://bugzilla.gnome.org/show_bug.cgi?id=728912
Comment 2 Pranav Kant 2014-06-08 21:22:12 UTC
Created attachment 278107 [details] [review]
MediaServerMiner: Generate stubs for the dleyna-server D-Bus API

http://bugzilla.gnome.org/show_bug.cgi?id=728912
Comment 3 Pranav Kant 2014-06-08 21:22:47 UTC
Created attachment 278108 [details] [review]
MediaServerMiner: Added wrappers over dleyna-server D-Bus API

http://bugzilla.gnome.org/show_bug.cgi?id=728912
Comment 4 Pranav Kant 2014-06-08 21:23:07 UTC
Created attachment 278109 [details] [review]
MediaServerMiner: Added a library for common tasks.

This library is created just for the sake of abstraction. It acts as a main
point of contact for fetching any data from the media server. It will be helpful
for adding support for new type of media servers other than DLNA in the future
while still hiding such details from the main miner class.

http://bugzilla.gnome.org/show_bug.cgi?id=728912
Comment 5 Pranav Kant 2014-06-08 21:23:24 UTC
Created attachment 278110 [details] [review]
MediaServerMiner: Implemented basic miner functionality

The miner currently mines data only for media servers that are searchable.

http://bugzilla.gnome.org/show_bug.cgi?id=728912
Comment 6 Pranav Kant 2014-06-10 21:03:28 UTC
Created attachment 278222 [details] [review]
check for gio-unix-2.0 in configure
Comment 7 Pranav Kant 2014-06-12 19:45:08 UTC
Created attachment 278370 [details] [review]
MediaServerMiner: Generate stubs for the dleyna-server D-Bus API

updated version
Comment 8 Pranav Kant 2014-06-12 19:46:27 UTC
Created attachment 278371 [details] [review]
MediaServerMiner: Added wrappers over dleyna-server D-Bus API

updated version
Comment 9 Pranav Kant 2014-06-12 19:51:09 UTC
Created attachment 278372 [details] [review]
MediaServerMiner: Generate stubs for the dleyna-server D-Bus API

http://bugzilla.gnome.org/show_bug.cgi?id=728912
Comment 10 Pranav Kant 2014-06-12 19:51:54 UTC
Created attachment 278373 [details] [review]
MediaServerMiner: Added wrappers over dleyna-server D-Bus API

http://bugzilla.gnome.org/show_bug.cgi?id=728912
Comment 11 Pranav Kant 2014-06-12 19:52:37 UTC
Created attachment 278374 [details] [review]
MediaServerMiner: Added a library for common tasks.

This library is created just for the sake of abstraction. It acts as a main
point of contact for fetching any data from the media server. It will be helpful
for adding support for new type of media servers other than DLNA in the future
while still hiding such details from the main miner class.

http://bugzilla.gnome.org/show_bug.cgi?id=728912
Comment 12 Pranav Kant 2014-06-12 19:52:56 UTC
Created attachment 278375 [details] [review]
MediaServerMiner: Implemented basic miner functionality

The miner currently mines data only for media servers that are searchable.

http://bugzilla.gnome.org/show_bug.cgi?id=728912
Comment 13 Pranav Kant 2014-06-12 19:53:38 UTC
Created attachment 278376 [details] [review]
MediaServerMiner: implemented mining for non-searchable devices

Also wrapped processing of returned GVariant inside a function to avoid
duplicate code.

http://bugzilla.gnome.org/show_bug.cgi?id=728912
Comment 14 Pranav Kant 2014-06-13 19:46:01 UTC
Created attachment 278426 [details] [review]
basic miner functionality

minor improvements
Comment 15 Debarshi Ray 2014-06-14 11:44:25 UTC
Review of attachment 278106 [details] [review]:

Thanks for the patch, Pranav.

::: src/gom-mediaserver-miner-main.c
@@ +30,3 @@
+#define MINER_OBJECT_PATH "/org/gnome/OnlineMiners/MediaServer"
+
+#include "gom-mediaserver-miner.h"

If we are going to capitalize 'M' and 'S', and separate them with hyphens or underscores, then we might as well name these files as gom-media-server-*. What do you think?

::: src/gom-mediaserver-miner.c
@@ +28,3 @@
+#include "gom-mediaserver-miner.h"
+
+#define MINER_IDENTIFIER "gd:media_server:miner:a4a47a3e-eb55-11e3-b983-14feb59cfa0e"

I think a '-' would be more appropriate than a '_' here.

@@ +47,3 @@
+static void
+gom_media_server_miner_dispose (GObject *object)
+{

You should chain up to the parent class, even if you are not doing anything here. Otherwise you might be leaking the parent's resources. The other option is to override the dispose when you actually have stuff in a private structure that you want to dispose. :)
Comment 16 Debarshi Ray 2014-06-14 12:23:55 UTC
Review of attachment 278372 [details] [review]:

::: src/Makefile.am
@@ +205,3 @@
+    gom-dleyna-server-manager.h   gom-dleyna-server-manager.c \
+    gom-dleyna-server-device.h    gom-dleyna-server-device.c  \
+    gom-dleyna-server-mediacontainer2.h    gom-dleyna-server-mediacontainer2.c  \

The rules for building these files are missing.
Comment 17 Debarshi Ray 2014-06-14 12:40:31 UTC
Review of attachment 278373 [details] [review]:

::: src/gom-dlna-server-device.c
@@ +238,3 @@
+                                                        NULL,
+                                                        G_PARAM_READABLE |
+                                                        G_PARAM_WRITABLE |

Use G_PARAM_READWRITE.

@@ +242,3 @@
+                                                        G_PARAM_STATIC_NAME |
+                                                        G_PARAM_STATIC_BLURB |
+                                                        G_PARAM_STATIC_NICK));

Use G_PARAM_STATIC_STRINGS.
Comment 18 Pranav Kant 2014-06-17 19:50:53 UTC
(In reply to comment #15)
> Review of attachment 278106 [details] [review]:
> 
> Thanks for the patch, Pranav.
> 
> ::: src/gom-mediaserver-miner-main.c
> @@ +30,3 @@
> +#define MINER_OBJECT_PATH "/org/gnome/OnlineMiners/MediaServer"
> +
> +#include "gom-mediaserver-miner.h"
> 
> If we are going to capitalize 'M' and 'S', and separate them with hyphens or
> underscores, then we might as well name these files as gom-media-server-*. What
> do you think?
> 
> ::: src/gom-mediaserver-miner.c
> @@ +28,3 @@
> +#include "gom-mediaserver-miner.h"
> +
> +#define MINER_IDENTIFIER
> "gd:media_server:miner:a4a47a3e-eb55-11e3-b983-14feb59cfa0e"
> 
> I think a '-' would be more appropriate than a '_' here.

Well at the moment, that is done just for making it consistent with the goa provider type which is "media_server".

> 
> @@ +47,3 @@
> +static void
> +gom_media_server_miner_dispose (GObject *object)
> +{
> 
> You should chain up to the parent class, even if you are not doing anything
> here. Otherwise you might be leaking the parent's resources. The other option
> is to override the dispose when you actually have stuff in a private structure
> that you want to dispose. :)


Yes, its just a stub. I have taken care of that in subsequent patches.
Comment 19 Pranav Kant 2014-06-17 19:52:02 UTC
Created attachment 278624 [details] [review]
MediaServerMiner: Generate stubs for the dleyna-server D-Bus API
Comment 20 Pranav Kant 2014-06-17 19:53:01 UTC
Created attachment 278625 [details] [review]
MediaServerMiner: Added wrappers over dleyna-server D-Bus API
Comment 21 Pranav Kant 2014-06-17 19:54:20 UTC
Created attachment 278626 [details] [review]
implemented mining for non searchable devices
Comment 22 Debarshi Ray 2014-06-19 13:32:50 UTC
(In reply to comment #18)
> (In reply to comment #15)
> > Review of attachment 278106 [details] [review] [details]:
> > 
> > Thanks for the patch, Pranav.
> > 
> > ::: src/gom-mediaserver-miner-main.c
> > @@ +30,3 @@
> > +#define MINER_OBJECT_PATH "/org/gnome/OnlineMiners/MediaServer"
> > +
> > +#include "gom-mediaserver-miner.h"
> > 
> > If we are going to capitalize 'M' and 'S', and separate them with hyphens or
> > underscores, then we might as well name these files as gom-media-server-*. What
> > do you think?

Given that we are already treating 'media' as a separate word in so many other places, I think we should split the names of these files too.

> > ::: src/gom-mediaserver-miner.c
> > @@ +28,3 @@
> > +#include "gom-mediaserver-miner.h"
> > +
> > +#define MINER_IDENTIFIER
> > "gd:media_server:miner:a4a47a3e-eb55-11e3-b983-14feb59cfa0e"
> > 
> > I think a '-' would be more appropriate than a '_' here.
> 
> Well at the moment, that is done just for making it consistent with the goa
> provider type which is "media_server".

True, but MINER_IDENTIFIER goes into the Tracker DB, and a hyphen is more consistent with how things are there.
Comment 23 Debarshi Ray 2014-06-19 13:41:28 UTC
Review of attachment 278624 [details] [review]:

::: src/Makefile.am
@@ +149,3 @@
+    gom-dleyna-server-manager.h   gom-dleyna-server-manager.c \
+    gom-dleyna-server-device.h    gom-dleyna-server-device.c  \
+    gom-dleyna-server-mediacontainer2.h    gom-dleyna-server-mediacontainer2.c  \

One file per line, please.

@@ +205,3 @@
+    gom-dleyna-server-manager.h   gom-dleyna-server-manager.c \
+    gom-dleyna-server-device.h    gom-dleyna-server-device.c  \
+    gom-dleyna-server-mediacontainer2.h    gom-dleyna-server-mediacontainer2.c  \

Please use a variable instead of copy pasting these around. Like we do in gnome-photos.

@@ +220,3 @@
+		--generate-c-code gom-dleyna-server-device \
+		--interface-prefix com.intel.dLeynaServer. \
+		$<

These files should be named goadleynaservermediadevice.xml to maintain consistency with the naming in gnome-online-accounts.

@@ +231,3 @@
+gom-dleyna-server-mediacontainer2.h gom-dleyna-server-mediacontainer2.c: gom-dleyna-server-mediacontainer2.xml
+	$(AM_V_GEN)gdbus-codegen \
+		--c-namespace DleynaServer \

This interface comes from the gupnp project, not dleyna which is built on top of gupnp. The files should therefore be named gom-upnp-media-container2.xml, etc.. The --c-namespace should be Upnp. Using UPnP would lead to slightly weird symbols in the generated code.

::: src/gom-dleyna-server-device.xml
@@ +5,3 @@
+<!--
+  GNOME Online Miners - crawls through your online content
+  Copyright (c) 2014 Pranav Kant.

No need for this trailing dot.

@@ +25,3 @@
+<node>
+  <interface name="com.intel.dLeynaServer.MediaDevice">
+

No need for this space either because we are not using them in the other XMLs.

@@ +33,3 @@
+      <arg type="s" name="MimeType" direction="out"></arg>
+    </method>
+

Ditto.
Comment 24 Debarshi Ray 2014-06-19 13:43:16 UTC
Created attachment 278767 [details] [review]
src: added stub for mediaserver miner

I took the liberty to fix the remaining issues in the first two patches and squashed them together.
Comment 25 Debarshi Ray 2014-06-19 14:07:51 UTC
Review of attachment 278625 [details] [review]:

Thanks for the patches, Pranav. Good progress here.

I have one high level concern about the wrappers. They appear to be diverging needlessly from the similar code that is present in gnome-photos and gnome-online-miners. I can see why they might be slightly different at times, but the overall layout of the code should be same. These classes are simple enough, and I don't want stupid bugs to creep in. Other than that, it is simply easier to read and understand the code if things are consistent.

::: src/Makefile.am
@@ +129,3 @@
+    gom-dlna-server-device.c \
+    gom-dlna-server-manager.h \
+    gom-dlna-server-manager.c \

The order broke. Please put them above.

::: src/gom-dlna-server-device.c
@@ +25,3 @@
+
+#include "gom-dleyna-server-device.h"
+#include "gom-dleyna-server-mediacontainer2.h"

The names of these files need to be adjusted.

@@ +31,3 @@
+{
+  DleynaServerMediaDevice *device;
+  DleynaServerMediaContainer2 *container;

The C namespace is now Upnp. Needs adjusting.

@@ +151,3 @@
+  GomDlnaServerDevicePrivate *priv = self->priv;
+  GError *error = NULL;
+

Spurious newline. Move it three lines below.

@@ +165,3 @@
+
+  if (error != NULL)
+    g_warning ("error found");

The GError is being leaked.

::: src/gom-dlna-server-device.h
@@ +53,3 @@
+typedef struct _GomDlnaServerDevice        GomDlnaServerDevice;
+typedef struct _GomDlnaServerDeviceClass   GomDlnaServerDeviceClass;
+typedef struct _GomDlnaServerDevicePrivate GomDlnaServerDevicePrivate;

I think GomDlnaServer would be a better name because it would be consistent with PhotosDlnaRenderer.

::: src/gom-dlna-server-manager.c
@@ +120,3 @@
+    g_warning (error->message);
+    g_error_free (error);
+    priv->got_error = TRUE;

self will be leaked.

@@ +147,3 @@
+    g_warning ("Unable to connect to the dlnaServer.Manager DBus object: %s", error->message);
+    g_error_free (error);
+    priv->got_error = TRUE;

self will be leaked.

@@ +156,3 @@
+                    "swapped-object-signal::lost-server",
+                    gom_dlna_server_manager_server_lost_cb, self,
+                    NULL);

Inconsistent with g-o-a and g-p.

@@ +160,3 @@
+  dleyna_server_manager_call_get_servers (priv->proxy, NULL,
+                                          gom_dlna_server_manager_proxy_get_server_cb,
+                                          self);

You need to take a ref on self.

@@ +176,3 @@
+                                                                        construct_params);
+
+  g_object_add_weak_pointer (gom_dlna_server_manager_singleton, (gpointer) &gom_dlna_server_manager_singleton);

Inconsistent with g-o-a and g-p.

@@ +187,3 @@
+
+  self->priv = priv = G_TYPE_INSTANCE_GET_PRIVATE (self, GOM_TYPE_DLNA_SERVER_MANAGER,
+                                                   GomDlnaServerManagerPrivate);

Please use gom_dlna_server_manager_get_instance_private.

::: src/gom-dlna-server-manager.h
@@ +69,3 @@
+GType                    gom_dlna_server_manager_get_type      (void) G_GNUC_CONST;
+
+GomDlnaServerManager *gom_dlna_server_manager_dup_singleton (void);

Broken alignment.

@@ +72,3 @@
+
+GomDlnaServerDevice * gom_dlna_server_manager_get_device (GomDlnaServerManager *self,
+                                                          const gchar *udn);

I think gom_dlna_server_manager_get_server would be a better name. It would be consistent with photos_dlna_renderers_manager_dup_renderers and goa_dlna_server_manager_dup_servers.

@@ +74,3 @@
+                                                          const gchar *udn);
+
+gboolean                 gom_dlna_server_manager_is_available  (void);

Do we really need this? If someone does not want DLNA support then I would suggest adding a compile time option to turn off the miner. However that is not something we need to bother with right now.
Comment 26 Debarshi Ray 2014-06-19 16:13:00 UTC
Review of attachment 278374 [details] [review]:

We don't know what we will be abstracting the future. For example we don't if we will ever support AirPlay and if we do how that API would look like. Lets just put this code in gom-media-server-miner, which is quite small at the moment, and keeps things simple.

::: src/gom-mediaserver.h
@@ +35,3 @@
+                                 const gchar *udn,
+                                 gboolean dlnaSupported,
+                                 GList **list);

It should be possible to return the list, I think.
Comment 27 Debarshi Ray 2014-06-19 16:15:58 UTC
Review of attachment 278426 [details] [review]:

Thanks for the patch, Pranav. A few quick comments.

::: src/gom-mediaserver-miner.c
@@ +154,3 @@
+  GoaObject *object = GOA_OBJECT (job->service);
+  const char *udn;
+

Spurious newline.

@@ +160,3 @@
+                "dlna-supported", &dlna_supported,
+                "udn", &udn,
+                NULL);

It is better to use the goa_media_server_* API. Other than that, usually, g_object_getting a string allocates a new string that has to be freed.

@@ +162,3 @@
+                NULL);
+
+  GList *list = NULL;

Put this at the top of the block.

@@ +172,3 @@
+
+
+  GList *l = NULL;

Ditto.
Comment 28 Debarshi Ray 2014-06-19 16:23:34 UTC
Review of attachment 278626 [details] [review]:

::: src/gom-dlna-server-device.h
@@ +66,3 @@
 };
 
+

Spurious newline.

@@ +80,3 @@
 const gchar          *gom_dlna_server_device_get_udn                   (GomDlnaServerDevice  *self);
 
+GDBusConnection      *gom_dlna_server_device_get_mediacontainer_connection (GomDlnaServerDevice *device);

You might not need this.

::: src/gom-mediaserver.c
@@ +109,3 @@
+                                                         obj_path,
+                                                         NULL, /* GCancellable */
+                                                         &error);

Can't you use upnp_media_container2_proxy_new_for_bus_sync ? Then you won't need the GDBusConnection * .
Comment 29 Pranav Kant 2014-06-19 21:54:28 UTC
Created attachment 278804 [details] [review]
MediaServerMiner: Added wrappers over dleyna-server D-Bus API

Merged library functions into main miner file
Comment 30 Pranav Kant 2014-06-19 21:55:07 UTC
Created attachment 278805 [details] [review]
src: added stub for mediaserver miner
Comment 31 Pranav Kant 2014-06-19 21:57:14 UTC
Created attachment 278806 [details] [review]
MediaServerMiner: Added wrappers over dleyna-server D-Bus API

Fixes:
Comment 32 Pranav Kant 2014-06-19 21:57:40 UTC
Created attachment 278807 [details] [review]
MediaServerMiner: Added basic implementation for mining on searchable devices.

Fixes:
Comment 33 Pranav Kant 2014-06-20 07:45:13 UTC
Created attachment 278817 [details] [review]
MediaServerMiner: Added wrappers over dleyna-server D-Bus API

Fixes:
Comment 34 Pranav Kant 2014-06-20 07:46:12 UTC
Created attachment 278818 [details] [review]
MediaServerMiner: Added basic implementation for mining on searchable devices.

Fixes:
Comment 35 Pranav Kant 2014-06-20 07:46:33 UTC
Created attachment 278819 [details] [review]
MediaServerMiner: Added support for searching non searchable devices.

Fixes:
Comment 36 Pranav Kant 2014-06-20 11:22:58 UTC
Created attachment 278828 [details] [review]
MediaServerMiner: Add support for collections
Comment 37 Debarshi Ray 2014-06-20 18:00:07 UTC
Review of attachment 278818 [details] [review]:

Much better.

::: src/gom-media-server-miner.h
@@ +70,3 @@
+  const gchar *mimetype;
+  const gchar *path;
+} PhotoItem;

This can go into the .c file because it is not used in the public API.
Comment 38 Pranav Kant 2014-06-22 20:32:08 UTC
Created attachment 278952 [details] [review]
MediaServerMiner: Added basic implementation for mining on searchable devices.

Fixes:
Comment 39 Pranav Kant 2014-06-22 20:32:48 UTC
Created attachment 278953 [details] [review]
MediaServerMiner: Added support for searching non searchable devices.

Fixes:
Comment 40 Pranav Kant 2014-06-22 20:35:12 UTC
Created attachment 278954 [details] [review]
MediaServerMiner: Add support for collections
Comment 41 Pranav Kant 2014-06-22 20:35:57 UTC
All patches adjusted as according to Comment 37
Comment 42 Pranav Kant 2014-07-03 10:11:08 UTC
Created attachment 279830 [details] [review]
MediaServerMiner: Added wrappers over dleyna-server D-Bus API

Fixes:
Comment 43 Pranav Kant 2014-07-03 10:11:37 UTC
Created attachment 279831 [details] [review]
MediaServerMiner: Add support for collections
Comment 44 Pranav Kant 2014-07-04 09:43:33 UTC
Created attachment 279888 [details] [review]
MediaServerMiner: Add support for collections
Comment 45 Pranav Kant 2014-07-06 15:17:22 UTC
Created attachment 279988 [details] [review]
MediaServerMiner: Add support for collections
Comment 46 Debarshi Ray 2014-08-18 09:16:00 UTC
Created attachment 283719 [details] [review]
src: Added stub for media server miner

Rebased against master
Comment 47 Debarshi Ray 2014-08-18 09:17:03 UTC
Created attachment 283720 [details] [review]
src: Added wrappers over dleyna-server D-Bus API
Comment 48 Pranav Kant 2014-08-19 19:31:28 UTC
Created attachment 283917 [details] [review]
Added basic implementation for mediaserver miner

Rebased to master
Comment 49 Pranav Kant 2014-08-19 19:32:10 UTC
Created attachment 283918 [details] [review]
Added support for non searchable devices

Rebased to master
Comment 50 Pranav Kant 2014-08-19 19:32:41 UTC
Created attachment 283919 [details] [review]
Added support for collections

Rebased to master
Comment 51 Debarshi Ray 2014-08-27 19:16:23 UTC
Review of attachment 283917 [details] [review]:

Thanks for rebasing, Pranav. A few comments.

::: src/gom-media-server-miner.c
@@ +33,3 @@
 
+struct _GomMediaServerMinerPrivate {
+  GObject *mngr;

Why GObject instead of GomDlnaServersManager?

@@ +40,3 @@
+  const gchar *url;
+  const gchar *mimetype;
+  const gchar *path;

These should not be marked as const because we are strduping them and they need to be free-ed. In other words, each PhotoItem owns them.

@@ +152,3 @@
+
+static PhotoItem *
+generate_photo_item_from_variant (GVariant *var)

I think photo_item_new would be a better name, because we can then define a photo_item_free because we need to free each field.

@@ +181,3 @@
+                             const gchar *udn)
+{
+  GomDlnaServersManager *dlna_mngr = GOM_DLNA_SERVERS_MANAGER (mngr);

Do we need to pass this as a parameter? We are always using priv->mngr, so we can use it directly.

@@ +182,3 @@
+{
+  GomDlnaServersManager *dlna_mngr = GOM_DLNA_SERVERS_MANAGER (mngr);
+  GomDlnaServer *server = gom_dlna_servers_manager_get_server (dlna_mngr, udn);

We are leaking server because gom_dlna_servers_manager_get_server adds a new reference to it. I think that it should not add a reference because usually 'get' methods don't do that.

@@ +240,3 @@
+      account_miner_job_process_photo (job, tmp->data, "media_server", &local_error);
+      g_slice_free (PhotoItem, tmp->data);
+    }

We are leaking photos_list. Since we need to free each field (see above), we should use g_list_free_full with photo_item_free.
Comment 52 Debarshi Ray 2014-08-27 19:29:46 UTC
Review of attachment 283917 [details] [review]:

::: src/gom-media-server-miner.c
@@ +161,3 @@
+
+  g_variant_lookup (var, "DisplayName", "s", &str);
+  photo->name = g_strdup (str);

Aren't we leaking str? What about using "&s"?

@@ +169,3 @@
+  photo->path = g_strdup (str);
+
+  g_variant_lookup (var, "URLs", "@as", &var1);

We are leaking var1.
Comment 53 Debarshi Ray 2014-08-27 19:59:01 UTC
Review of attachment 283917 [details] [review]:

::: src/gom-media-server-miner.c
@@ +232,3 @@
+  GoaObject *object = GOA_OBJECT (g_hash_table_lookup (job->services, "photos"));
+
+  mediaserver = goa_object_peek_media_server (object);

Since the query vfunc is called in a thread, we should not use the _peek_ methods. Instead we should use goa_object_get_media_server. (we must fix this in the other miners)
Comment 54 Debarshi Ray 2014-08-27 22:32:01 UTC
Review of attachment 283918 [details] [review]:

::: src/gom-upnp-media-container2.xml
@@ +101,3 @@
       <arg type="o" name="RefPath" direction="out"></arg>
     </method>
+    <property type="i" name="ChildCount" access="read"></property>

This doesn't look right. The property is 'u' as documented in https://wiki.gnome.org/Projects/Rygel/MediaServer2Spec and implemented by dleyna-server.
Comment 55 Debarshi Ray 2014-08-27 23:27:33 UTC
Review of attachment 283918 [details] [review]:

::: src/gom-media-server-miner.c
@@ +42,3 @@
   const gchar *mimetype;
   const gchar *path;
+  const gchar *type;

Same thing as the previous patch. This shouldn't be const.

@@ +176,3 @@
+  if (g_str_equal (photo->type, "container"))
+    {
+      photo->url = NULL;

Since we are using g_slice_new0 there is no need to initialize it to NULL.

@@ +358,3 @@
+          g_warning ("Unable to process photo : %s",
+                     local_error->message);
+          g_error_free (local_error);

Just freeing the GError is not enough. It needs to be set to NULL, otherwise it can't be used in the next iteration of the for loop. We should use g_clear_error instead.

Also, this hunk should ideally be part of the previous patch.
Comment 56 Debarshi Ray 2014-08-27 23:30:32 UTC
Review of attachment 283720 [details] [review]:

::: src/gom-dlna-server.c
@@ +287,3 @@
+  GVariant *out = NULL;
+  gchar *query = g_strdup_printf ("Type = \"image.photo\"");
+  const gchar const *filter[] = {"DisplayName", "URLs", "Path", "MIMEType"};

The array should be NULL terminated. Otherwise there is no way of knowing how many elements there are because when it passed to the function it degenerates into a pointer. Sorry for not noticing this earlier.
Comment 57 Debarshi Ray 2014-08-28 00:19:43 UTC
Review of attachment 283917 [details] [review]:

::: src/gom-media-server-miner.c
@@ +123,3 @@
+    (job->connection,
+     job->cancellable, error,
+     job->datasource_urn, creator);

Unless we have something better than 'media_server' or the friendly name, we should leave this out, because this does not reflect well in the gnome-photos UI. The server is the source. The creator is a human.
Comment 58 Debarshi Ray 2014-08-28 00:32:46 UTC
Review of attachment 283917 [details] [review]:

::: src/gom-media-server-miner.c
@@ +65,3 @@
+  tmp_arr = g_strsplit_set (photo->path, "/", -1);
+
+  photo_id = g_strdup (tmp_arr[g_strv_length (tmp_arr) - 1]);

We are leaking photo_id. Is there a need to g_strdup it?

@@ +69,3 @@
+  photo_name = photo->name;
+
+  mimetype = photo->mimetype;

We don't need separate variables for these. We can use the fields in the struct directly.
Comment 59 Debarshi Ray 2014-08-28 00:33:34 UTC
Created attachment 284649 [details] [review]
media-server: NULL-terminate the filter array
Comment 60 Debarshi Ray 2014-08-28 00:34:12 UTC
Created attachment 284650 [details] [review]
media-server: Don't take a reference in dlna_servers_manager_get_server
Comment 61 Debarshi Ray 2014-08-28 00:43:58 UTC
Created attachment 284651 [details] [review]
media-server: Added implementation for searchable servers
Comment 62 Debarshi Ray 2014-09-05 13:12:49 UTC
Created attachment 285490 [details] [review]
data: add a DBus service file for org.gnome.OnlineMiners.MediaServer
Comment 63 Debarshi Ray 2014-09-05 18:04:59 UTC
Review of attachment 283918 [details] [review]:

::: src/gom-media-server-miner.c
@@ +328,1 @@
     }

I think it would be better to encapsulate all these details inside GomDlnaServer similar to the share/unshare API in PhotosDlnaRenderer. We could rename gom_dlna_server_search_objects to gom_dlna_server_get_photos (or something) and have all the logic that determines if a server is searchable or not, etc. there.
Comment 64 Pranav Kant 2014-09-14 22:49:01 UTC
Created attachment 286179 [details] [review]
Added support for non searchable devices

This attachment doesn't include suggestions from Comment 63 as of now.
Comment 65 Pranav Kant 2014-12-14 16:10:54 UTC
Created attachment 292698 [details] [review]
Added support for non searchable devices

This takes into account all suggestions as mentioned in Comment 63 plus minor tweaks.
Comment 66 Pranav Kant 2015-03-10 19:02:48 UTC
Created attachment 299045 [details] [review]
MediaServerMiner: Added support for searching non searchable devices
Comment 67 Debarshi Ray 2016-02-14 17:56:05 UTC
Review of attachment 299045 [details] [review]:

Thanks for the new patch, Pranav. This looks much better. However, it would be nice to split it into (a) a patch that just shuffles the code around, and (b) one that implements support for non-searchable devices. To make up for the delay in reviewing this, I will do this myself.

Some smaller bugs and issues:

::: src/gom-dlna-server.c
@@ +314,3 @@
+
+  g_variant_lookup (var, "DisplayName", "&s", &str);
+  photo->name = g_strdup (str);

We lost the gom_filename_strip_extension call.

@@ +442,3 @@
+  g_free (photo->mimetype);
+  g_free (photo->path);
+  g_free (photo->url);

We are not freeing photo->type.

::: src/gom-dlna-server.h
@@ +72,3 @@
+  gchar *url;
+  gchar *type;
+} PhotoItem;

Since it is no longer an internal type, I would namespace it with at least 'GomDlna'.

@@ +95,3 @@
 const gchar          *gom_dlna_server_get_udn                   (GomDlnaServer  *self);
 
+const GList          *gom_dlna_server_get_photos                (GomDlnaServer  *self);

Just GList *. See below.

::: src/gom-media-server-miner.c
@@ +123,3 @@
   GError *local_error = NULL;
+  GoaMediaServer *media_server;
+  const GList *photos_list;

We usually don't use const in the GLib/GObject world, unless it is something like a 'const gchar *'. Other than that, const is also inappropriate since we have to free the list ourselves.

@@ +143,3 @@
+  media_server = goa_object_get_media_server (object);
+  udn = goa_media_server_get_udn (media_server);
+  dlna_server = gom_dlna_servers_manager_get_server (GOM_DLNA_SERVERS_MANAGER (priv->mngr), udn);

The cast is not needed.

@@ +146,3 @@
+  if (dlna_server == NULL)
+    return;
+  friendly_name = gom_dlna_server_get_friendly_name (dlna_server);

friendly_name is unused.

@@ +150,3 @@
+
+  if (server == NULL)
+    return NULL; /* Server is offline. */

dlna_server and server are identical. Copy/paste error?

@@ +167,3 @@
   g_list_free_full (photos_list, (GDestroyNotify) photo_item_free);
+  g_object_unref (media_server);
+  g_object_unref (dlna_server);

dlna_server is owned by priv->mngr.
Comment 68 Debarshi Ray 2016-02-14 18:13:32 UTC
Review of attachment 299045 [details] [review]:

::: src/gom-dlna-server.c
@@ +375,3 @@
+  Keep inserting photo items into Glist **photo_list as they
+  are found.
+*/

The comment is not needed. It is quite obvious. :)

@@ +384,3 @@
+  GVariant *var = NULL;
+  GError *error = NULL;
+  GList *containers = NULL, *tmp;

'GList *l' is more idiomatic.

@@ +432,3 @@
+ out:
+  g_variant_unref (var);
+  g_object_unref (proxy);

Both var and proxy can be NULL in case of an error.
Comment 69 Debarshi Ray 2016-02-14 18:34:31 UTC
Created attachment 321144 [details] [review]
media-server: Shuffle some code around
Comment 70 Debarshi Ray 2016-02-14 18:35:14 UTC
Created attachment 321145 [details] [review]
media-server: Support non-searchable DLNA servers
Comment 71 André Klapper 2021-06-05 16:35:53 UTC
GNOME is going to shut down bugzilla.gnome.org in favor of gitlab.gnome.org.
As part of that, we are mass-closing older open tickets in bugzilla.gnome.org
which have not seen updates for a longer time (resources are unfortunately
quite limited so not every ticket can get handled).

If you can still reproduce the situation described in this ticket in a recent
and supported software version, then please follow
  https://wiki.gnome.org/GettingInTouch/BugReportingGuidelines
and create a new enhancement request ticket at
  https://gitlab.gnome.org/GNOME/gnome-online-miners/-/issues/

Thank you for your understanding and your help.