GNOME Bugzilla – Bug 642557
Display maps in contact preview
Last modified: 2013-09-13 01:10:27 UTC
Created attachment 181115 [details] [review] Integrates maps to contact preview and removes contact-maps plugin This patches are actually replacement for contacts-map plugin. They integrate map into the contact preview pane, displaying it below contact addresses. It's actually not a plugin (addressbook does not provide rich-enough interface to do that as a plugin), but it's done by conditional compilation. The feature is disabled by default for now, because it's not stable enough (requires unstable libchamplain>=0.9 and is not compatible with libchamplain-0.8 stable API). It can be enabled by passing --with-contact-maps=yes option to autoconf. --with-clutter option is required too. The first patch also removes the contacts-map plugin. The second patch inserts "Show maps" option to View -> Preview menu. It allows user to decide whether to display maps in preview or not. The option is unchecked by default and is visible only when --with-contact-maps=yes of course. I added new properties PREVIEW_SHOW_MAPS to EBookShellContent and SHOW_MAPS to EABContactDisplay. Both properties are always compiled, the menu item is only set to be hidden when #ifndef WITH_CONTACT_MAPS.
Created attachment 181116 [details] [review] Adds menu option to turn off the maps Second patch for the menu item
Thanks for a patch. a) Please merge them into one, it's easier to maintain (at least for me) b) Definitely not this way: +#ifdef WITH_CONTACT_MAPS +#include "eab-contact-display-geo.c" +#endif Create an eab-contact-display-geo.h and do there the public interface for it. Then you would do Makefile.am change the right way too (in the new .c file just check for WITH_CONTACT_MAPS on respective places (you might not hide public functions, though). c) the second patch seems to have missing the .ui file change Just an idea, what about, if there will be no WITH_CONTACT_MAPS or it'll be disabled in View menu, add there a link to, say google earth, instead of inline map, so users will be able to get the map somehow? Also, does it do anything when you click on the map preview? If not (I didn't notice in the patch), then what about adding there same URL link to google earth?
d) Err, I forgot, the configure option is --enable-contacts-map, but you call it --with-contacts-map in the configure option help. That might be fixed too.
ad b) I'd rather not compile eab-contact-display-geo.c at all when WITH_CONTACT_MAPS is not defined, since it could fail linking (assume that user who does not want to use this feature does not have libchamplain and libclutter installed). Therefore having #ifdef...#include...#endif seems to be the safest way to do it. Any code that could call the public cm_create_map is safely disabled, because it's called from object-requested signal handler only and it will not be emitted because there are no objects inserted into the HTML when maps are disabled. I'll use header with public interface (I was just too lazy :P), but I don't think that having eab-contact-display-geo.c in code, when WITH_CONTACT_MAPS is not defined, is a good idea. ad d) I can't see it anywhere, only --with-clutter; line number please? ad google link: I really can't handle clicking on map, because mouse release is indistinguishable from map-drag end, but displaying link to Google Maps when maps are disabled should not be a problem.
(In reply to comment #4) > ad b) I'd rather not compile eab-contact-display-geo.c at all... I suppose you'll also do #ifdef ... #endif for whole eab-contact-display-geo.h, so the same can be done for eab-contact-display-geo.c as well. It'll be cleaner than fiddling with Makefile.am. (From my point of view.) > ad d) I can't see it anywhere, only --with-clutter; line number please? Oh, bad of me, I read it incorrectly. I'm sorry for that, forgot of d) please. > ad google link: I really can't handle clicking on map, because mouse release is > indistinguishable from map-drag end, but displaying link to Google Maps when > maps are disabled should not be a problem. Good, please do.
Created attachment 181209 [details] [review] Fixed and merged patch Hehe, and I thought you'd kick me for #ifdefing whole source files :) Anyway public interface of eab-contact-display-geo.c is now in .h file which is included from eab-contact-display.c and content of both -geo files is #ifdef'ed. When maps are disabled, link "Open map" is displayed. Both patches were merged and missing .ui file was added. I hope I didn't miss anything.
map_link in accum_address is not freed. The patch doesn't apply cleanly against actual git master, which is really interesting. When building Google-maps URL, I suggest to use also g_uri_escape_string(), just to make sure texts are passed in the URI properly. And they changed API just recently, because I have the latest git master of libchamplain and I cannot build evolution with it: eab-contact-display-geo.c: In function 'create_champlain_widget': eab-contact-display-geo.c:120:2: error: implicit declaration of function 'champlain_view_get_layout_manager' eab-contact-display-geo.c:120:2: warning: nested extern declaration of 'champlain_view_get_layout_manager' eab-contact-display-geo.c:120:17: warning: assignment makes pointer from integer without a cast eab-contact-display-geo.c:133:2: error: implicit declaration of function 'champlain_location_set_position' eab-contact-display-geo.c:133:2: warning: nested extern declaration of 'champlain_location_set_position' make[5]: *** [libeabwidgets_la-eab-contact-display-geo.lo] Error 1
Created attachment 185087 [details] [review] Fixed patch against libchamplain 0.10.0
(In reply to comment #0) > Created an attachment (id=181115) [details] [review] > Integrates maps to contact preview and removes contact-maps plugin Why do you want to remove the contacts-map plugin? it's not really fair for me who spent some time doing it and improving it (see bug 621516).
(In reply to comment #9) > Why do you want to remove the contacts-map plugin? it's not really fair for me > who spent some time doing it and improving it (see bug 621516). As we discussed on IRC, Dan is not removing it, he is moving it into the core of evolution, for better integration into preview panel than was able to be done through plugin framework. Let's meet with Dan for further discussion on evo's IRC.
Created attachment 187196 [details] [review] Reworked and extended patch This patch provides the same features as the previous patches + two news: The biggest change in this patch is introduction of three new clases: EContactMap: derived from GtkChamplainEmbed, provides interface for adding contacts to map (by passing EContact or name, EContactAddress and EContactPhoto structs) and automatically handles resolving of GPS coords from address. EContactMarker: Virtually ChamplainLabel, the only difference is that it is capable of displaying the text below the image (rather then next to the image, as ChamplainLabel does). EContactMapWindow: A window which embeds EContactMarker, Zoom-in/out buttons and contact search entry with autocompletion. Second change is port of the original Contacts-map plugin (e.g. one big map for whole address-book accessible via address-book popup) to the EContactMapWindow so that it is capable of little more than *just* displaying markers on map. Double-clicking on a marker opens editor for the appropriate contact.
newly created map_link GString wasn't freed in accum_address() modules/addressbook/e-book-shell-view-actions.c may include e-contact-map-window.h only when WITH_CONTACT_MAPS defined, I suppose. The 'book' should be unreffed in map_window_show_contact_editor_cb() on error and at the end, the editor takes reference on it. No need to use gtk_tree_model_get_value() in e-contact-map-window.c when you can get string directly. No need to #include "e-contact-map.h" in e-contact-map-window.h, same as its interface can be available always, even without 'WITH_CONTACT_MAPS'. Typo in contact_map_address_resolved_cb(), you used '&&' instead of '&' beside 'fields'. e-contact-marker.c includes config.h twice. Forgotten "//static void draw_marker (EContactMarker *marker);" in the same file. Rather do not set "*error" in texture_new_from_pixbuf(), callers usually can pass NULL there, it's a legal and pretty common usage of errors. Might the ClutterButtonEvent::click_count does the trick for the double-click recognition? I also noticed that there are different copyright notices in new header and content files. They seem to be different at least. Could you harmonize it a bit, like pick one from evo for header and .c file and use it there, please? Warning spot when compiling with --enable-contact-maps (by the way, you moved the 's' one word to the right, which confused me a bit, before I realized I use wrong configure flag and in what way it is wrong), with gcc 4.6.0: > e-contact-map-window.c: In function 'map_contact_added_cb': > e-contact-map-window.c:138:22: warning: variable 'contact_uid' set but not > used [-Wunused-but-set-variable] By the way, you've right that it stopped warning about "//" comments. I swear it used to. Finally, this is what valgrind claims to me when I select contact from bug #572731 comment #1. Invalid read of size 8 at 0x3061B3B900: __memmove_ssse3_back (in /lib64/libc-2.13.90.so) by 0x306366483A: g_string_append_vprintf (in /lib64/libglib-2.0.so.0.2800.6) by 0x3063664A26: g_string_append_printf (in /lib64/libglib-2.0.so.0.2800.6) by 0x13ABFE34: render_contact_block (eab-contact-display.c:538) by 0x13AC0889: render_contact_vertical (eab-contact-display.c:696) by 0x13AC09F6: render_contact (eab-contact-display.c:727) by 0x13AC0ADB: eab_contact_display_render_normal (eab-contact-display.c:753) by 0x13AC24EC: eab_contact_display_set_contact (eab-contact-display.c:1421) by 0x1AA588A5: e_book_shell_content_set_preview_contact (e-book-shell-content.c:686) by 0x1AA5CE5E: book_shell_view_selection_change_foreach (e-book-shell-view-private.c:92) by 0x691C179: e_bit_array_foreach (e-bit-array.c:210) by 0x59F21E8: esma_foreach (e-selection-model-array.c:264) Address 0x90a6528 is 184 bytes inside a block of size 188 alloc'd at 0x4A064F2: realloc (vg_replace_malloc.c:525) by 0x3061AF78F9: __vasprintf_chk (in /lib64/libc-2.13.90.so) by 0x3063684A9A: g_vasprintf (in /lib64/libglib-2.0.so.0.2800.6) by 0x306366480D: g_string_append_vprintf (in /lib64/libglib-2.0.so.0.2800.6) by 0x3063664A26: g_string_append_printf (in /lib64/libglib-2.0.so.0.2800.6) by 0x13ABFE34: render_contact_block (eab-contact-display.c:538) by 0x13AC0889: render_contact_vertical (eab-contact-display.c:696) by 0x13AC09F6: render_contact (eab-contact-display.c:727) by 0x13AC0ADB: eab_contact_display_render_normal (eab-contact-display.c:753) by 0x13AC24EC: eab_contact_display_set_contact (eab-contact-display.c:1421) by 0x1AA588A5: e_book_shell_content_set_preview_contact (e-book-shell-content.c:686) by 0x1AA5CE5E: book_shell_view_selection_change_foreach (e-book-shell-view-private.c:92) Invalid read of size 8 at 0x3061B3B480: __memmove_ssse3_back (in /lib64/libc-2.13.90.so) by 0x306366483A: g_string_append_vprintf (in /lib64/libglib-2.0.so.0.2800.6) by 0x3063664A26: g_string_append_printf (in /lib64/libglib-2.0.so.0.2800.6) by 0x13AC036C: render_work_block (eab-contact-display.c:595) by 0x13AC08C5: render_contact_vertical (eab-contact-display.c:701) by 0x13AC09F6: render_contact (eab-contact-display.c:727) by 0x13AC0ADB: eab_contact_display_render_normal (eab-contact-display.c:753) by 0x13AC24EC: eab_contact_display_set_contact (eab-contact-display.c:1421) by 0x1AA588A5: e_book_shell_content_set_preview_contact (e-book-shell-content.c:686) by 0x1AA5CE5E: book_shell_view_selection_change_foreach (e-book-shell-view-private.c:92) by 0x691C179: e_bit_array_foreach (e-bit-array.c:210) by 0x59F21E8: esma_foreach (e-selection-model-array.c:264) Address 0x8fbd3d8 is 184 bytes inside a block of size 185 alloc'd at 0x4A064F2: realloc (vg_replace_malloc.c:525) by 0x3061AF78F9: __vasprintf_chk (in /lib64/libc-2.13.90.so) by 0x3063684A9A: g_vasprintf (in /lib64/libglib-2.0.so.0.2800.6) by 0x306366480D: g_string_append_vprintf (in /lib64/libglib-2.0.so.0.2800.6) by 0x3063664A26: g_string_append_printf (in /lib64/libglib-2.0.so.0.2800.6) by 0x13AC036C: render_work_block (eab-contact-display.c:595) by 0x13AC08C5: render_contact_vertical (eab-contact-display.c:701) by 0x13AC09F6: render_contact (eab-contact-display.c:727) by 0x13AC0ADB: eab_contact_display_render_normal (eab-contact-display.c:753) by 0x13AC24EC: eab_contact_display_set_contact (eab-contact-display.c:1421) by 0x1AA588A5: e_book_shell_content_set_preview_contact (e-book-shell-content.c:686) by 0x1AA5CE5E: book_shell_view_selection_change_foreach (e-book-shell-view-private.c:92) Invalid free() / delete / delete[] at 0x4A0556E: free (vg_replace_malloc.c:366) by 0x3063649AC2: g_free (in /lib64/libglib-2.0.so.0.2800.6) by 0x75B8650: e_contact_address_free (e-contact.c:2056) by 0x13AC1E11: contact_display_object_requested (eab-contact-display.c:1210) by 0x70E2283: html_g_cclosure_marshal_BOOLEAN__OBJECT (htmlmarshal.c:81) by 0x306460E2ED: g_closure_invoke (in /lib64/libgobject-2.0.so.0.2800.6) by 0x306461F19F: ??? (in /lib64/libgobject-2.0.so.0.2800.6) by 0x306462872A: g_signal_emit_valist (in /lib64/libgobject-2.0.so.0.2800.6) by 0x3064628B11: g_signal_emit (in /lib64/libgobject-2.0.so.0.2800.6) by 0x70850CE: html_engine_object_requested_cb (gtkhtml.c:549) by 0x70E2283: html_g_cclosure_marshal_BOOLEAN__OBJECT (htmlmarshal.c:81) by 0x306460E2ED: g_closure_invoke (in /lib64/libgobject-2.0.so.0.2800.6) Address 0x2621bbf0 is 0 bytes inside a block of size 1 free'd at 0x4A0556E: free (vg_replace_malloc.c:366) by 0x3063649AC2: g_free (in /lib64/libglib-2.0.so.0.2800.6) by 0x75B8650: e_contact_address_free (e-contact.c:2056) by 0x59CD8FB: resolve_marker_position (e-contact-map.c:185) by 0x59CE05D: e_contact_map_add_marker (e-contact-map.c:330) by 0x13AC1DF9: contact_display_object_requested (eab-contact-display.c:1206) by 0x70E2283: html_g_cclosure_marshal_BOOLEAN__OBJECT (htmlmarshal.c:81) by 0x306460E2ED: g_closure_invoke (in /lib64/libgobject-2.0.so.0.2800.6) by 0x306461F19F: ??? (in /lib64/libgobject-2.0.so.0.2800.6) by 0x306462872A: g_signal_emit_valist (in /lib64/libgobject-2.0.so.0.2800.6) by 0x3064628B11: g_signal_emit (in /lib64/libgobject-2.0.so.0.2800.6) by 0x70850CE: html_engine_object_requested_cb (gtkhtml.c:549) Note that contact contains invalid address, and you main not claim on the console about it, with below critical warning, but rather accept such circumstance without any error. Maybe show some reason instead of a map. > e-contact-map.c-WARNING **: Error while geocoding: no coordinates > WARNING **: Unable to transfer to clutter: Failed to create Cogl texture > Clutter-CRITICAL **: clutter_texture_set_cogl_texture: assertion > `cogl_is_texture (cogl_tex)' failed > Cogl-glx-CRITICAL **: cogl_object_unref: assertion `object != NULL' failed
*** Bug 647752 has been marked as a duplicate of this bug. ***
Created attachment 188912 [details] [review] Fixed patch This patch fixes all issues Milan mentioned in his comment above. The only thing is that I left #include "e-contact-map.h" in e-contact-map-window.h because of e_contact_map_window_get_map(). Everything should be OK now.
Looks good, I committed it. I only did some minor changes in your patch before committing. Created commit 6dc3c69 in evo master (3.1.2+)
I compiled with --enable-contact-maps=yes and --with-clutter=yes. In the 3.1.91 Contacts preview pane I see an "Open map" link going to maps.google.com (though I personally like the service a similar service that is "more free" could be considered). But I don't see "View > Preview > "Show _Maps" in the contacts. The gconf key /apps/evolution/addressbook/display/preview_shop_maps is enabled. I am running evolution in a jhbuild shell. In jhbuild I compiled libchamplain, clutter-gtk, and geoclue. What am I missing?
Make sure WITH_CONTACT_MAPS is defined in your config.h. --enable-contact-maps without the necessary libraries should cause configure to abort with an error message about what you're missing. File a bug if that's not the case. Features should never be silently disabled during configure. Also we should add a line for contact maps to the configure summary. I'll take care of that much.
(In reply to comment #17) > Make sure WITH_CONTACT_MAPS is defined in your config.h. It said /* #undef WITH_CONTACT_MAPS */ here. > --enable-contact-maps without the necessary libraries should cause configure to > abort with an error message about what you're missing. File a bug if that's > not the case. Features should never be silently disabled during configure. I assume that was my case, or something else is fishy here...
(In reply to comment #18) > It said > /* #undef WITH_CONTACT_MAPS */ > here. Yeah, that's the problem. It should say: #define WITH_CONTACT_MAPS 1
Changing /* #undef WITH_CONTACT_MAPS */ to WITH_CONTACT_MAPS 1 manually in config.h finally triggered configure:error: champlain-gtk-0.10 >= 0.10 is required for maps in contacts preview and afterwards everything worked fine. Thanks.