On Tue, Mar 16, 2021 at 12:49 PM Harald Sitter <sit...@kde.org> wrote:
>
> 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
Oh! Three follow ups:
- is it intentional that this isn't a uniqueapp [1]? do multiple
concurrent koko instances even work with the database?
- 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
- 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
[1] https://api.kde.org/frameworks/kdbusaddons/html/classKDBusService.html
