GNOME Bugzilla – Bug 747123
Show GPS information in properties
Last modified: 2017-01-14 18:21:08 UTC
The properties don't show anything about GPS information. It would be good if it were visible in the properties, and included an easy way to remove them, if necessary (for example, before I shared this photo).
Created attachment 315875 [details] geo-tagged image example
Hint: tracker exposes the information through the slo:location property.
it could nice if it could lunch Maps (gnome-maps) with the location pinned. might need ui-review for this.
(In reply to Debarshi Ray from comment #2) > Hint: tracker exposes the information through the slo:location property. I have tested tracker with the attached pic, but I don't know how to convert the value (if possible) to a human readable 'location'. 'slo:location' = 'urn:uuid:daff0e2a-6ba0-0629-db8c-96f8997463aa'
(In reply to Miguel Vaello Martínez from comment #4) > (In reply to Debarshi Ray from comment #2) > > Hint: tracker exposes the information through the slo:location property. > > I have tested tracker with the attached pic, but I don't know how to convert > the value (if possible) to a human readable 'location'. > > 'slo:location' = 'urn:uuid:daff0e2a-6ba0-0629-db8c-96f8997463aa' 'urn:uuid:daff0e2a-6ba0-0629-db8c-96f8997463aa' is the tracker identifier for the location object (slo:GeoLocation[0]). Try running on a terminal: "tracker info urn:uuid:daff0e2a-6ba0-0629-db8c-96f8997463aa". You'll see the properties of the given slo:GeoLocation object such as altitude, latitude, longitude... that's a good start. To get a more precise/human-readable info, you could query a database of locations (look at what gnome-maps does). Although I think showing the coordinates and a easy way to open them in a map is good enough. [0] https://developer.gnome.org/ontology/stable/slo-GeoLocation.html
Created attachment 335768 [details] [review] Ask for the slo:location of each item and save it as a gchar pointer
Review of attachment 335768 [details] [review]: Thanks for the patch, Shivam. The commit message can be better. We usually don't literally describe the changes in the patch, because one can already see them using 'git show'. It is more important to describe the overall impact of the changes. eg., "Query the slo:location and store it in PhotosBaseItem" Some more comments below: ::: src/photos-base-item.c @@ +88,3 @@ gchar *id; gchar *identifier; + gchar *location; We should g_free 'location' during destruction (ie. in _finalize). @@ +1639,3 @@ + location = tracker_sparql_cursor_get_string(cursor, PHOTOS_QUERY_COLUMNS_LOCATION, NULL); + photos_utils_set_string(&priv->location, location); Missing blank space before opening parenthesis. @@ +2281,2 @@ const gchar * +photos_base_item_get_location(PhotosBaseItem *self) Ditto. @@ +2283,3 @@ +{ + g_return_val_if_fail (PHOTOS_IS_BASE_ITEM (self), NULL); + return self->priv->location; This will not work with current Git master. We no longer use a priv pointer. Instead, you need to use photos_base_item_get_instance_private.
The next step would be to query the properties of the slo:Location object from Tracker and create the UI that we want. And as Felipe mentioned ... (In reply to Felipe Borges from comment #3) > it could nice if it could lunch Maps (gnome-maps) with the location pinned. > might need ui-review for this. ... we do need some input from the designers about this. (In reply to Felipe Borges from comment #5) > Try running on a terminal: "tracker info > urn:uuid:daff0e2a-6ba0-0629-db8c-96f8997463aa". > > You'll see the properties of the given slo:GeoLocation object such as > altitude, latitude, longitude... that's a good start. > > To get a more precise/human-readable info, you could query a database of > locations (look at what gnome-maps does). Although I think showing the > coordinates and a easy way to open them in a map is good enough. We can use geocode-glib to get a human readable (ie. city, street, etc.) representation of the (latitude, longitude, altitude) tuple. That's also what gnome-maps uses. I need to find out how to go from (latitude, longitude, altitude) to an actual dot on a map (probably a libchamplain tile). But, anyway, all this depends on the kind of UI that we want.
(In reply to Debarshi Ray from comment #8) > I need to find out how to go from (latitude, > longitude, altitude) to an actual dot on a map (probably a libchamplain > tile). From #gnome-hackers on GIMPNet: 18:45 <rishi> jonasdn: mattiasb: Hey! How do I go from (latitude, longitude) to a dot on a map (probably a libchamplain tile)? I know that geocode-glib will give me a GeocodePlace. 18:45 <rishi> I am trying to understand what I need to do to actually have a dot on a map. 19:05 <jonasdn> rishi: check the code in libchamplain demo dir 19:06 <jonasdn> I am phone, it is pretty much: get a ChamplainView, create. ChamplainMarker or ChamplainPoint 19:07 <jonasdn> It inherits ChamplainLocation 19:07 <jonasdn> So it has latitude and longitude props 19:07 <jonasdn> Add marker to view 19:07 *** kushal (kdas@82.221.107.187.adsl.inet-telecom.org) has quit: Quit: Leaving 19:08 <jonasdn> Or check the code on Contacts 19:08 <jonasdn> In 19:08 <phako> rishi: we have some code in a shotwell branch for that 19:13 <rishi> jonasdn: phako: Ok, great. Thanks.
Created attachment 337459 [details] [review] Query the slo:location and store it in PhotosBaseItem
Created attachment 337460 [details] [review] Query the slo:location and store it in PhotosBaseItem
Review of attachment 337460 [details] [review]: Thanks for the new patch, Shivam. This looks better. Let's wait a bit for the other patches to pan out before we merge it.
Created attachment 340935 [details] [review] Query slo:latitude and slo:longitude and save it in PhotosPropertiesDialog
Review of attachment 340935 [details] [review]: ::: src/photos-properties-dialog.c @@ +344,3 @@ + if (!error && connection) + { + cursor = tracker_sparql_connection_query (connection, location_query, NULL, &error); There are two problems here: (a) tracker_sparql_connection_query is a slow synchronous call. It will do some IPC, query a database and the UI will freeze until all that is completed. Instead, we should use an asynchronous call. photos_camera_cache_get_camera_async is one such example. (b) We should use photos_tracker_queue_select, which queues all pending tracker queries and issues them one by one.
Created attachment 342118 [details] [review] query-builder: Add photos_query_builder_location_query
Created attachment 342119 [details] [review] properties-dilaog: Query and display location
Created attachment 342128 [details] [review] properties-dilaog: Query and display location
Review of attachment 337460 [details] [review]: ::: src/photos-query-builder.c @@ +183,3 @@ "nmm:isoSpeed (?urn) " + "nmm:flash (?urn) " + "slo:location (?urn)", Nitpick: it will be nice to retain the space before the last closing parenthesis to ensure that the query string is easier to read when debugging.
Review of attachment 342118 [details] [review]: ::: src/photos-query-builder.c @@ +280,2 @@ PhotosQuery * +photos_query_builder_location_query (PhotosSearchContextState *state, const gchar *resource) Nitpick: trailing whitespace needs to be removed. You can configure git to mark these in red: $ git config --global color.diff auto $ git config --global color.status auto $ git config --global color.branch auto Also move it a bit lower to retain the order. ::: src/photos-query-builder.h @@ +44,3 @@ PhotosQuery *photos_query_builder_equipment_query (PhotosSearchContextState *state, GQuark equipment); +PhotosQuery *photos_query_builder_location_query (PhotosSearchContextState *state, const gchar *resource); Ditto about moving it lower.
Review of attachment 342128 [details] [review]: Aren't you missing the changes to configure.ac and src/Makefile.am? Also there is a typo in the commit message.
Review of attachment 337460 [details] [review]: ::: src/photos-query-builder.c @@ +183,3 @@ "nmm:isoSpeed (?urn) " + "nmm:flash (?urn) " + "slo:location (?urn)", Sorry, I meant "after the closing parenthesis", not "before".
(In reply to Debarshi Ray from comment #20) > Review of attachment 342128 [details] [review] [review]: > > Aren't you missing the changes to configure.ac and src/Makefile.am? Also > there is a typo in the commit message. I thought maybe a separate commit for it would be needed :) I'll add them.
Created attachment 342612 [details] [review] properties-dialog: Query and display location
Created attachment 342613 [details] [review] Query the slo:location and store it in PhotosBaseItem
Created attachment 342615 [details] [review] query-builder: Add photos_query_builder_location_query
Created attachment 342617 [details] [review] properties-dialog: Query and display location
(In reply to Shivam Tripathi from comment #22) > (In reply to Debarshi Ray from comment #20) > > Review of attachment 342128 [details] [review] [review] [review]: > > > > Aren't you missing the changes to configure.ac and src/Makefile.am? Also > > there is a typo in the commit message. > > I thought maybe a separate commit for it would be needed :) I'll add them. The guiding principle is to make sure that the code compiles and works after every single commit. It is important because: (a) We have Continuous Delivery [1] in the form of gnome-continuous and the nightly flatpak bundles and we don't want to temporarily break them. (b) When there is a regression, it becomes hard to identify the offending commit if some of the commits break the build or the functionality. See 'git bisect'. [1] https://en.wikipedia.org/wiki/Continuous_delivery
Review of attachment 342617 [details] [review]: Thanks for the new patch, Shivam. It is getting better. There are some compiler warnings, memory errors and some general code layout issues that I have pointed out below: ::: configure.ac @@ +107,3 @@ PKG_CHECK_MODULES(PNG, [libpng16]) PKG_CHECK_MODULES(TRACKER, [tracker-control-1.0 tracker-sparql-1.0]) +PKG_CHECK_MODULES(GEOCODE, [geocode-glib-1.0]) Nitpick: move it above to maintain the order. ::: src/photos-properties-dialog.c @@ +128,3 @@ + +const gchar * +photos_properties_dialog_get_location_finish (PhotosPropertiesDialog *self, GAsyncResult *res, GError **error) This function should be static since it is not used outside this file and doesn't have an entry in the header file. Secondly, it returns a 'gchar *', not 'const gchar *' because the caller has to free the returned string. However, this won't be needed after all. See below. @@ +167,3 @@ + gtk_widget_set_halign (location_data, GTK_ALIGN_START); + gtk_grid_attach_next_to (GTK_GRID (self->grid), location_data, self->location_w, GTK_POS_RIGHT, 2, 1); + gtk_widget_show (location_data); You can move this to the function below. @@ +181,3 @@ + GTask *task = G_TASK (user_data); + GeocodeReverse *rev; + GeocodePlace *place; Would be nicer if place if initialized to NULL. @@ +188,3 @@ + if(error != NULL) + { + g_task_return_error (task, error); We can simplify things by not using a GTask. See below. @@ +201,3 @@ + + out: + g_object_unref (rev); We don't need to unref 'rev' (ie. source_object). The convention for GAsyncReadyCalback functions is to not unref the source_object because whoever invoked the callback will take care of it. @@ +202,3 @@ + out: + g_object_unref (rev); + g_object_unref (place); In case of an error, 'place' will be NULL. In that case, using g_object_unref will lead to a CRITICAL. Instead we should use g_clear_object. @@ +292,3 @@ + task = g_task_new (self, self->cancellable, photos_properties_dialog_get_location, self); + g_task_set_source_tag (task, photos_properties_dialog_get_location_async); + g_task_set_task_data (task, location_url, NULL); We can simplify things by not using a GTask. See below. @@ +452,3 @@ gchar *date_created_str = NULL; gchar *date_modified_str; + gchar *location_url; const gchar * @@ +575,3 @@ + location_url = photos_base_item_get_location (item); + photos_properties_dialog_get_location_async (self, location_url); This looks a bit odd. Usually *_async functions take a GCancellable, a GAsyncReadyCallback, etc.. You can either make it accept all that, or I would suggest simplifying it by moving the majority of photos_properties_dialog_get_location_async inside the following 'if branch'.
Created attachment 342873 [details] [review] properties-dialog: Query and display location
Review of attachment 342873 [details] [review]: I took a really quick look. There are some pending issues from the previous patch and few others: ::: configure.ac @@ +107,3 @@ PKG_CHECK_MODULES(PNG, [libpng16]) PKG_CHECK_MODULES(TRACKER, [tracker-control-1.0 tracker-sparql-1.0]) +PKG_CHECK_MODULES(GEOCODE, [geocode-glib-1.0]) Nitpick: please move it higher up to maintain the order. ::: src/Makefile.am @@ +308,3 @@ $(PNG_CFLAGS) \ $(TRACKER_CFLAGS) \ + $(GEOCODE_CFLAGS) \ Ditto. @@ +338,3 @@ $(PNG_LIBS) \ $(TRACKER_LIBS) \ + $(GEOCODE_LIBS) \ Ditto. ::: src/photos-properties-dialog.c @@ +132,3 @@ + PhotosPropertiesDialog *self = PHOTOS_PROPERTIES_DIALOG (user_data); + + rev = (GeocodeReverse *) source_object; Use GEOCODE_REVERSE and move it above to the line where the variable is defined. Also 'reverse' would be better than 'rev'. @@ +143,3 @@ + { + location = g_strdup_printf ("%s, %s, %s", geocode_place_get_name(place), + geocode_place_get_state(place), There should be a whitespace before the opening parenthesis. Also please break the function calls into separate lines and variables. @@ +500,3 @@ + location_url = photos_base_item_get_location (item); + // photos_properties_dialog_get_location_async (self, location_url); This needs to be removed. @@ +516,3 @@ + + photos_tracker_queue_select (queue, + query->sparql, The alignment is off. @@ +517,3 @@ + photos_tracker_queue_select (queue, + query->sparql, + self->cancellable, There is no need to use the self->cancellable here. Just pass NULL. This cancellable is meant to cancel pending operations when the PropertiesDialog is destroyed. However, since we are taking a reference to it while the query is going on, it can't get destroyed in the first place.
(In reply to Debarshi Ray from comment #30) > Review of attachment 342873 [details] [review] [review]: > > I took a really quick look. There are some pending issues from the previous > patch and few others: > > ::: configure.ac > @@ +107,3 @@ > PKG_CHECK_MODULES(PNG, [libpng16]) > PKG_CHECK_MODULES(TRACKER, [tracker-control-1.0 tracker-sparql-1.0]) > +PKG_CHECK_MODULES(GEOCODE, [geocode-glib-1.0]) > > Nitpick: please move it higher up to maintain the order. > > ::: src/Makefile.am > @@ +308,3 @@ > $(PNG_CFLAGS) \ > $(TRACKER_CFLAGS) \ > + $(GEOCODE_CFLAGS) \ > > Ditto. > > @@ +338,3 @@ > $(PNG_LIBS) \ > $(TRACKER_LIBS) \ > + $(GEOCODE_LIBS) \ > > Ditto. > > ::: src/photos-properties-dialog.c > @@ +132,3 @@ > + PhotosPropertiesDialog *self = PHOTOS_PROPERTIES_DIALOG (user_data); > + > + rev = (GeocodeReverse *) source_object; > > Use GEOCODE_REVERSE and move it above to the line where the variable is > defined. Also 'reverse' would be better than 'rev'. > > @@ +143,3 @@ > + { > + location = g_strdup_printf ("%s, %s, %s", > geocode_place_get_name(place), > + > geocode_place_get_state(place), > > There should be a whitespace before the opening parenthesis. Also please > break the function calls into separate lines and variables. > > @@ +500,3 @@ > > + location_url = photos_base_item_get_location (item); > + // photos_properties_dialog_get_location_async (self, location_url); > > This needs to be removed. > > @@ +516,3 @@ > + > + photos_tracker_queue_select (queue, > + query->sparql, > > The alignment is off. > > @@ +517,3 @@ > + photos_tracker_queue_select (queue, > + query->sparql, > + self->cancellable, > > There is no need to use the self->cancellable here. Just pass NULL. > > This cancellable is meant to cancel pending operations when the > PropertiesDialog is destroyed. However, since we are taking a reference to > it while the query is going on, it can't get destroyed in the first place. ** This cancellable is meant to cancel pending operations when the PropertiesDialog is destroyed. However, since we are taking a reference to it while the query is going on, it can't get destroyed in the first place. Ok. ** This needs to be removed. ** I took a really quick look. There are some pending issues from the previous patch and few others Missed them by mistake.
Created attachment 342969 [details] [review] properties-dialog: Query and display location
Created attachment 343446 [details] [review] properties-dialog: Query and display location
Review of attachment 343446 [details] [review]: Thanks for the new patch, Shivam. Some comments after a closer inspection: ::: src/photos-properties-dialog.c @@ +143,3 @@ + goto out; + } + else No need for the 'else' since you are using a 'goto out' earlier. @@ +148,3 @@ + location_state = geocode_place_get_state (place); + location_country = geocode_place_get_country (place); + location = g_strdup_printf ("%s, %s, %s", location_name, location_state, location_country); I am a little unsure about the best way to format this. The mockups in bug 751116 have '<city>, <country>'. I don't know what will happen if some of the properties are not valid for a locations. Will they be NULL? eg., not every country has states. We cannot have '(nil)' show up in the UI. Also, the 'state' for attachment 315875 [details] is 'Southeast', not 'South Moravia', which seems strange. Similarly, as far as I can see, the 'place' isn't always correct, but it is close. It doesn't resolve to the same street that my flat is on. Maybe we shouldn't show a string that is likely to be wrong. In the future we will have map to accurately pinpoint the location if someone is interested. How do rural locations compare with urban ones? We should consult the designers on this. In the meantime, could you please gather some data on how accurately it is resolving co-ordinates in your location? I'd suggest using something less likely to be wrong as a stop-gap. @@ +150,3 @@ + location = g_strdup_printf ("%s, %s, %s", location_name, location_state, location_country); + + if (location != NULL) After the previous g_strdup_printf, 'location' is guaranteed to be non-NULL. @@ +175,3 @@ + GeocodeLocation *loc; + GeocodeReverse *reverse; + GCancellable *cancellable = g_cancellable_new (); We should use self->cancellable here. The idea is to cancel all pending asynchronous operations when the PropertiesDialog is destroyed. @@ +185,3 @@ + if (error != NULL) + { + g_warning ("%s", error->message); Once you use self->cancellable with the async call, you need to carefully handle the cancellation case. Nitpitck: it's good to prefix the error->message with a string literal so that it is easier to locate where the error came from. @@ +199,3 @@ + + + error = NULL; This seems unnecessary. Typo? @@ +202,3 @@ + + loc = geocode_location_new (latitude, longitude, GEOCODE_LOCATION_ACCURACY_UNKNOWN); + reverse = geocode_reverse_new_for_location (loc); We need to unref reverse. @@ +226,3 @@ + if (error != NULL) + { + g_warning ("%s", error->message); Ditto about prefixing the error string. @@ +230,3 @@ + } + + tracker_sparql_cursor_next_async (cursor, NULL, photos_properties_dialog_location_cursor_next, user_data); Use self->cancellable, not NULL. See above. @@ +516,3 @@ + app = g_application_get_default (); + state = photos_search_context_get_state (PHOTOS_SEARCH_CONTEXT (app)); + queue = photos_tracker_queue_dup_singleton (NULL, &error); In the rare case that something is wrong with the environment, queue can be NULL and the GError can be set. So, we need to at least free the GError and check if queue is NULL or not. We should also unref queue at some point. @@ +520,3 @@ + + photos_tracker_queue_select (queue, + query->sparql, Nitpick: the alignment is still off.
Created attachment 343477 [details] [review] query-builder: Add photos_query_builder_location_query Committed with a minor tweak.
Created attachment 343478 [details] [review] properties-dialog: Show the location I fixed the above issues and pushed it.
I am closing this bug, but we still need to resolve the issues surrounding the formatting of the location string (see comment 34). Let's use a fresh new bug for that.
(In reply to Debarshi Ray from comment #36) > Created attachment 343478 [details] [review] [review] > properties-dialog: Show the location > > I fixed the above issues and pushed it. Thank you. >> No need for the 'else' since you are using a 'goto out' earlier. Yes. >> I am a little unsure about the best way to format this. The mockups in bug 751116 have '<city>, <country>'. Ok. I didn't know about it. I will try to gather data about accuracy. >> After the previous g_strdup_printf, 'location' is guaranteed to be non-NULL. Yes. >> We should use self->cancellable here. The idea is to cancel all pending asynchronous operations when the PropertiesDialog is destroyed.
I am little uncomfortable with the correct usage of cancellable. >> Nitpitck: it's good to prefix the error->message with a string literal so that it is easier to locate where the error came from. Ok. >> This seems unnecessary. Typo? Maybe I was trying to reset error. Appears it is not needed. >> We need to unref reverse. Yes >> In the rare case that something is wrong with the environment, queue can be NULL and the GError can be set. So, we need to at least free the GError and check if queue is NULL or not. I didn't completely understand. I'll ask on irc. Thank you for fixing the issues. :)