Review of KGeoTag

[Albert Astals Cid]

El diumenge, 29 de novembre de 2020, a les 12:26:01 CET, Tobias Leupold va escriure:
> 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?

sounds reasonable.

> > 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?

That's nice, thanks :)

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

Cool, icons in menus make the app look a bit less dull :)

> 
> > 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!

I'm a bit biased, but i think it's better to have, like what if i'm an avid user and want to set a shortcut to the add file or to Save images actions?

I can't because you're not using kxmlgui that gives me that for free.

As said, it's not mandatory, but it gives niceties that makes apps "more consistent" amongst eachother.

Also, you may want to add a KCrash::initialize call in your main to initialize kcrash and get the nice "this thing crashed report a bug" dialog.

Cheers,
  Albert

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