[KPhotoAlbum] gps search: a patch

Discussion:

Reimar Imhof

2015-05-31 19:59:55 UTC

Hallo,

here you'll find a patch for a simple gps search.

It's included in the search dialog. The map comes up in standard pan mode -
you can change that to region selection.

If you select a region you'll get images with gps coordinates in that region.
Images without gps info will not be found.

If you want to select images without gps info or don't care about gps you just
don't need to select a region. Or you could press the new "Remove Search
Region" button.

There is one little problem in
KGeoMap::GeoCoordinates DB::ImageInfo::coordinates() const
This runs an exifdb update if it's not version 2.
This update just takes a little time. That's why the first search including
gps info might take some time. And there is no search progress bar...

Enhancements are welcome.

Reimar

Johannes Zarl-Zierl

2015-05-31 20:17:21 UTC

Permalink

Hello Reimar,

Thanks for the patch! Searching for GPS locations is certainly a nice feature.

I'll look into it as soon as possible (which should be some time this week)
and give you some feedback. Maybe Tobias can also give some feedback...

Cheers,
Johannes

Post by Reimar Imhof
Hallo,
here you'll find a patch for a simple gps search.
It's included in the search dialog. The map comes up in standard pan mode -
you can change that to region selection.
If you select a region you'll get images with gps coordinates in that
region. Images without gps info will not be found.
If you want to select images without gps info or don't care about gps you
just don't need to select a region. Or you could press the new "Remove
Search Region" button.
There is one little problem in
KGeoMap::GeoCoordinates DB::ImageInfo::coordinates() const
This runs an exifdb update if it's not version 2.
This update just takes a little time. That's why the first search including
gps info might take some time. And there is no search progress bar...
Enhancements are welcome.
Reimar

Johannes Zarl-Zierl

2015-05-31 21:14:11 UTC

Permalink

Hello again,

I've cleaned the patch up a little for fellow reviewers. I did not yet have
time for any real testing, though.

You added caching of the coordinates in DB::ImageInfo. I'm curious: did you
get significant performance differences with/without caching for searching or
for "regular" MapWidget use?
I'm asking because we initially did the same caching there, but there was no
discernible performance difference...

Cheers,
Johannes

Post by Johannes Zarl-Zierl
Hello Reimar,
Thanks for the patch! Searching for GPS locations is certainly a nice feature.
I'll look into it as soon as possible (which should be some time this week)
and give you some feedback. Maybe Tobias can also give some feedback...
Cheers,
Johannes

Tobias Leupold

2015-05-31 21:17:45 UTC

Permalink

Apparently, we actually had a lookm at this at the very same time ... I pushed
a cleaned up (but otherwise unchanged) version of the patch to the new
gpssearch branch, so that everybody can have a look at it. ;-)

Post by Johannes Zarl-Zierl
Hello again,
I've cleaned the patch up a little for fellow reviewers. I did not yet have
time for any real testing, though.
You added caching of the coordinates in DB::ImageInfo. I'm curious: did you
get significant performance differences with/without caching for searching
or for "regular" MapWidget use?
I'm asking because we initially did the same caching there, but there was no
discernible performance difference...
Cheers,
Johannes

Reimar Imhof

2015-06-01 19:58:10 UTC

Permalink

Hello Johannes,

after Tobias put his version of the patch to the gps git branch I had a look
at that one.

To Your question about caching.
I've just recompiled kphotoalbum without caching.
gps searching took me about 15 to 20 sec (no cache).
With caching first search run took about 2 sec, any following searches I just
couldn't measure - the answer came immediately.
I also go the impression that opening a lot images (about 1500 in my largest
folder) in the annotation dialog is much faster with caching.

Some numbers: I've got about 16800 pictures, 2500 of them have gps position
info. So searching for gps info has to look at all the pictures.
Hardware is an about 5 year old i5 with hdd, no ssd, 4gb ram.

Cheers
Reimar

Post by Reimar Imhof
Hallo,
here you'll find a patch for a simple gps search.
It's included in the search dialog. The map comes up in standard pan
mode
-
you can change that to region selection.
If you select a region you'll get images with gps coordinates in that
region. Images without gps info will not be found.
If you want to select images without gps info or don't care about gps you
just don't need to select a region. Or you could press the new "Remove
Search Region" button.
There is one little problem in
KGeoMap::GeoCoordinates DB::ImageInfo::coordinates() const
This runs an exifdb update if it's not version 2.
This update just takes a little time. That's why the first search including
gps info might take some time. And there is no search progress bar...
Enhancements are welcome.
Reimar

Johannes Zarl-Zierl

2015-06-01 20:29:58 UTC

Permalink

Hi Reimar,

Post by Reimar Imhof
after Tobias put his version of the patch to the gps git branch I had a look
at that one.

No problem.

Post by Reimar Imhof
To Your question about caching.
I've just recompiled kphotoalbum without caching.
gps searching took me about 15 to 20 sec (no cache).
With caching first search run took about 2 sec, any following searches I
just couldn't measure - the answer came immediately.
I also go the impression that opening a lot images (about 1500 in my largest
folder) in the annotation dialog is much faster with caching.
Some numbers: I've got about 16800 pictures, 2500 of them have gps position
info. So searching for gps info has to look at all the pictures.
Hardware is an about 5 year old i5 with hdd, no ssd, 4gb ram.

Thanks for the numbers!

Cheers,
Johannes

Tobias Leupold

2015-05-31 22:17:17 UTC

Permalink

Hi Reimar!

I just had a quick look at your patch (actually at the cleaned up version in
the gpssearch branch). Looks very nice :-) Only thing I changed is to hide the
mode selection when the annotation dialog is opened in annotation mode, as we
don't need the selection there (to keep the UI clean and as simple as
possible).

Just speaking of the UI: There's a QToolButton* removeCurrentSelectionButton
inside the KGeoMap widget connected with an action called
"actionRemoveCurrentRegionSelection", with the text "Remove the current region
selection". Apparently, this is intended to do the very same as your newly
added "m_deleteSearchRegionButton" button.

(At least here) It's always disabled. Do you have an idea how to enable it (I
didn't manage to do so ;-) This way, we could cheap out the extra button.

Cheers, Tobias

Reimar Imhof

2015-06-01 20:15:42 UTC

Permalink

Hi Tobias,

thanks a lot for looking at my patch!

I've seen the removeCurrentSelectionButton but I couldn't find a way using it.
I think it's used in the more complex digikam gps search, that allows to
search for more complex areas, if I got that right. I didn't try to adopt that
- I'm happy with what I have ;-)
I've also seen that this button is always disabled, and I couldn't find a way
to at least get rid of it or even use it. Well, I've just forgotten to ask for
how to use that button - so I can't help you.

To me the one point to deal with is the performance problem when the gps
access triggers an exiv db update. I mentioned that yesterday. I didn't
measure that time but it just felt like taking ages.
I could think about two possibilities:
- having a search progress bar - meaning the first search including that
update will take some time
or
- convert the exiv db during start up (like looking for new pictures) - so the
first start up could by time expensive.

What do you think?

Cheers
Reimar

Post by Tobias Leupold
Hi Reimar!
I just had a quick look at your patch (actually at the cleaned up version in
the gpssearch branch). Looks very nice :-) Only thing I changed is to hide
the mode selection when the annotation dialog is opened in annotation mode,
as we don't need the selection there (to keep the UI clean and as simple as
possible).
Just speaking of the UI: There's a QToolButton* removeCurrentSelectionButton
inside the KGeoMap widget connected with an action called
"actionRemoveCurrentRegionSelection", with the text "Remove the current
region selection". Apparently, this is intended to do the very same as your
newly added "m_deleteSearchRegionButton" button.
(At least here) It's always disabled. Do you have an idea how to enable it
(I didn't manage to do so ;-) This way, we could cheap out the extra
button.
Cheers, Tobias

_______________________________________________
KPhotoAlbum mailing list
https://mail.kdab.com/mailman/listinfo/kphotoalbum

Johannes Zarl-Zierl

2015-06-01 20:45:50 UTC

Permalink

Hi,

Post by Reimar Imhof
To me the one point to deal with is the performance problem when the gps
access triggers an exiv db update. I mentioned that yesterday. I didn't
measure that time but it just felt like taking ages.

(1)

Post by Reimar Imhof
- having a search progress bar - meaning the first search including that
update will take some time
or

(2)

Post by Reimar Imhof
- convert the exiv db during start up (like looking for new pictures) - so
the first start up could by time expensive.

For me, neither option is ideal. Option 2 (update on startup) will be quite
frustrating, especially for users who don't use geotagging.

Option 1 is potentially better, but I would prefer it if the user gets a
choice as soon as he/she enables the gps search.

I see at least one other option:
(3) Do not trigger updates to the exiv DB, and ignore missing gps values

A definite upside to option 3 is a low performance overhead. The downside that
should not be understated lies in the potential confusion of the users.

Johannes

Reimar Imhof

2015-06-03 17:41:52 UTC

Permalink

Hi Johannes,

if we wanted Option 2 (update on startup) it needed to include a question "Do
you want to prepare for gps search? It could take some time." (or something
like that).

About option 1: How about a progress bar with cancel button? After pressing
cancel there could be an info about the implicit gps preparation saying
searching will be fast if not searching for gps position info and searching
will be fast again after the first gps search has been done.

To option 3: The automatic exiv update is already (kphotoalbum 4.6.2)
triggered when opening the annotation dialog. It just deals with the shown
pictures and does not convert the exiv info for all pictures.

What about an option 4: Have a new checkbox in the kphotoalbum options dialog
"enable gps search". When doing so it could ask if the gps info should be
converted because it's time consuming. Option 2 and 4 could be combined.

Perhaps option 1 (progress bar with cancel button) could also be useful if a
search process just takes a little more time. For example on my machine the
first gps search costs about 2 seconds. When I've taken more pictures those 2
seconds will become 3, 4 or 5 seconds. And that's to long for a program not
showing any response.
I'd like to have a progress bar (including cacnel option).

What do you think?

Cheers
Reimar

I'd prefer an gps yes/no choice in the kphotoalbum option dialog

Post by Johannes Zarl-Zierl
Hi,

(1)

Post by Reimar Imhof
- having a search progress bar - meaning the first search including that
update will take some time
or

(2)

Post by Reimar Imhof
- convert the exiv db during start up (like looking for new pictures) - so
the first start up could by time expensive.

For me, neither option is ideal. Option 2 (update on startup) will be quite
frustrating, especially for users who don't use geotagging.
Option 1 is potentially better, but I would prefer it if the user gets a
choice as soon as he/she enables the gps search.
(3) Do not trigger updates to the exiv DB, and ignore missing gps values
A definite upside to option 3 is a low performance overhead. The downside
that should not be understated lies in the potential confusion of the
users.
Johannes
_______________________________________________
KPhotoAlbum mailing list
https://mail.kdab.com/mailman/listinfo/kphotoalbum

Johannes Zarl-Zierl

2015-06-04 11:07:18 UTC

Permalink

Hi,

Personally, option 2+4 has the most appeal to me.

Post by Reimar Imhof
if we wanted Option 2 (update on startup) it needed to include a question
"Do you want to prepare for gps search? It could take some time." (or
something like that).
About option 1: How about a progress bar with cancel button? After pressing
cancel there could be an info about the implicit gps preparation saying
searching will be fast if not searching for gps position info and searching
will be fast again after the first gps search has been done.

My gut feeling is that this ends up being frustrating to users.

Post by Reimar Imhof
To option 3: The automatic exiv update is already (kphotoalbum 4.6.2)
triggered when opening the annotation dialog. It just deals with the shown
pictures and does not convert the exiv info for all pictures.

I'm aware of that. The rationale for this was that the annotation dialog is
mostly used with relatively small subsets of the general pictures. I.e. you
invest the time in small chunks instead of a single multi-hour investment.

Post by Reimar Imhof
What about an option 4: Have a new checkbox in the kphotoalbum options
dialog "enable gps search". When doing so it could ask if the gps info
should be converted because it's time consuming. Option 2 and 4 could be
combined.

Having a config option seems reasonable - you can enable the gps search by
default for new users only. Re-reading the exif db could also enable gps
search.

You don't even need to make it a checkbox in the settings dialog - you could
just make it a button on the map widget of the search dialog.
Once you turn it on and complete the exif db transformation, it doesn't make
sense to turn it off again. Or does it?

Post by Reimar Imhof
Perhaps option 1 (progress bar with cancel button) could also be useful if a
search process just takes a little more time. For example on my machine the
first gps search costs about 2 seconds. When I've taken more pictures those
2 seconds will become 3, 4 or 5 seconds. And that's to long for a program
not showing any response.
I'd like to have a progress bar (including cacnel option).

That would be an individual feature unrelated to the gps search. I can see the
value of it, though...

Cheers,
Johannes

Tobias Leupold

2015-06-04 12:19:40 UTC

Permalink

Post by Johannes Zarl-Zierl
You don't even need to make it a checkbox in the settings dialog - you could
just make it a button on the map widget of the search dialog.
Once you turn it on and complete the exif db transformation, it doesn't make
sense to turn it off again. Or does it?

We are only talking of a one-time rebuild of the EXIF db, aren't we? So why
not simply enable it for new setups (as the EXIF db will be filled with GPS
data anyway in this case) and add a button "Enable GPS search". If the user
clicks on this one, we display a message:

"Enabling the GPS search requires a rebuild of the EXIF database. This process
can take some minutes. Do you want to run the rebuild now? --> Yes / No".

If that update is done, we add some value to the config (which is set by
default for new setups) and don't display that button anymore.

And that's it. Or am I missing/misunderstanding something?

Cheers, Tobias

Johannes Zarl-Zierl

2015-06-04 12:37:35 UTC

Permalink

Post by Tobias Leupold

Post by Johannes Zarl-Zierl
You don't even need to make it a checkbox in the settings dialog - you
could just make it a button on the map widget of the search dialog.
Once you turn it on and complete the exif db transformation, it doesn't
make sense to turn it off again. Or does it?

We are only talking of a one-time rebuild of the EXIF db, aren't we? So why
not simply enable it for new setups (as the EXIF db will be filled with GPS
data anyway in this case) and add a button "Enable GPS search". If the user
"Enabling the GPS search requires a rebuild of the EXIF database. This
process can take some minutes. Do you want to run the rebuild now? --> Yes
/ No".
If that update is done, we add some value to the config (which is set by
default for new setups) and don't display that button anymore.

That config value is exactly the thing I was talking about. Hence the "Re-
reading the exif db could also enable gps search."

Johannes

Tobias Leupold

2015-06-04 13:00:03 UTC

Permalink

Post by Johannes Zarl-Zierl
That config value is exactly the thing I was talking about. Hence the "Re-
reading the exif db could also enable gps search."

Even better :-) So we actually only do have to add that one button, which will
be visible or not, don't we?

Tobias Leupold

2015-06-03 19:26:37 UTC

Permalink

Post by Johannes Zarl-Zierl
(3) Do not trigger updates to the exiv DB, and ignore missing gps values

I think this is a good idea. With the current implementation, the GPS data is
added to the db as soon as new images are found or geotagged via KIPI from
inside KPA.

So actually, we do just "short-circuit" users that know what they're doing
(and actually using the whole GPS thing). We could say "If you want to use
this for all your images, run a rebuild of the EXIF db". And here we go. No
frustrated users who don't use it anyway, and those who want to use it will
simply wait for that update to be done.

No problem for new users, as the GPS values are added to the db anyway.

Tobias Leupold

2015-06-03 19:21:46 UTC

Permalink

Post by Reimar Imhof
I've seen the removeCurrentSelectionButton but I couldn't find a way using
it. I think it's used in the more complex digikam gps search, that allows
to search for more complex areas, if I got that right. I didn't try to
adopt that - I'm happy with what I have ;-)
I've also seen that this button is always disabled, and I couldn't find a
way to at least get rid of it or even use it. Well, I've just forgotten to
ask for how to use that button - so I can't help you.

I asked the Digikam/libkgeomap guys about that. Let's see if we get an answer
;-)

Reimar Imhof

2015-06-13 10:39:06 UTC

Permalink

Hi Tobias, hi Johannes,

thanks for your work on the gps search.

There are two patches I suggest to revert them:

82a56c0 "In search mode, center the map so that it will be completely visible
when starting the dialog."
If you have subsequent searches I'd like the map not to move to an initial
position. I'd like to stay at the same position and do not need to navigate to
my last position again. And even when starting the first gps search after
kphotoalbum start: I like it being taken to the last map position I was using.

ad318d3 "Hide the setLastCenterButton button in the search dialog, because
it's not meaningful there."
If that patch 82a56c0 was removed, showing the setLastCenterButton again could
just bring the center functionality. You could say "Bring me back" and press
the button. Or you don't and just don't press the button.

What do you think?

Cheers
Reimar

Post by Tobias Leupold

I asked the Digikam/libkgeomap guys about that. Let's see if we get an
answer ;-)
_______________________________________________
KPhotoAlbum mailing list
https://mail.kdab.com/mailman/listinfo/kphotoalbum