Koko in KDEReview

[Harald Sitter]

Yay!

- please get a bugzilla produce created for it
- COPYING-CMAKE-SCRIPTS seems unused
- the find_package calls on qt5 probably should be versioned on
whatever version you actually tested with, currently it's unversioned
which I doubt is correct
- same for kquickimageeditor
- kquickimageeditor is found with an empty COMPONENTS list. is that intentional?
- unless you specifically target kf5.70 it might make sense to bump to
.79 and use KDEGitCommitHooks so clang-format is more consistently
applied
- kdtree.{c,h} references BSD-3-Clause but that license isn't in the source
- on the topic of licensing: since the code base is really close to
complete reuse coverage it might be nice to push it over the finishing
line and then `reuse lint` it to not have it fall behind again
- some classes have trivial constructors/destructors and could use
=default (e.g. roles.cpp, types.cpp, processor.cpp, openfilemodel.cpp,
probably others)
- some of those are further QObjects that have a parent signature but
don't delegate construction correctly (i.e. the parent arg is never
forwarded to QObject). since they are also trivial the constructors
could be removed and replaced by `using QObject::QObject;` to adopt
QObject's ctors (e.g. roles.cpp, types.cpp)
- some of the .h's have `parent = 0`, it'd be nice to have them use
nullptr instead
- some functions take references when they should take const refs
(e.g. `void setPath(QString &url)` or the ImageProcessorRunnable ctor)
this suggests they mutate the object, but really the mentioned
functions don't
- for future reference: .appdata.xml is a legacy suffix and
.metainfo.xml ought to be used instead. alas, no point changing this
now to avoid extra work for the i18n team
- the appstream file must use the CDN not a wiki for screenshots
https://invent.kde.org/websites/product-screenshots
- also generally speaking please don't generate or use thumbs for
appstream files, appstream-generator will thumbnail the raw images
during assembling stage on the distro side of things
- appstream data has a help link that goes nowhere

While koko is running, if I copy a file with geodata to the pictures
dir it crashes on `kd_insert3 (tree=0x0,` supposedly because the
geocoder had been deinit()'d while it was still running. most notably
Processor (which is the gui thread) calls deinit on the geocoder
(which is used from various runner threads). in point of fact,
glancing over the code I'm not convinced this is actually correctly
threading....

ImageProcessorRunnable is a runnable that is put into the thread pool.
Inside its run there's

        if (!m_geoCoder->initialized()) {
            m_geoCoder->init();
        }


This is racing code. In between the call to initialized() and the
init() another thread might have done init() already. At best that is
leaking kd_create instances, at worst that crashes on one of the many
asserts on members. On top of that Processor then also calls into
deinit() from its thread which might be at any point in time while the
Runnable's run() runs. The entire construct is lacking any sort of
appreciation for thread synchronization. This needs at least a mutex
to synchronize the init/deinit and possibly lookup if kd_* is not
thread safe.

I am not sure if the init-deinit dance is really adding much value
here. If it isn't I might just do away with the deinit TBH.

HS