Review of KGeoTag

[Tobias Leupold]

Hi Albert!

Thanks for your feedback :-)

> It would be good if you could add a OARS https://hughsie.github.io/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
b826d11b1099540879a7a5ca00d5fe1da9694a22.

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
c4f2c5e2ac7b776c63afb644a35bcd70f52fcd0c.

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
18e305067ccaa1af78c24c7d97129def7fa67cc5.

> 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
> 	https://invent.kde.org/sdk/clazy/-/blob/master/docs/checks/README-const-sig
> nal-or-slot.md

Const declarations for signals are removed with
042631ebc763ae704d067dfacc98a5979d357205.

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