GNOME Bugzilla – Bug 741591
Add show-contact action
Last modified: 2015-01-31 23:51:33 UTC
So we want to be able to show the location of contacts in Maps. To do so we need to be able to get contact data out of the Folks backend. This is not possible using gjs atm. Since libgee is not very introspectable. So we would need a C shim library that we introspect ourself. The end goal of this is being able to press a contact in some way in gnome-contacts or elsewhere and get to Maps.
Created attachment 292805 [details] [review] utils: Handle all loadable icons
Created attachment 292806 [details] [review] Add c shim library
Created attachment 292807 [details] [review] Convert searchResultMarker to placeMarker
Created attachment 292808 [details] [review] mapView: Add gotoBBox helper
Created attachment 292809 [details] [review] mapBubble: Do not convert to Place
Created attachment 292810 [details] [review] Add busyMarker module
Created attachment 292811 [details] [review] mainWindow: Add markBusy method
Created attachment 292812 [details] [review] Add contact module
Created attachment 292813 [details] [review] Add 'show-contact' action
Created attachment 292822 [details] Cast of contacts with map
Created attachment 292837 [details] [review] Add c shim library
Review of attachment 292837 [details] [review]: Great work! Some general comments: - Wouldn't be better to name this library GnomeMaps or GnomeMapsPrivate (and gnome_maps/gnome_maps_private) instead of MapsC? - The Emacs and Vim modelines are wrong (it says that it should be used JS2 syntax mode instead of C and the indentation configuration doesn't correspond to the file content) - Should we use 4-spaces or 2-spaces indentation for the C code? Also, the systemd/Linux code style is more similar to our current JS style I think, but is not very common in the GLib world. - What about change "Contacts" class name to ContactList, ContactCollection or ContactsManager? - Do we have any advantage on writing this glue code in Vala? ::: lib/Makefile.am @@ +19,3 @@ + --identifier-prefix MapsC \ + --fhead "#ifndef __MAPSC_ENUM_TYPES_H__\n#define __MAPSC_ENUM_TYPES_H__\n\n#include \"mapsc-contacts.h\"\n\nG_BEGIN_DECLS\n" \ + --fprod "/* enumeratiobns from \"@filename@\" */\n" \ enumeratiobns -> enumerations @@ +33,3 @@ + --vprod " { @VALUENAME@, \"@VALUENAME@\", \"@valuenick@\" }," \ + --vtail " { 0, NULL, NULL }\n };\n etype = g_@type@_register_static (\"@EnumName@\", values);\n }\n return etype;\n}\n" $^ > xgen-$(@F) \ + && mv -f xgen-$(@F) $@) God! all this mkenums stuff is Automess+GLib black magic! :D Isn't better to use template files? @@ +39,3 @@ +INTROSPECTION_GIRS = +INTROSPECTION_SCANNER_ARGS = --warn-all +INTROSPECTION_COMPILER_ARGS = Unneded INTROSPECTION_COMPILER_ARGS var? @@ +50,3 @@ +MapsC_1_0_gir_SCANNERFLAGS = \ + --symbol-prefix=mapsc \ + --identifier-prefix=MapsC \ Grrr... I really don't know if these two flags are really necessary, some use these, some don't. @@ +51,3 @@ + --symbol-prefix=mapsc \ + --identifier-prefix=MapsC \ + --pkg-export=mapsc-1.0 There is a _EXPORT_PACKAGES variable to specify the --pkg-export option. ::: lib/mapsc-contact.c @@ +112,3 @@ + break; + } +} There are some indentation inconsistencies in these methods. @@ +219,3 @@ +mapsc_contact_get_places (MapsCContact *contact) +{ + g_return_if_fail (contact != NULL); Looking at GLib's _G_TYPE_CIT macro, it looks like MAPSC_IS_CONTACT is enough to check if the object is not null. So this line is not needed. Same in other parts of patch. @@ +244,3 @@ + geocode_place_set_location (data->place, location); + g_object_set (G_OBJECT (data->place), "osm-type", + geocode_place_get_osm_type (place)); Alignment @@ +246,3 @@ + geocode_place_get_osm_type (place)); + g_object_set (G_OBJECT (data->place), "osm-id", + geocode_place_get_osm_id (place)); Alignment @@ +247,3 @@ + g_object_set (G_OBJECT (data->place), "osm-id", + geocode_place_get_osm_id (place)); + Do we want to request the addition of setters to these properties @@ +256,3 @@ + geocode_place_get_street (place)); + + } street-address has setter. @@ +257,3 @@ + + } + } May I ask why we don't just duplicate the GeocodePlace object? @@ +271,3 @@ + if (call_callback) + data->callback (contact); +} Should we free "places"? @@ +282,3 @@ + g_value_set_static_string (value, s); + g_hash_table_insert (ht, g_strdup (key), value); + Empty line ::: lib/mapsc-contact.h @@ +38,3 @@ + * MapsCContactGeocodeCallback: + * @contact: A #MapsCContact object + **/ */ (extra asterisk). Same applies to other files. @@ +41,3 @@ +typedef void (*MapsCContactGeocodeCallback) (MapsCContact *contact); + +struct _MapsCContact { Separated line for brace? (same in other files). @@ +44,3 @@ + GObject parent_instance; + MapsCContactPrivate *priv; + Empty line @@ +58,3 @@ +GList *mapsc_contact_get_places (MapsCContact *contact); +void mapsc_contact_geocode (MapsCContact *contact, + MapsCContactGeocodeCallback callback); Alignment? ::: lib/mapsc-contacts.c @@ +110,3 @@ + MapsCContact *contact; + GLoadableIcon *avatar; + GeeCollection *set; Maybe addresses or adresses_set? @@ +118,3 @@ + + iter = gee_iterable_iterator (GEE_ITERABLE (set)); + if (!iter) Reading the documentation of Gee, it looks like gee_iterable_iterator never returns null, you should read the "valid" property. @@ +121,3 @@ + return NULL; + + if (!gee_iterator_has_next (iter)) Or maybe this is just enough? @@ +152,3 @@ + map = folks_abstract_field_details_get_parameters (details); + keys = gee_multi_map_get_keys (map); + keys_iter = gee_iterable_iterator (GEE_ITERABLE (keys)); Same as iter variable @@ +154,3 @@ + keys_iter = gee_iterable_iterator (GEE_ITERABLE (keys)); + if (!keys_iter) + return NULL; continue instead of return? Don't you want to iterate the whole set of postal addresses? @@ +168,3 @@ + type = gee_iterator_get (values_iter); + } + } I really don't get what are the FolksAbstractFieldDetails parameters. ::: lib/mapsc.h @@ +1,2 @@ +#ifndef __MAPSC_CONTACTS_H__ +#define __MAPSC_CONTACTS_H__ Wrong constant. @@ +7,3 @@ +#include "mapsc-contact-address.h" + +#endif Missing copyright?
Review of attachment 292837 [details] [review]: Thanks for review! So to answer: - Naming is hard. GnomeMaps or GnomeMapsPrivate (with gnome_maps- or gnome_maps_private prefix) feels to long to write :) And a bit reduntant to be honest, since we only use it inside GNOME Maps. The special thing about the library is that it is a C-wrapper. The naming does not need to be all that descriptive since this is a private library. But I agree MapsC may not be the best. - Will fix the modlines, silly copy/paste error. - I prefer 2 spaces. And I want to keep the style close to glib, since that is what others in our ecosystem use, even if I personally maybe would prefer others. - I do not really like tacking on stuff like "Manager" to classes, it does not add that much. ContactList, maybe but it is not really a list. ContactStore, maybe? - Yes, writing it in Vala would probably be a nice match for libgee/libfolks and if I where more comfortable in that language I would probably have done so. But having it in C might be nice if we add more stuff to the shim. C was the language that enabled me to do this quick (and quite sloppy) :) I will not be rewriting it in Vala. If you want to, feel free. Thanks again! ::: lib/Makefile.am @@ +19,3 @@ + --identifier-prefix MapsC \ + --fhead "#ifndef __MAPSC_ENUM_TYPES_H__\n#define __MAPSC_ENUM_TYPES_H__\n\n#include \"mapsc-contacts.h\"\n\nG_BEGIN_DECLS\n" \ + --fprod "/* enumeratiobns from \"@filename@\" */\n" \ Yes! @@ +33,3 @@ + --vprod " { @VALUENAME@, \"@VALUENAME@\", \"@valuenick@\" }," \ + --vtail " { 0, NULL, NULL }\n };\n etype = g_@type@_register_static (\"@EnumName@\", values);\n }\n return etype;\n}\n" $^ > xgen-$(@F) \ + && mv -f xgen-$(@F) $@) Maybe? Probably? :) This is all pretty much copy/paste from libchamplain/geocode-glib. @@ +39,3 @@ +INTROSPECTION_GIRS = +INTROSPECTION_SCANNER_ARGS = --warn-all +INTROSPECTION_COMPILER_ARGS = Seems so! Thanks! @@ +50,3 @@ +MapsC_1_0_gir_SCANNERFLAGS = \ + --symbol-prefix=mapsc \ + --identifier-prefix=MapsC \ They are needed. If we keep the name. I think. Since MapsC is already a bit camel cased. Not 100% sure tho, was def. needed for the enum-types. @@ +51,3 @@ + --symbol-prefix=mapsc \ + --identifier-prefix=MapsC \ + --pkg-export=mapsc-1.0 Oh, so MapsC_1_0_gir_EXPORT_PACKAGES=mapsc-1.0? ::: lib/mapsc-contact.c @@ +112,3 @@ + break; + } +} yes, will fix. @@ +219,3 @@ +mapsc_contact_get_places (MapsCContact *contact) +{ + g_return_if_fail (contact != NULL); Great! @@ +244,3 @@ + geocode_place_set_location (data->place, location); + g_object_set (G_OBJECT (data->place), "osm-type", + geocode_place_get_osm_type (place)); yes @@ +247,3 @@ + g_object_set (G_OBJECT (data->place), "osm-id", + geocode_place_get_osm_id (place)); + Not sure. It is not that burdensome to be without them, and setting them is not usual behaviour. So I think having them without setters is fine. It also shows that setting them is special case i nregards to the functionality in geocode-glib. @@ +256,3 @@ + geocode_place_get_street (place)); + + } yep, can be converted. @@ +257,3 @@ + + } + } You may. I think that is what we should do. It is probably really a left over from when I had a MapsC::ContactAddress object as well, until I thought that just having place objects on the Contact class was good enough. @@ +271,3 @@ + if (call_callback) + data->callback (contact); +} Probably, will investigate. ::: lib/mapsc-contact.h @@ +38,3 @@ + * MapsCContactGeocodeCallback: + * @contact: A #MapsCContact object + **/ Thanks! @@ +58,3 @@ +GList *mapsc_contact_get_places (MapsCContact *contact); +void mapsc_contact_geocode (MapsCContact *contact, + MapsCContactGeocodeCallback callback); Yep. ::: lib/mapsc-contacts.c @@ +110,3 @@ + MapsCContact *contact; + GLoadableIcon *avatar; + GeeCollection *set; yeah @@ +118,3 @@ + + iter = gee_iterable_iterator (GEE_ITERABLE (set)); + if (!iter) Yes, that is probably so. At one point I had a lot of asserts about iter being NULL. But I think that was related to another bug. @@ +121,3 @@ + return NULL; + + if (!gee_iterator_has_next (iter)) I think so. @@ +154,3 @@ + keys_iter = gee_iterable_iterator (GEE_ITERABLE (keys)); + if (!keys_iter) + return NULL; This is not iterating the postal addresses, it is iterating the parameter property of a postal address, which will hold the type, eg "Work", "Home" or "Other" @@ +168,3 @@ + type = gee_iterator_get (values_iter); + } + } It is a mess and took me a long time to figure out. FolkAbstractFieldDetails is a class that describes a parameter, exactly what it is is dependent on context. In this case it is the type of the postal address. ::: lib/mapsc.h @@ +1,2 @@ +#ifndef __MAPSC_CONTACTS_H__ +#define __MAPSC_CONTACTS_H__ Yep. @@ +7,3 @@ +#include "mapsc-contact-address.h" + +#endif Yes, do you want one?
Created attachment 293229 [details] [review] Unify favoritesPopover/placeEntry filter code The code is moved to a method in Place class. https://bugzilla.gnome.org/show_bug.cgi?id=741767
Created attachment 293230 [details] [review] utils: Handle all loadable icons
Created attachment 293231 [details] [review] Convert searchResultMarker to placeMarker
Created attachment 293232 [details] [review] mapView: Add gotoBBox helper
Created attachment 293233 [details] [review] mapBubble: Do not convert to Place
Created attachment 293234 [details] [review] Add busyMarker module
Created attachment 293235 [details] [review] mainWindow: Add markBusy method
Created attachment 293236 [details] [review] Add c shim library
Created attachment 293237 [details] [review] Add contact module
Created attachment 293238 [details] [review] Add 'show-contact' action
(In reply to comment #12) > > Some general comments: > > - Wouldn't be better to name this library GnomeMaps or GnomeMapsPrivate (and > gnome_maps/gnome_maps_private) instead of MapsC? I named it just Maps in this version. With a library name libgnome-maps.la. > - The Emacs and Vim modelines are wrong (it says that it should be used JS2 > syntax mode instead of C and the indentation configuration doesn't correspond > to the file content) Removed the modelines. > - What about change "Contacts" class name to ContactList, ContactCollection or > ContactsManager? Named it ContactStore. > - Do we have any advantage on writing this glue code in Vala? > Maybe, but it is still in C :)
Created attachment 293239 [details] [review] Add c shim library
Created attachment 293247 [details] [review] Add c shim library
Created attachment 293248 [details] [review] Add contact module
Created attachment 293249 [details] [review] Add 'show-contact' action
Created attachment 293254 [details] [review] Add contact module
Created attachment 293255 [details] [review] Add contactMarker class Code shamelessly stolen from Damián Nohales.
Created attachment 293256 [details] [review] Add 'show-contact' action
Review of attachment 293247 [details] [review]: Some code style general comments: - Braces on new lines when declaring structs and enums - Same for while, if, switch, etc statements - Some methods doesn't have it parameters aligned I have this review in draft since Tuesday, I prefer submit it now to streamline the review and keep going tomorrow. ::: configure.ac @@ +40,3 @@ +GEOCODE_MIN_VERSION=3.15.2 + +PKG_CHECK_MODULES(GNOME_MAPS_LIB, [ Maybe name the variable GNOME_MAPS_DEPS? ::: lib/Makefile.am @@ +52,3 @@ + +CLEANFILES = $(gir_DATA) $(typelib_DATA) $(BUILT_SOURCES) +endif According to Mattias, we need to align all backslashes to column 72 using 8-width tabs ::: lib/maps-contact-store.c @@ +214,3 @@ + * @store: A #MapsContactStore object + * @callback: (scope async): A #MapsContactStoreLookupCallback function + * Empty doc line ::: lib/maps-contact-store.h @@ +32,3 @@ +typedef struct _MapsContactStore MapsContactStore; +typedef struct _MapsContactStoreClass MapsContactStoreClass; +typedef struct _MapsContactStorePrivate MapsContactStorePrivate; I hate myself for telling this, but I think we need to align this. @@ +44,3 @@ + MAPS_CONTACT_STORE_STATE_LOADING, + MAPS_CONTACT_STORE_STATE_LOADED, + } MapsContactStoreState; Unneeded indentation @@ +69,3 @@ + const char *id, + MapsContactStoreLookupCallback callback); +GList *maps_contact_store_get_contacts (MapsContactStore *store); I would prefer to not align so much, but I see Gtk+ also aligns the return type of functions (note that geocode doesn't do that) ::: lib/maps-contact.c @@ +173,3 @@ + G_PARAM_READWRITE | + G_PARAM_STATIC_STRINGS); + g_object_class_install_property (maps_class, PROP_ICON, pspec); Gtk+ declares a props array of GParamSpec to then call to g_object_class_install_properties. But I really don't know, maybe both approach are valid.
Review of attachment 293247 [details] [review]: Thanks! I have fixed this for the most part. I did not put braces on new line for enums, from where did you get this style? ::: configure.ac @@ +40,3 @@ +GEOCODE_MIN_VERSION=3.15.2 + +PKG_CHECK_MODULES(GNOME_MAPS_LIB, [ I prefer to show that it is the dependencies for the shim library. So there is one GNOME_MAPS and one GNOME_MAPS_LIB
Created attachment 293735 [details] [review] utils: Handle all loadable icons
Created attachment 293736 [details] [review] Convert searchResultMarker to placeMarker
Created attachment 293737 [details] [review] mapView: Add gotoBBox helper
Created attachment 293738 [details] [review] mapBubble: Do not convert to Place
Created attachment 293739 [details] [review] Add busyMarker module
Created attachment 293740 [details] [review] mainWindow: Add markBusy method
Created attachment 293741 [details] [review] Add c shim library
Created attachment 293742 [details] [review] Add contact module
Created attachment 293743 [details] [review] Add contactMarker class Code shamelessly stolen from Damián Nohales.
Created attachment 293744 [details] [review] Add 'show-contact' action
Created attachment 293779 [details] [review] utils: Handle all loadable icons
Created attachment 293780 [details] [review] Convert searchResultMarker to placeMarker
Created attachment 293781 [details] [review] mapView: Add gotoBBox helper
Created attachment 293782 [details] [review] mapBubble: Do not convert to Place
Created attachment 293783 [details] [review] Add busyMarker module
Created attachment 293784 [details] [review] mainWindow: Add markBusy method
Created attachment 293785 [details] [review] Add c shim library
Created attachment 293786 [details] [review] Add contact module
Created attachment 293787 [details] [review] Add contactMarker class Code shamelessly stolen from Damián Nohales.
Created attachment 293788 [details] [review] Add 'show-contact' action
Created attachment 293789 [details] [review] utils: Handle all loadable icons
Created attachment 293790 [details] [review] Convert searchResultMarker to placeMarker
Created attachment 293791 [details] [review] mapView: Add gotoBBox helper
Created attachment 293792 [details] [review] mapBubble: Do not convert to Place
Created attachment 293793 [details] [review] Add busyMarker module
Created attachment 293794 [details] [review] mainWindow: Add markBusy method
Created attachment 293795 [details] [review] Add c shim library
Created attachment 293796 [details] [review] Add contact module
Created attachment 293797 [details] [review] Add contactMarker class Code shamelessly stolen from Damián Nohales.
Created attachment 293798 [details] [review] Add 'show-contact' action
I'll try to do a more thorough review again as soon as I get rid of this cold. Meanwhile some really low hanging fruit from jshint: src/contact.js: line 23, col 7, Redefinition of '_'. src/mapView.js: line 211, col 37, Expected '===' and instead saw '=='.
Created attachment 294018 [details] [review] utils: Handle all loadable icons
Created attachment 294019 [details] [review] Convert searchResultMarker to placeMarker
Created attachment 294020 [details] [review] mapView: Add gotoBBox helper
Created attachment 294021 [details] [review] mapBubble: Do not convert to Place
Created attachment 294022 [details] [review] Add busyMarker module
Created attachment 294023 [details] [review] mainWindow: Add markBusy method
Created attachment 294024 [details] [review] Add c shim library
Created attachment 294025 [details] [review] Add contact module
Created attachment 294026 [details] [review] Add contactMarker class Code shamelessly stolen from Damián Nohales.
Created attachment 294027 [details] [review] Add 'show-contact' action
Created attachment 294349 [details] [review] utils: Handle all loadable icons
Created attachment 294350 [details] [review] Convert searchResultMarker to placeMarker
Created attachment 294351 [details] [review] mapView: Add gotoBBox helper
Created attachment 294352 [details] [review] mapBubble: Do not convert to Place
Created attachment 294353 [details] [review] Add busyMarker module
Created attachment 294354 [details] [review] mainWindow: Add markBusy method
Created attachment 294355 [details] [review] Add c shim library
Created attachment 294356 [details] [review] Add contact module
Created attachment 294357 [details] [review] Add 'show-contact' action
Review of attachment 294349 [details] [review]: ::: src/utils.js @@ +268,3 @@ } +function _load_icon(icon, loadCompleteCallback) { I'd love to rename all these icon related function =) @@ +274,3 @@ + _load_http_icon(icon, loadCompleteCallback); + return; + } So, the _iconStore doesn't work anymore since the only place where we check for cached icons was removed. So, either we remove _iconStore or we use it only for remote icons. @@ +285,3 @@ } catch(e) { log("Failed to load pixbuf: " + e); + loadCompleteCallback(null); We must be consistent on all the _load_*_icon for this case, where we pass null on fail.
Review of attachment 294355 [details] [review]: Looks better, good work! Behind the MapsContactStorePrivate::list comment, I think it looks good. Two general comments: - Missing Emacs/Vi modelines? - Add an empty line at the end of every file. ::: configure.ac @@ +43,3 @@ +PKG_CHECK_MODULES(GNOME_MAPS_LIB, [ + gee-0.8 >= $GEE_MIN_VERSION + folks >= $FOLKS_MIN_VERSION I just want to note that this make libfolks and gee required dependencies ::: lib/Makefile.am @@ +5,3 @@ + maps-enum-types.h + +libgnome_maps_headers_public = maps-contact-store.h maps-contact.h Missing maps.h? @@ +33,3 @@ + +GnomeMaps-1.0.gir: libgnome-maps.la +GnomeMaps_1_0_gir_NAMESPACE = GnomeMaps No need to specify _NAMESPACE, it will be fetched from gir name ::: lib/maps-contact-store.c @@ +94,3 @@ + G_PARAM_STATIC_STRINGS); + g_object_class_install_property (maps_class, PROP_STATE, pspec); + Empty line @@ +134,3 @@ + avatar); + + while (gee_iterator_has_next (iter)) { Crappy GNU code style needed here :P @@ +230,3 @@ + id, + (GAsyncReadyCallback) maps_contact_store_lookup_cb, + callback); You already have the list of contacts saved in memory, why don't search in that list instead? Would it be more convenient to use a more indexable data structure for priv->list? OTOH, why do we want the priv->list field at all, the maps_contact_store_get_contacts method is never used afterwards, do you have plans to use it in the future? Is efficient to have all those MapsContact objects during the whole program lifetime? Moreover, contacts could be changed, added, removed and looks like the list won't be synchronized. ::: lib/maps-contact.c @@ +115,3 @@ +} + +G_DEFINE_TYPE_WITH_PRIVATE (MapsContact, maps_contact, G_TYPE_OBJECT) Move this to the top @@ +197,3 @@ + * @contact: A #MapsContact object + * @place: A #GeocodePlace object + * Empty line @@ +287,3 @@ + * @contact: A #MapsContact object + * @callback: (scope async): A #MapsContactGeocodeCallback function + * Empty line @@ +291,3 @@ +void +maps_contact_geocode (MapsContact *contact, + MapsContactGeocodeCallback callback) Should we accept GCancellable argument? @@ +296,3 @@ + g_return_if_fail (callback != NULL); + + GeocodeForward *forward; Can be declared inside "for"
Review of attachment 294356 [details] [review]: ::: src/contact.js @@ +27,3 @@ +const Place = imports.place; + +const ContactPlace = new Lang.Class({ I am thinking that if we'd had a setter for GeocodePlace::icon we would not need this class at all (and that is actually implementable in Place.Place) @@ +33,3 @@ + _init: function(params) { + this._avatar = params.avatar; + delete params.avatar; I think passing the whole Contact object instead of the avatar gives us a nicer level of abstraction. @@ +44,3 @@ + get isContact() { + return true; + } What is this for? @@ +51,3 @@ + + _init: function(mapscContact) { + this._mapscContact = mapscContact; We could inherit from MapsContact and... 1) Move the bbox property to MapsContact (renaming it to bounding_box like GeocodeGlib does), we can do the bbox calculations inside the mutex lock in on_geocode_search_async function. 2) Create a getter named resolved_places that returns the places with locations, converted to our ContactPlace class. 3) Remove avatar getter, we can use icon getter (you can rename it in MapsContact if you want) 4) Then, we don't need to implement geocode method here.
Review of attachment 294355 [details] [review]: Thanks for review! General: - Yes, no modelines! If you really want them feel free to add them later. - I do not see any warning about missing line in git, where are you seeing this? ::: configure.ac @@ +43,3 @@ +PKG_CHECK_MODULES(GNOME_MAPS_LIB, [ + gee-0.8 >= $GEE_MIN_VERSION + folks >= $FOLKS_MIN_VERSION Yes, it is unfortunate I guess. But I am not sure we want to have conditionals here. Like only add support if the libraries are there. These are dependencies for gnome-contacts so for a GNOME desktop they should be there, right? ::: lib/Makefile.am @@ +5,3 @@ + maps-enum-types.h + +libgnome_maps_headers_public = maps-contact-store.h maps-contact.h Possibly. Not that important since it does not contain any gir information. @@ +33,3 @@ + +GnomeMaps-1.0.gir: libgnome-maps.la +GnomeMaps_1_0_gir_NAMESPACE = GnomeMaps Allright. ::: lib/maps-contact-store.c @@ +134,3 @@ + avatar); + + while (gee_iterator_has_next (iter)) { Well we could choose not to use it, many glib libraries do not, such as geocode. @@ +230,3 @@ + id, + (GAsyncReadyCallback) maps_contact_store_lookup_cb, + callback); It is used in the allow route to contact bug. Or rather it is used when we want to get all contacts (with addresses available). And right, at the moment it does not get updated. We might want to add that later. I do not think it is that much big of a deal if you have to restart Maps to see changes to your contacts. But it can be a nice to have to hook up to the FolksIndividual signals and update the list in contact-store. And sure one could search list instead of using lookup. It is just that look-up matches 1-1 with the backend, and we get the id as an argument to the show-contact action, so it seemed nice. Also, if you check bug: https://bugzilla.gnome.org/show_bug.cgi?id=736803, there we geocode all contacts with addresses before adding them. My concern was needing to wait until that completes after activating show-contact. So I thought using look_up would ensure a not _too_ long wait. ::: lib/maps-contact.c @@ +115,3 @@ +} + +G_DEFINE_TYPE_WITH_PRIVATE (MapsContact, maps_contact, G_TYPE_OBJECT) sure @@ +197,3 @@ + * @contact: A #MapsContact object + * @place: A #GeocodePlace object + * yep @@ +287,3 @@ + * @contact: A #MapsContact object + * @callback: (scope async): A #MapsContactGeocodeCallback function + * yep @@ +291,3 @@ +void +maps_contact_geocode (MapsContact *contact, + MapsContactGeocodeCallback callback) Not sure when we would use it, to be honest. And it is such a small operation, so I am inclined to not have it. If we feel the need later to abort abort abort geocoding, we can add it. @@ +296,3 @@ + g_return_if_fail (callback != NULL); + + GeocodeForward *forward; Indeed!
Review of attachment 294356 [details] [review]: ::: src/contact.js @@ +27,3 @@ +const Place = imports.place; + +const ContactPlace = new Lang.Class({ Well, I want some way of knowing that this is a contact place, since it needs some special considerations (see allow route to contact bug) as an own way of doing its unique-id so that it does not conflict with other stuff with same osm-id/osm-type. So I think it might as well be an own class. @@ +44,3 @@ + get isContact() { + return true; + } Determining that the place is a contact. I guess one could use instanceof instead if you'd prefer. Mattias? @@ +51,3 @@ + + _init: function(mapscContact) { + this._mapscContact = mapscContact; Well, inheriting from MapsContact seems weird, since then we would construct a MapsContact from MapsContact, but rather we could try to do without this class in JavaScript. Moving the bounding box, like you say. And do the conversion to ContactPlace on the result of the get_places call.
Review of attachment 294349 [details] [review]: Thanks! ::: src/utils.js @@ +268,3 @@ } +function _load_icon(icon, loadCompleteCallback) { Patches welcome! :) @@ +274,3 @@ + _load_http_icon(icon, loadCompleteCallback); + return; + } I say remove it. @@ +285,3 @@ } catch(e) { log("Failed to load pixbuf: " + e); + loadCompleteCallback(null); Yes.
Created attachment 294391 [details] [review] utils: Handle all loadable icons
Created attachment 294392 [details] [review] Convert searchResultMarker to placeMarker
Created attachment 294393 [details] [review] mapView: Add gotoBBox helper
Created attachment 294394 [details] [review] mapBubble: Do not convert to Place
Created attachment 294395 [details] [review] Add busyMarker module
Created attachment 294396 [details] [review] mainWindow: Add markBusy method
Created attachment 294397 [details] [review] Add c shim library
Created attachment 294398 [details] [review] place: Add contact specific functionality
Created attachment 294399 [details] [review] placeStore: Do not store contact places
Created attachment 294400 [details] [review] placeBubble: Do not add favorite button for contacts https://bugzilla.gnome.org/show_bug.cgi?id=736803
Created attachment 294401 [details] [review] Add 'show-contact' action
Review of attachment 294400 [details] [review]: Will remove double bug reference.
Review of attachment 294355 [details] [review]: ::: configure.ac @@ +43,3 @@ +PKG_CHECK_MODULES(GNOME_MAPS_LIB, [ + gee-0.8 >= $GEE_MIN_VERSION + folks >= $FOLKS_MIN_VERSION I don't think it'd be common to have only gnome-maps installed. However, we need to add libfolks as a dependency to jhbuild and distributors needs to add the dependency as well. For the future, we could make libfolks optional.
Review of attachment 294397 [details] [review]: Great. Looks good I think.
Review of attachment 294398 [details] [review]: A really big louse is itching me for the isContact stuff. What about have a ContactPlace inheriting from Place as you had before, you can have a reference to the Contact object in that class. let place = new ContactPlace.ContactPlace({ place: p, contact: contact }); Then you can override the icon and uniqueID getters and you can use instanceof in the following patches, removing isContact method. We can have less "contact noise" in the Place class using this approach. ::: src/place.js @@ +127,3 @@ + return this._icon; + + return this.get_icon(); return this.parent()?
Review of attachment 294398 [details] [review]: I agree, and the approach with instanceof sounds saner, thanks! ::: src/place.js @@ +127,3 @@ + return this._icon; + + return this.get_icon(); I think I tried that first and it didn't work, maybe only works in vfuncs?
Review of attachment 294401 [details] [review]: Nice, some little comments. ::: src/mapView.js @@ +214,3 @@ + this._placeLayer.remove_all(); + places.forEach((function(p) { + let place = new Place.Place( { place: p, Space between ( and { @@ +222,3 @@ + }).bind(this)); + + let place = new Place.Place( { place: p, Oops! :)
Review of attachment 294401 [details] [review]: Thanks! ::: src/mapView.js @@ +214,3 @@ + this._placeLayer.remove_all(); + places.forEach((function(p) { + let place = new Place.Place( { place: p, Yup. @@ +222,3 @@ + }).bind(this)); + + let place = new Place.Place( { place: p, So, no logs in code? :)
Review of attachment 294392 [details] [review]: I like it, it enables us to do more specific implementations for Place based marker/bubbles by just inheriting this. ::: src/searchResultBubble.js @@ +35,3 @@ +const PlaceBubble = new Lang.Class({ + Name: "PlaceBubble", You can do single quotes here on the way :)
Review of attachment 294392 [details] [review]: Thanks! Yeah, that might be nice! ::: src/searchResultBubble.js @@ +35,3 @@ +const PlaceBubble = new Lang.Class({ + Name: "PlaceBubble", Yup, nice catch.
Review of attachment 294393 [details] [review]: Nice.
Review of attachment 294394 [details] [review]: I trust on you! :)
Review of attachment 294394 [details] [review]: This is because if we create a ContactPlace we do not want to convert it to Place here. Since then we can't check for instanceof later, right?
Review of attachment 294395 [details] [review]: I remember the God of designers telling you about bad omen of the busy marker. I don't think is as bad.
Review of attachment 294396 [details] [review]: ::: src/mainWindow.js @@ +90,3 @@ this._restoreWindowGeometry(); + this._busyId = 0; What about _busySignalId? @@ +346,3 @@ + return; + + if (!this._busy) { Is this._busy "declared"? maybe a this._busy = null in the constructor makes the code clearer. But not sure, I think we had the same discussion when developing MapMarker. @@ +361,3 @@ + unmarkBusy: function() { + if (this._busy) + this._busy.show(); I think you are trying to not waste resources by not initializing this._busy on constructor, so why don't you destroy this._busy here? If you want to do that, you could just return when this._busy === null, if you don't, maybe return when this._busyId === 0 would be convenient so you don't call to stage.disconnect(0) afterwards.
Created attachment 295823 [details] [review] Convert searchResultMarker to placeMarker
Created attachment 295824 [details] [review] Add busyMarker module
Created attachment 295825 [details] [review] mainWindow: Add markBusy method
Created attachment 295826 [details] [review] Add c shim library
Created attachment 295827 [details] [review] Add contactPlace module
Created attachment 295828 [details] [review] placeStore: Do not store contact places
Created attachment 295829 [details] [review] placeBubble: Do not add favorite button for contacts
Created attachment 295830 [details] [review] Add 'show-contact' action
Review of attachment 295830 [details] [review]: ::: src/mapView.js @@ +226,3 @@ + places.forEach((function(p) { + let place = new ContactPlace.ContactPlace({ place: p, + return; I would like that ContactPlace accept the contact as parameter instead of icon, maybe it gives us a higher level of abstraction (and it won't consume more memory, they are all references)
Review of attachment 295827 [details] [review]: ::: src/contactPlace.js @@ +31,3 @@ + _init: function(params) { + this._icon = params.icon; + delete params.avatar; typo? @@ +37,3 @@ + + get icon() { + return this._icon; So, this would be "return this.contact.icon"
Attachment 294391 [details] pushed as e06f60c - utils: Handle all loadable icons Attachment 294393 [details] pushed as 107f5ae - mapView: Add gotoBBox helper Attachment 294394 [details] pushed as b7b3a4a - mapBubble: Do not convert to Place Attachment 295823 [details] pushed as 9f7af5e - Convert searchResultMarker to placeMarker Attachment 295824 [details] pushed as 5526995 - Add busyMarker module Attachment 295825 [details] pushed as 1933b20 - mainWindow: Add markBusy method Attachment 295826 [details] pushed as 9d0bce4 - Add c shim library Attachment 295828 [details] pushed as c4f65a8 - placeStore: Do not store contact places Attachment 295829 [details] pushed as fca944a - placeBubble: Do not add favorite button for contacts Attachment 295830 [details] pushed as bbeec47 - Add 'show-contact' action