[patch] basic map widget implementation
Submitted by an unknown user
Link to original bug (#716987)
Description
---- Reported by shotwell-maint@gnome.bugs 2010-11-24 10:45:00 -0800 ----
Original Redmine bug id: 2875
Original URL: http://redmine.yorba.org/issues/2875
Searchable id: yorba-bug-2875
Original author: andreas -
Original description:
Attached patch implements the libchamplain widget into shotwell to display marker pins on a map for geo-coded pictures.
unlike the request for google-maps in #2570 (closed), this actually uses OpenStreetMap maps which are license-wise less problematic and allowed to be embedded. Libchamplain also caches the tiles, so that the markers also display on a map when used offline.
improvement possibilities / TODO (feel free to hack on):
-
Allow for zoom-out (zoom-in works by double-clicking, courtesy of libchamplain). Basically just requires zoom-in/out buttons and signal bindings.
-
Allow for drag-n-drop geo-coding of pictures without geo data
-
Menu option to switch the the different maps that libchamplain can display (cycle map, height map,..).
-
Make the layer clickable (and display pictures by selecting the marker - Easy task)
-
Increase the marker pin size when many pictures on small area instead of cluttering the map with many pins
-
…
On my intel card on ubuntu 10.10 i need to set the environment variable CLUTTER_VBLANK=none to disable vsync or the map will not render! see http://www.google.com/search?q=CLUTTER_VBLANK%3Dnone
so use 'CLUTTER_VBLANK=none shotwell' when testing if it doesn't seem to work.
I sometimes get "BadDrawable" crashes (async X api errors) - not sure what it's about. Might be related to the mesa/clutter issue.
The current clutter-gtk requires a minor fix (patch submitted upstream at https://bugzilla.gnome.org/show_bug.cgi?id=635522 and attached to this ticket)
I'd be happy to get some more feedback (esp. if you encounter the same crashes on systems different from ubuntu 10.10 (11.04 or fedora might be better))
-> Kudos to the libchamplin guys for the widget in the first place!
Related issues:
- related to shotwell - Feature #1473 (closed): geographic tree in sidebar (Open)
---- Additional Comments From shotwell-maint@gnome.bugs 2013-09-17 15:24:00 -0700 ----
History
Comment 1
Updated by andreas - almost 3 years ago
- Keywords changed from map gps to map gps geocoding
Comment 2
Updated by andreas - almost 3 years ago
Of course this feature adds additional dependencies on
-
libchamplain >= 0.4.6
-
clutter >= 1.0
-
clutter-gtk >= 0.10
(might work with lower versions, but these are the ones i'm using)
The occasional crash results in this message on stdout:
The program 'shotwell' received an X Window System error.
This probably reflects a bug in the program.
The error was 'BadDrawable (invalid Pixmap or Window parameter)'.
(Details: serial 57947 error_code 9 request_code 137 minor_code 7)
(Note to programmers: normally, X errors are reported asynchronously;
that is, you will receive the error a while after causing it.
To debug your program, run it with the --sync command line
option to change this behavior. You can then get a meaningful
backtrace from your debugger if you break on the gdk_x_error() function.)
Comment 3
Updated by Adam Dingle almost 3 years ago
Andreas,
it's nice to see you're working on this. I'd love to try out your patch. I tried applying it to the current trunk, but that failed, so probably the trunk has changed since you generated it. I looked at your patch to see which Subversion revision number it had been generated from, but that's not evident since you generated it using git. So please regenerate the patch from the latest Shotwell trunk and resubmit – thanks.
(By the way, Yorba is planning to switch from Subversion to git some time after we ship Shotwell 0.8.)
Comment 4
Updated by Adam Dingle almost 3 years ago
Never mind: I didn't realize that I needed to run 'patch -p1' since there was an extra directory component in filenames in the patch file. The patch applies cleanly to the current trunk. I'll try to build and run it soon.
Comment 5
Updated by andreas - almost 3 years ago
Replying to [comment:5 adam]:
Never mind: I didn't realize that I needed to run 'patch -p1' since there was an extra directory component in filenames in the patch file. The patch applies cleanly to the current trunk. I'll try to build and run it soon.
no problem. git makes it easier to maintain (local) branches which is why i use it over svn.. would love to see a native git repo.
I'll make sure to note the revision next time and indicate which patch level to use.
Comment 6
Updated by Adam Dingle almost 3 years ago
Andreas,
I was finally able to get your code to build and run. I had to make the following changes:
- In the Makefile, I had to change champlain-0.10 to champlain-0.4.
- In add_gps_marker() in Properties.vala, I had to change the calls to get_latitude() and get_longitude() to property accessors (e.g. base_marker.latitude).
Are you really sure that you're using libchamplain 0.4.6 as you mentioned in your comment above?
Also, please update the EXT_PKG_VERSIONS define in the Makefile so that it will check for the minimum required versions of all the libraries you mentioned above.
When I run Shotwell with your changes, I see a libchamplain map in the Basic Information area in the lower left. As soon as I select a photo without GPS info, however, the map goes away. When I then select a photo with GPS info, the map does not return, but I see duplicate sets of text fields in the Basic Information area. Unfortunately I don't seen any GPS marker on the map, even when I select a photo with GPS information. Not sure what's wrong here.
The Shotwell team hasn't yet decided where in our user interface we'd like maps to display. The Basic Information pane is one possibility, but alternatively we might want this to be a separate window. We're currently busy finishing up 0.8, so I think we might not get a chance to discuss this for another week or two. In the meantime, you can keep working on the feature in the Basic Information pane if you like, though we may ask you to move it somewhere else in the interface at some point.
Thanks again for your efforts so far!
Comment 7
Updated by andreas - almost 3 years ago
Hey adam
thanks for the interest and sorry for wasting your time with the library version mixup.. i was trying out how it worked on champlain-0.10-trunk and forgot to switch back (btw. the BadDrawable crash also occurs there for me).
The properties you needed to change to make it compile also work on the trunk so we'll go with that.
The issue with the map not being displayed again on the second time is related to the markers not displaying. Make sure to set the environment variable CLUTTER_VBLANK=none (google the string for more infos).
Good luck on the 0.8 release, I'm looking forward to it.
Comment 8
Updated by andreas - almost 3 years ago
i just rebased on trunk (0.8, assuming it's released based on the downloadable version on the main shotwell page – you really need a tag to pin the release version though)
..just noticed i could've include the version checks to EXT_PKG_VERSIONS.. next time
happy new year :)
Comment 9
Updated by andreas - almost 3 years ago
..ok so the EXT_PKG_VERSIONS are in – the previous rebase needed other 0.8 tweaks anyway..
the “BadDrawable?†crashes are worse for me now – anyone NOT experiencing these -> i'd be interested in video card, driver, distribution, clutter and champlain version
Comment 10
Updated by Adam Dingle over 2 years ago
- Target version set to 0.10
- Priority set to High
We'll think about whether we could take this in some form for 0.10.
Comment 11
Updated by Adam Dingle over 2 years ago
-
Target version deleted (
<strike>
_0.10_</strike>
)
Comment 12
Updated by andreas - over 2 years ago
yay i spent the last two days on the feature, since i really want to see it in shotwell.
I think i finally figured out why i had these bad drawable errors. They seem to be related to some kind of race between the event handlers and the reparenting ops on the gtk widget.. so the solution is to not put it into the table with the other props but rather create a vbox that contains both the table and the map, that solves the existing issue.
Too bad the 0.10 milestone was removed but it's understandable.. Also i wouldn't wanna hold back on all other cool features that are to come..
downside of the current situation on ubuntu is that natty ships with libchamplain 0.8 (instead of 0.10, the current stable which is compatible with 0.11) and requires gtk3 (well clutter-gtk-1.0 requires it, but that's a dependency of libchamplain-0.8) and since gtk3 conflicts with gtk2 on which shotwell depends, it's not actually buildable – doh.
Still that's not a reason to give up so here comes a new patch against the newest libchamplain (0.11 which will end up being stable 0.12) – unfortunately you'll also need to patch libchamplain-0.11 to build against clutter-gtk-0.10 (instead of 1.0) since that still depends on Gtk2 and additionally a patch against the vala bindings of libchamplain.
As a cookie i've added
-
rebased against current master tip
-
zooming out on right-doubleclicks (or use the mousewheel if you have one)
-
add gps tags to the DB since startup was about 5x slower when displaying the library page with all my 1700 pics (half of them with gps tags) – with gps data in the DB it's now on par with an unpatched shotwell
Seems like this ticket will remain dependent on #3455 (closed) (Port to GTK+3)
TODO:
-
if somebody could just help me figuring out why the map widget is not hiding when i call hide() on it, then the map would not constantly be showing (esp. when it's not supposed to).
-
figure out how to update the DB to store the gps data of all existing pics. I'll post that to the mailing list..
-
it doesn't show up as an issue in the profiler performance wise but i'd like to use Clutter.Clone() on the gps marker texture since they're all using the same backing texture and are not modifying it.
-
wait for the gtk3 port to happen (#3455 (closed)) at least with git i don't feel it's going to be such a pain to merge anymore :)
..and as always testing makes your sad days a little brighter :)
Comment 13
Updated by andreas - over 2 years ago
- File add_champlain_map-gtk3_rebase.patch added
- Description updated (diff)
here's a new update, this time (mostly) a rebase against the gtk3 branch - since gtk3 is dependency anyway.
Same stuff as before applies (esp. the NoAccessorMethods patch)
Comment 14
Updated by Sander Deryckere about 2 years ago
- Description updated (diff)
For the case you don't know it, there's a €30 reward for this feature on Elveos (not by me btw, I just saw it and thought I should mention it).
https://elveos.org/en/features/343/contributions?title=show-geolocalied- photos-on-a-map
Comment 15
Updated by andreas - about 2 years ago
..i actually wanted to post that on elveos but I'd need a login.
This feature heavily depends on how fast clutter gets integrated into mainstream distributions (i.e. ubuntu) - and they only plan it for 12.04 precise pangolin. This dependency on clutter is due to libchamplain. So don't expect an upstream shotwell release with this feature within the next 5 months. But as mentioned above, you can of course compile it yourself - it's working but could use some more features.
and of course thanks for the pledges but I'm doing it for fun :) If someone wants to hack on that, feel free to claim the pledge - should it get over the $1000, I'd however ask that at least half the amount be donated to yorba.
cheers
andreas
Comment 16
Updated by andreas - almost 2 years ago
-
File deleted (
<strike>
_fix-clutter-gtk-binding.patch_</strike>
)
Comment 17
Updated by andreas - almost 2 years ago
-
File deleted (
<strike>
_use-gtk_-2.0.patch_</strike>
)
Comment 18
Updated by andreas - almost 2 years ago
-
File deleted (
<strike>
_add_champlain_map-gtk3_rebase.patch_</strike>
)
Comment 19
Updated by andreas - almost 2 years ago
-
File deleted (
<strike>
_add-noaccessormethod.patch_</strike>
)
Comment 20
Updated by andreas - almost 2 years ago
- File add-champlain-map.patch added
Finally - the first no hacks, no external patches release. Just apply, copy the marker png and try.
No additional features in this release, just a rebase to current master
Tested with native Ubuntu Precise Pangolin packages libchamplain-0.12 and libclutter-gtk-1.0-0
Comment 21
Updated by andreas - almost 2 years ago
-
File deleted (
<strike>
_add-champlain-map.patch_</strike>
)
Comment 22
Updated by andreas - over 1 year ago
-
File deleted (
<strike>
_add-champlain-map.patch_</strike>
)
Comment 23
Updated by andreas - over 1 year ago
- File add-champlain-map.patch added
New release which saw a general code overhaul and separates the map stuff from the preferences into its own file/classes.
Runs with the stock ubuntu precise libs.
New features
- Hovering over the markers on the map highlights the thumbnail
- Drag a photo or thumbnail onto the map to set its position (can't drag markers on the map itself)
- Load the marker texture once and reuse it instead of loading it from file for every marker
Caveats:
- Hovering over the thumbnail highlights the marker on the map.. kinda. The code is called, but marker isn't highlighted when using the marker texture, with the simple dot - the fallback when the marker file isn't available - this works.
- Imports of geo tagged photos are slow because the map transition blocks until completed - not sure why as i tell it to cancel. Could be a libchamplain bug.
- DONT USE ON PRODUCTION LIBRARIES AS IT MODIFIES THE DB STRUCTURE
Comment 24
Updated by Roumano - over 1 year ago
It's will be very nice to have gps maps view in the trunk of shotwell
As some patch are available, can anyone review it and/or add a version target ?
Comment 25
Updated by Roumano - over 1 year ago
Hi Andreas,
I want attempt to test your patch as it was for me a important improvement in shotwell ...
I have successfully patched & compile shotwell.
But when i try to launch the patched shotwell i got this crash :
ERROR:/home/roumano/bin/shotwell/src/db/PhotoTable.vala:437:photo_table_get_al l: assertion failed: (tmp3 == SQLITE_OK)
Abandon
Comment 26
Updated by andreas - over 1 year ago
Hi Roumano,
[edit] ..scratch that, i can reproduce it on a db of an actual shotwell instance.
Feel free to try it out on a new db meanwhile (e.g. shotwell -d ~/shotwell- test)
Comment 27
Updated by Lucas Beeler over 1 year ago
- Target version set to 0.13
Thanks for the patch, gentlemen. This is a big patch and there's a fair degree of product management that will need to happen before it can be landed. Those of us on the Shotwell team will have to think very seriously about what we want the Shotwell geo data user experience to look like and whether this patch is in line with our vision. As you guys know, we take user experience very seriously at Yorba.
That said, this is shaping up to be an awesome patch. I'll talk to the rest of the team and we'll see if we can't do something with this for Shotwell 0.13. Keep up the good work!
Lucas
Comment 28
Updated by Adam Dingle over 1 year ago
- Status changed from Review to Open
Andreas,
I just tried your latest patch. This is looking promising - I'd love to see this land in 0.13 if possible.
Issues I saw:
- When I first ran, I saw the same database error Roumano reported.
- When I run with your patch, I see a bunch of critical assertions (in gdk_pixbuf_get_has_alpha, gdk_pixbuf_get_pixels and so on) at startup.
- There should be a separator line between the map and the other items in the Basic Information pane.
- There needs to be a View menu item to show or hide the map.
- It's cool that the map jumps from one place to another when I select different photos, however that sometimes takes a few seconds which can be annoying. I think the animation should always complete within 1.5 seconds or so.
- I saw Shotwell crash twice within a few minutes: "GLib-ERROR **: Creating pipes for GWakeup: Too many open files - Trace/breakpoint trap (core dumped)"
- When I mouse over a photo the corresponding map marker turns red, but when I mouse away it remains red and never goes back to blue.
Please address these issues in the next iteration of your patch. Thanks!
Comment 29
Updated by andreas - over 1 year ago
-
File deleted (
<strike>
_add-champlain-map.patch_</strike>
)
Comment 30
Updated by andreas - over 1 year ago
- File add-champlain-map.patch added
Here's a new patch bumping the db schema - that will cause the db to be upgraded.
again, I seriously recommend using a new test DB as it will mess with the schema! Furthermore the coordinates of pictures are only saved to db on import
- therefor all geotagged images must be removed and reimported anyway for them to show up on the map.
Comment 31
Updated by andreas - over 1 year ago
Thanks Lucas and Adam for your review and consideration
Adam Dingle wrote:
- When I first ran, I saw the same database error Roumano reported.
fixed.
- When I run with your patch, I see a bunch of critical assertions (in gdk_pixbuf_get_has_alpha, gdk_pixbuf_get_pixels and so on) at startup.
i never saw any of these on my setup. ubuntu precise with intel graphics.
- There should be a separator line between the map and the other items in the Basic Information pane.
- There needs to be a View menu item to show or hide the map.
I'm not that much of a UI guy though i might be able to handle that. i was also thinking about an easier and faster way to zoom in and out (though mouse- wheel would probably work). Though these are certainly the last things I'll add.
- It's cool that the map jumps from one place to another when I select different photos, however that sometimes takes a few seconds which can be annoying. I think the animation should always complete within 1.5 seconds or so.
Would have done so if it was configurable. This will require changes in libchamplain and if you plan on releasing 0.13 with ubuntu's stock libchamplain that's not unless it gets updated upstream.
The animation cancellation is also something highly desirable to make imports faster. As a workaround, a switch could probably be added to disable animations during imports.
- I saw Shotwell crash twice within a few minutes: "GLib-ERROR **: Creating pipes for GWakeup: Too many open files - Trace/breakpoint trap (core dumped)"
Never had those either
- When I mouse over a photo the corresponding map marker turns red, but when I mouse away it remains red and never goes back to blue.
Not sure why this is, but I'm getting those too. With the gps-marker.png installed it doesn't highlight at all. Could be a libchamplain issue.
Please address these issues in the next iteration of your patch. Thanks!
As you'll understand, this all depends on the time at my disposition which can strongly vary due to my current job. I try to release as often as possible so that others could pick up at any given time if interest arises. Unfortunately I'm not able to make any promises though, only best effort.
cheers
Comment 32
Updated by Roumano - over 1 year ago
Hi,
Thanks both for the quick respond.
I can run now patched shotwell,
I just have a question i couldn't respond, where is the db file of the patches shotwell ?
if i remove my ~/.shotwell/data/photo.db (and .bak) , i run the patches shotwell still see lot of image, but normal shotwell see it empty ...
I thing i have found a bug to normal shotwell (same issue with patches/unpatched shotwell) :
select all (Ctrl-A) , then edit/remove to library, (only remove to library)
When it at 100%, shotwell hang and never finish : (because it can't remove video of the library...)
Relaunch shotwell, you will see all your video & zero pictures
Comment 33
Updated by Adam Dingle over 1 year ago
Roumano,
in the trunk build of Shotwell, the database is at ~/.local/share/shotwell/data/photo.db .
If you've found an unrelated bug then please file a new ticket - we prefer that each ticket refer to only one bug. Thanks!
Comment 34
Updated by Adam Dingle over 1 year ago
Roumano wrote via email:
if you have a lot of near location, all blue point on the map are like a potetoes & difficult to find what you want/need
I don't known if it possible but it will be great if all near location can be grouped (depending of the zoom view) & degrouped when making a
zoom to the map Like what it's done with the plugin "RV Maps & Earth" on Piwigo website (it's use gmaps as api)
You can have a exemple on my website :http://roumano.com/images/map.php?/categories
I agree. It would be really cool if Shotwell could automatically group nearby locations and ungroup them when you zoom in.
Comment 35
Updated by andreas - over 1 year ago
-
File deleted (
<strike>
_add-champlain-map.patch_</strike>
)
Comment 36
Updated by andreas - over 1 year ago
- File add-champlain-map.patch added
- File gps-marker-selected.png added
Good news for all of you who have been waiting patiently..
This iteration of the patch resolves the technical issues with the previous iterations:
- Importing pictures is not slow as hell because the map first has to move to the marker before being able to continue (+)
- GPS metadata of pictures imported with previous shotwell versions are now updated into the db
- The marker is reset when the cursor is not over the thumbnail any more
as always apply on current master (da6e2fa2)
The first issue was a tough nut to crack.. the problem was that the priority of "idle" queued tasks was lower than the priority libchamplain uses, so the new request (to cancel the current move) didn't get in before the whole transition was done.. i therefor upped the priority of "idle" tasks a bit (reviewers please check that this behavior doesn't have other adverse effects
- i didn't discover any)
The critical assertions (in gdk_pixbuf_get_has_alpha, gdk_pixbuf_get_pixels) come from not having the marker png's installed (put them in the "icons" dir - the source patch only contains the svgs from which the pngs are made, but those are not used!)
Still don't know what the GLib Errors are that adam came across.
Thanks for testing and as said before, if someone wants to tackle the easy GUI tasks that adam wished for: go ahead, it's unlikely i'll do them. I'd rather get the grouping of markers done first.
Comment 37
Updated by Alexander Wilms over 1 year ago
Thanks for you work! It'd be great if Shotwell had a new view featuring a bigger map like Aperture: http://images.apple.com/euro/aperture/images/whatis_organize_places.jpg, maybe combining the normal OSM tiles with a relief layer.
Do you think that something like this could be done for version 0.14?
Comment 38
Updated by Adam Dingle over 1 year ago
- Status changed from Open to Review
Comment 39
Updated by Adam Dingle over 1 year ago
-
Target version deleted (
<strike>
_0.13_</strike>
)
Comment 40
Updated by Adam Dingle over 1 year ago
Alexander: yes, I agree that a bigger map view would be nice. As to what we can achieve for 0.14, well, it depends as always on our resources (which are limited) and the priority of other tasks. Patches are always welcome. :)
Comment 41
Updated by Alexander Wilms over 1 year ago
- File shotwell.svg added
Unfortunately I'm not a developer, I can only do translations & a bit of design.
I created these less skeuomorphic pins, IMHO they'd rather fit into Shotwell's UI:
Comment 42
Updated by andreas - over 1 year ago
Thanks Alexander, they look really good! I'll put in these for the next patch iteration. For that I'd just need a quick written confirmation and a statement that you created them yourself or derived them from a permissible source. (You know, the legal stuff..)
I won't be able to guarantee that they'll be the finally used icons, should the patch be merged into shotwell, but I really do appreciate the effort.
Comment 43
Updated by Alexander Wilms over 1 year ago
I did create these icons myself and license them under cc0. And thanks for including them :)
Comment 44
Updated by andreas - over 1 year ago
-
File deleted (
<strike>
_gps-marker.png_</strike>
)
Comment 45
Updated by andreas - over 1 year ago
-
File deleted (
<strike>
_gps-marker-selected.png_</strike>
)
Comment 46
Updated by andreas - over 1 year ago
-
File deleted (
<strike>
_add-champlain-map.patch_</strike>
)
Comment 47
Updated by andreas - over 1 year ago
- File add-champlain-map.patch added
New patch iteration
- Group markers when they are too close (set to group when < 30px)
- Alexander's new icons included in the diff. No more external .pngs needed.
As always, don't use on your production DB, create a new one with shotwell -d /home/foo/my-test-shotwell-folder
Comment 48
Updated by Roumano - over 1 year ago
Sorry, i can't test anymore (maybe for a while) these patch as the git master of shotwell now require vala 0.17 & in gentoo only .16 is available ....
Comment 49
Updated by andreas - over 1 year ago
The patch also applies to shotwell-0.12.3 (and runs)
patch -p1 < ~/path/to/add-champlain-map.patch
However I only test-compiled it with valac-0.17.4
In other news, it seems that the import is still waiting on the map to finish the transition before moving on.. thought I fixed that
Comment 50
Updated by Roumano - over 1 year ago
It's no compile for me (shotwell-0.12.3) :
/home/roumano/bin/shotwell/src/MapWidget.vala:41: undefined reference to `clutter_actor_get_first_child'
/home/roumano/bin/shotwell/src/MapWidget.vala:41: undefined reference to `clutter_actor_get_first_child'
( i have the clutter version 1.8.4 )
Comment 51
Updated by andreas - over 1 year ago
So i forgot to adjust the minimal version (which is 1.10.0) in the Makefile then - Thanks for trying, fixed it in my branch.
A workaround for clutter 1.8 is probably ClutterGroup.get_nth_child which might need typecasting to ClutterGroup. http://developer.gnome.org/clutter/1.8/ClutterGroup.html#clutter-group-get- nth-child
Comment 52
Updated by Roumano - 12 months ago
Not sure if it's can help, but a another software on Linux make a very good step forward about map implementation : it's darktable software (version 1.1, writing in C)
Now, they can
- see pictures on the map
- with several map source (like openstreetmap, googlemap, google satellite, opencycle map, ...
- degrouping them with make a zoom
- add a gps location by moving a picture (or several) on the map
- add gps location with correlation from a .gpx file
- find a location & update the map with the result (like search london city)
- display gps information of a image (available in 2 format, first for human : N 45°22,771' & the second more userful : in computer format : N 45.3796509)
Comment 53
Updated by Jim Nelson 12 months ago
- Target version set to 0.14.0
Interesting. Let's see if we can get this in for 0.14.
Comment 54
Updated by Jim Nelson 11 months ago
- Category set to library-mode
Comment 55
Updated by Lucas Beeler 11 months ago
- Status changed from Review to Open
- Assignee set to andreas -
@andreas:
I took another look at this patch last night and I really like the functionality of having the map – complete with GPS pins – in the Shotwell info pane. There are, however, two things that I think we need to do to get this patch to the point that it's ready for a comprehensive pre-landing review. First, could you update the diff so that it applies against today's Shotwell master? An even better idea would be for you to set up your own clone of the Shotwell repo on GitHub and keep your work in it, rebasing changes from Shotwell master onto it as needed. This is a great way to keep a patch from going stale. Second, could we avoid storing geolocation information in the Shotwell database and instead just read it out of the PhotoMetadata object that's associated with every Photo as-needed? I ask for this because we go out of our way to minimize database schema changes here at Yorba. Thank you so much for your work. Overall, this looks really promising!
Comment 56
Updated by andreas - 10 months ago
Hi Lucas,
I actually already put it on github a month ago but rebased it against today's master.
https://github.com/abrauchli/shotwell
Two your second request: this will make the whole thing unusable! i tried in the first iterations of this patch, before i decided to put it into the db. Quote from comment 12:
"- add gps tags to the DB since startup was about 5x slower when displaying the library page with all my 1700 pics (half of them with gps tags) – with gps data in the DB it's now on par with an unpatched shotwell"
If it's just about displaying single pictures it'll work and won't make that much of a difference, but even one single event will be much slower if metadata has to be fetched.
You can probably try it out by populating the gps_coords struct with get_gps() instead of database-data in "public GpsCoords get_gps_coords()"@src/photos/PhotoMetadata.vala:999
Also when thinking about expanding this patch for location-based tags it'll be much harder without quick access to location data. One thing that might be worth investigating is moving the data to a separate table. That will avoid column-explosion if even more location-related columns are added in the future (this patch already adds 3 columns) - what i had in mind is adding the country/city/region with reverse-geolookup for automatic location-based tags.
Cheers
Comment 57
Updated by Lucas Beeler 10 months ago
- Status changed from Open to Review
Comment 58
Updated by Lucas Beeler 10 months ago
- Status changed from Review to Open
Hi Andreas,
Jim and I met to discuss how we'd like to proceed with adding geolocation support to Shotwell. Your patch is great work and represents a phenomenal start. In fact, our only serious issue with your patch is that it tries to do too much! ;-) Shotwell has grown into a big product and as it's grown we've taken a more conservative, iterative, incremental approach to integrating new features. We definitely want geolocation support in Shotwell, and we think your work is great. We just want to scale the first iteration of the feature back to the minimal set of functionality that the average Shotwell user will find useful. We've come up with the following design:
(i)
The Gtk+ map widget should appear only in the single photo view (i.e., the view that appears when you double-click on a photo in the thumbnail overview).
(ii)
The map widget should show the location of the current photo (the nice thing about single photo view is that there's only one photo to deal with ;-) ) with a location marking pin. The location marking pin should be centered in the map view. When the user moves forward or backward in the photo sequence (say, by pressing the left or right arrow keys on the keyboard), the map should scroll to show the location of the new current photo. Once again, the location should be marked with a with a centered marking pin.
Comment 59
Updated by Jim Nelson 9 months ago
- Target version changed from 0.14.0 to 0.15.0
The time frame for getting this in to 0.14 is closing. It doesn't look like this will be ready. We'll revisit this for 0.15.
Comment 60
Updated by Jim Nelson 8 months ago
- Target version changed from 0.15.0 to 0.16.0
Comment 61
Updated by Jim Nelson 6 months ago
-
Target version deleted (
<strike>
_0.16.0_</strike>
)
Comment 62
Updated by andreas - 4 months ago
Hi Lucas and Jim,
sorry about the delay, life happened off-the-line.
I've rebased my branch (https://github.com/abrauchli/shotwell) against the latest master in case anybody is interested in using it. During the process I've updated the code to reflect deprecation changes in libchamplain, clutter and gtk3. I'll delete the patch on this ticket as it's growing old.
While I'm still very much interested in seeing this work merged, I do see myself constrained in my available time. I have, thus, currently not implemented any of your suggested changes but I completely agree that this minimalistic approach is well delimited and represents a good introductory step.
Implementing it is mostly about removing (db) code from the current implementation. I'd be happy to review and merge into my repo in case anybody wants to give it a go.
cheers
Comment 63
Updated by andreas - 4 months ago
-
File deleted (
<strike>
_add-champlain-map.patch_</strike>
)
Comment 64
Updated by Jim Nelson 4 months ago
- Status changed from Open to Review
Comment 65
Updated by Joe Bylund 4 months ago
Andreas,
This crashes with
**
ERROR:/home/jbylund/Desktop/shotwell/src/db/PhotoTable.vala:437:photo_table_get_all: assertion failed: (res == Sqlite.OK)
Aborted (core dumped)
on an existing database. I an re-importing some photos on a fresh db to give it a try now, but to me that would be a showstopper.
-Joe
Comment 66
Updated by andreas - 4 months ago
Hey Joe, thanks for testing!
I forgot to bump the hardcoded schema version to trigger the db upgrade and I only tested it with new pristine db or one that already had the columns.
It's fixed in https://github.com/abrauchli/shotwell/commit/6017b2e58e3237c80a7 d4a48dc8199e14df4b8d8
cheers
Comment 67
Updated by Joe Bylund 4 months ago
So at the moment it does not write the gps location to file even if you have writing tags to file enabled? One of the first things I tried was dropping on a map and then publishing to flickr.
Comment 68
Updated by andreas - 4 months ago
Yes that was true. Although it's implemented now.
As side note: while examining what I need to support the writeback I discovered that (around Photo.vala:2382/2410) public void set_comment_persistent and set_title_persistent seem stale. They aren't called from anywhere in the code.
Comment 69
Updated by Joe Bylund 4 months ago
Andreas,
Tested again, worked great. I'll let you know if I see anything else, and let me know if there's anything you'd like me to test. Thank you so much for this patch, I'm super excited to use it.
-Joe
Comment 70
Updated by F M 3 months ago
I have compiled https://github.com/abrauchli/shotwell/ on Ubuntu 13.04 without a problem.
It is a great addition to shotwell, I would be very happy to see this integrated soon.
Comment 71
Updated by Alexander Wilms 3 months ago
I think it would look great if a photo's meta-data and the map widget would be displayed in a side pane like on Google+
Comment 72
Updated by Jens . 2 months ago
I would like to test this and report back but cannot build Shotwell on Ubuntu 12.04. The current daily build does not contain this patch yet (right?). Is it possible to provide a build for Ubuntu 12.04? Thank you!
Comment 73
Updated by Joe Bylund 2 months ago
Jens,
This is not in the current build yet. The easiest way is to
git clone https://github.com/abrauchli/shotwell.git
and then build shotwell as you would otherwise. I recommend backing up your shotwell library before doing this because it triggers a db upgrade, so you can't go back to the current unless you either restore a previous library or wipe your library and start from scratch.
-Joe
Comment 74
Updated by Jens . 2 months ago
As I said - I cannot build Shotwell on Ubuntu 12.04 because there are too many
missing dependancies. I actually cloned the above repo, added the valac PPA
and tried to find libclutter
etc. in up to date versions, but I don't want
to mess with my whole system just because of one application. The only
alternative would be to install a VM with Ubuntu 13.04 or whatever is needed,
I'll do that if there really is no alternative. But in the end I want to get
Shotwell running on my personal machine, which will stay on ubuntu LTS
versions.
Can maybe somebody running Ubuntu 12.04 run fakeroot dpkg-buildpackage -b
and provide the .deb files?
Comment 75
Updated by andreas - 2 months ago
Dear Jens, thank you for your interest.
Please note that this feature is a developer preview intended for developers to enhance the started work for eventual merging into mainline. It is not something that I will (or others should) make available as finished build as it will only create support requests (breaks DB compatibility with main shotwell) and thus burden the yorba team.
Thanks for understanding
Comment 76
Updated by Jens . 2 months ago
I understand!
However, I have a large interest in moving to Shotwell (from iPhoto, btw) and thus I am not so easily scared away. ;)
So I installed an Ubuntu 13.04 VM just to compile Shotwell and have a look. Here are some comments. I know the code in the Git repository is not finished yet and some of my thoughts below create a lot of work - please don't misunderstand. I just want to help by providing some user feedback. Also I'm perfectly willing to test some more if updates appear. I tried several photo managers and Shotwell seems to be the best so far (Digikam is overloaded and installs 180MB of KDE libraries with it, Darkroom is more editor than organizer, FSpot crashes too often, ...).
Shotwell is nice, clean, fast (ok, it hasn't seen my 32'000 120GB photo library yet). This is why I'm here.
So, my comments regarding the map after playing with current CVS for an evening:
-
The map is based on OpenStreetmap. This is good (more detail than e.g. Google map) and bad (less detail than Google satellite view). I think the advantages weigh more but this is perhaps a matter of taste.
-
The map size is adjustable (by adjusting the sidebar width). This is great and it's one major gripe I have with iPhoto - finding locations in a 5x5cm map in a sidebar with a fixed size is horrible. When geotagging, I want to see a larger map than when browsing images.
-
The automatic zoom level is not ideal (yet), for single images the map should zoom further in so that at least individual streets are visible. (In the Google Maps API, which I am familiar with, this would correspond to a zoom level of 15 on a scale of 0..20.)
-
Importing images with existing GPS data takes a lot longer because of the map animation. For me the animation is not necessary and during import the map could also be completely disabled. (Also, what happens if you import without a network connection?)
-
The markers on the map should be movable (adjusting the GPS location) and perhaps even show the GPS location as a tooltip when moving. (Or better yet: the geolocated location name, e.g. street or facility, if possible.)
-
During import of roughly 100 iPhone 4S photos, the current build crashed twice with a segfault (at different photos). If you like I can research this further (eg. compiling with debug symbols), please provide the command line to do this best. Some additional thoughts:
-
The whole idea of geolocation is to do something with this data. So when you have a thousand photos with GPS locations, what is the benefit for the user?
- The most obvious usage is to filter by locality - for example, provide another sidebar tree containing continents, countries, districts, cities, ghettos, streets etc which are in use. To organize this kind of data the first thing coming to my mind is the xAL (extensible Address Language) standard combined with the appropriate IPTC fields (for JPEG data at least). Then, when the user selects an item in the sidebar, update the map view to contain markers for each photo (grouped if too many) and the main view to contain all matching photos. To do this, Shotwell would need to request this data from OSM for each photo and save it locally (otherwise it's probably too slow).
- Also good (and probably relatively easy to implement) would be searching by GPS data (and localities) and creating saved searches based on this data.
- Another nice idea is a travel map: if I have a lot of geotagged photos I'd like to see them displayed on a map in chronological order, like a directions route. Granted this is probably not going to be core functionality, but could this be done as a plugin?
-
Most cameras, especially smartphones, have inaccurate GPS data - so if you take many shots in a single location the GPS locations will be spread out over a few hundred meters / yards. Correcting this is one step in my import workflow. In iPhoto I can manage my locations by name and radius, and imported pictures whose GPS coordinates are within the set radius of a known/saved/named location will automatically be assigned to this location (of course the locations cannot overlap). This is a major work saver. The rest I update by selecting them and typing in a new location name (which is searched via Google). IMHO an equivalent or better solution (in addition to the location manager with a radius option) could be selecting multiple markers on the map and dragging them (together) to a new location.
Thank you for listening. ;)
--- Bug imported by chaz@yorba.org 2013-11-25 21:48 UTC ---
This bug was previously known as bug 2875 at http://redmine.yorba.org/show_bug.cgi?id=2875 Imported an attachment (id=261867)
Unknown version " in product shotwell. Setting version to "!unspecified". Unknown milestone "unknown in product shotwell. Setting to default milestone for this product, "---". Setting qa contact to the default for this product. This bug either had no qa contact or an invalid one. Resolution set on an open status. Dropping resolution CC member joseph.bylund+shotwell@gmail.com does not have an account here
Resolution: RESOLVED FIXED