Review of KGeoTag

Tobias Leupold tobias.leupold at
Sun Nov 29 11:26:01 GMT 2020

Hi Albert!

Thanks for your feedback :-)

> It would be good if you could add a OARS
> definition on the appdata.xml there's some stuff out there like flathub
> that enforces having one.

I didn't know about this yet, but it surely won't hurt! Added with

I don't think one would need anything but the empty tag here, as locations are
possibly exchanged with a server (when requesting altitude values), but not
necessarily the local address or such, and even if so, the server doesn't know
this and can't/doesn't relate it with any personal data, right?

> Would it make sense to make File->Add Images also accept folders in addition
> to individual files?

Added an additional action to load all images from a directory with

Correct me if I'm wrong, but choosing either a directory or multiple files
from the same QFileDialog seems to be impossible or at least only achieveable
via hacks ...

Is the "Add all images from directory" option okay for you?

> I'd suggest using KStandardAction::preferences to create your configure
> action, so you get the default icon all apps get,  same for
> KStandardAction::quit i guess.

Changed with b9fddfe0ce04552e9286f55264d63e459efccf3f.

I also added some more standard icons to the menu with

> It's not mandatory, something for the future maybe? but i think it'd be
> nicer if you make your app use kxmlgui, so you don't have to create your
> own KHelpMenu by hand, and you get one one those configure shortcuts by
> free, etc.

I thought about doing this, but the menu is quite small and most probably
won't grow much, also, there's no toolbar or such.

So I was wondering if adding another abstraction level here would make things
actually more simple or more complicated ... I ended up with "for those few
actions, I can as well write it by hand".

I can of course move to an KXmlGuiWindow if you think I really should. Please
tell me!

> You have a few signals marked as const, that is a bit weird

Const declarations for signals are removed with

After quite some years with C++ and Qt, I still don't grasp all concepts ;-)

> You also have a few methods marked as virtual and override, which is also a
> bit weird, since all overriden methods are virtual, so you don't need the
> virtual to be specified.

Fixed with 77d7485ef7a3abab1495f93605d8235b7b5f80e3.

After having read some docs about this (apparently for the first time), I's
clear that "virtual" is for the real initial virtual declaration and not for
the actual override function implementing it.

Thanks for pointing this out! Maybe someday, I'll really know what I do when
coding C++ ;-)

If there's still work to do, please let me know!

Cheers, Tobias

More information about the kde-core-devel mailing list