Koko in KDEReview

[Harald Sitter]

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

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

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