Koko in KDEReview

Fri Apr 23 00:00:12 BST 2021

Le mardi, mars 23, 2021 1:31 PM, Harald Sitter <sitter at kde.org> a écrit :

> On 16.03.21 20:10, Carl Schwan wrote:
>
> > 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.
>
> Did I miss something? The last agreement I recall was that we are using
> bugzilla for bugs and gitlab for tasks for the time being. If even as
> developer I have to go hunting where $project tracks their bugs I'm sure
> not going to be a happy camper.
>
> > Also we don't use KCrash.
>
> Shouldn't you?
>
> > > > 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.
>
> Still crashes when I move an image with geodata into the picture dir :(
>
> at ../src/reversegeocoder.cpp:151

I think I fixed it with https://invent.kde.org/graphics/koko/-/commit/d2c51a814fcc1646003338d1dbcbfb1aa4126d3f
>
> > > 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.
>
> That's what [1] does.
> You register on kdbusservice register as org.kde.koko and connect to
> activateRequested so when the user clicks on an image while koko is
> already running that opening request is sent to the running instance
> instead of starting a completely new one.

Finally found the time to implement it: https://invent.kde.org/graphics/koko/-/merge_requests/65/diffs :D
>
> > > -   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.
>
> Works after I wiped config and data to start with a fresh set of data
> :shrug:
>
> HS

</sitter at kde.org>Le mardi, mars 23, 2021 1:31 PM, Harald Sitter <sitter at kde.org> a écrit :

> On 16.03.21 20:10, Carl Schwan wrote:
>
> > 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.
>
> Did I miss something? The last agreement I recall was that we are using
> bugzilla for bugs and gitlab for tasks for the time being. If even as
> developer I have to go hunting where $project tracks their bugs I'm sure
> not going to be a happy camper.
>
> > Also we don't use KCrash.
>
> Shouldn't you?

The problem is that DrKonqi doesn't really work on Plasma Mobile and I don't think
this justify getting locked up in using Bugzilla. If having a bugzilla product
is really required, we can request one but I can't guarantee that I will look at it
more often than I look at the kde-www bug reports in bugzilla (every 6 months).

> > > > 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.
>
> Still crashes when I move an image with geodata into the picture dir :(
>
> at ../src/reversegeocoder.cpp:151

I think I fixed it with https://invent.kde.org/graphics/koko/-/commit/d2c51a814fcc1646003338d1dbcbfb1aa4126d3f
>
> > > 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.
>
> That's what [1] does.
> You register on kdbusservice register as org.kde.koko and connect to
> activateRequested so when the user clicks on an image while koko is
> already running that opening request is sent to the running instance
> instead of starting a completely new one.

Finally found the time to implement it: https://invent.kde.org/graphics/koko/-/merge_requests/65/diffs
>
> > > -   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.
>
> Works after I wiped config and data to start with a fresh set of data
> :shrug:
>
> HS

</sitter at kde.org>