Koko in KDEReview

[Carl Schwan]

Le mardi, mars 16, 2021 12:55 PM, Harald Sitter <sitter at kde.org> a écrit :

> On Tue, Mar 16, 2021 at 12:49 PM Harald Sitter sitter at kde.org wrote:
>
> > Yay!

Thanks for the big review :)

> >
> > -   please get a bugzilla produce created for it

Not a fan of that. This will ends up exactly like the www bugs, something
that I look into every 6 months. We already have many issues opened in
invent and it's working fine for us. Also we don't use KCrash.

> > -   COPYING-CMAKE-SCRIPTS seems unused

removed

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

Fixed

> > -   kquickimageeditor is found with an empty COMPONENTS list. is that intentional?

I think I fixed it. The CMake code is inspired by some random stuff I found in
the frameworks, this is probably why...

> > -   unless you specifically target kf5.70 it might make sense to bump to
> >     .79 and use KDEGitCommitHooks so clang-format is more consistently
> >     applied

Done

> > -   kdtree.{c,h} references BSD-3-Clause but that license isn't in the source

Done

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

This will be a bit tricky since the remaining files were not written by any
current contributors. I could try to contact the old contributors.

> > -   some classes have trivial constructors/destructors and could use
> >     =default (e.g. roles.cpp, types.cpp, processor.cpp, openfilemodel.cpp,
> >     probably others)

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

Fixed
> >
> > -   some of the .h's have `parent = 0`, it'd be nice to have them use
> >     nullptr instead

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

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

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

This code is now using a ReadWriteLock. This should fix the racing code.

>
> Oh! Three follow ups:
>
> -   is it intentional that this isn't a uniqueapp [1]? do multiple
>     concurrent koko instances even work with the database?

It is intentional since users can open image from Dolphin while Koko is
already running. Maybe I could look into using KDSingleApplication for
this usecase. Also multiple koko instances seems to be working in my
experience but I didn't really test that in details.

>
> -   the geodata magic doesn't seem to be working for me. it correctly
>     maps the geodata in ReverseGeoCoder but in the UI nothing is displayed
>     under locations

Could you take a look into `~/.local/share/koko/imageData.sqlite3` with
sqlitebrowser and tell me if the locations and image are correctly tagged?
Also try to remove `~/.local/share/koko/imageData.sqlite3` and `~/.local/share/koko/fstracker.sqlite3`
after pulling the project new, this might have been caused by the racing
code from above.

>
> -   since it doesn't appear in the UI, I can't check: is the geodata
>     actually getting localized? at least the geocoder seems to spit out
>     non-localized values

Yeah, it's not localized :( Cities and region names are using the local
name and countries are using the English name. It probably make sense to
fix the contries name to be localized but the cities/regions names won't
be realistic since there are just too many of them (~137 562).
>
>     [1] https://api.kde.org/frameworks/kdbusaddons/html/classKDBusService.html
>