From kde-core-devel Thu Apr 22 23:00:12 2021 From: Carl Schwan Date: Thu, 22 Apr 2021 23:00:12 +0000 To: kde-core-devel Subject: Re: Koko in KDEReview Message-Id: <7XBZgMI_S2VuPlLe3-KR3hMAXTId1AbKxk8ULQX_k_1SVRT7tNHR1TFmsruYVcSVAL6i7AiUYFklp1HJb8WC-4NYHio0k_J4s98KFt7Yn6Y= () carlschwan ! eu> X-MARC-Message: https://marc.info/?l=kde-core-devel&m=161913244715161 Le mardi, mars 23, 2021 1:31 PM, Harald Sitter a =C3=A9cri= t : > On 16.03.21 20:10, Carl Schwan wrote: > > > Le mardi, mars 16, 2021 12:55 PM, Harald Sitter sitter@kde.org a =C3= =A9crit : > > > > > On Tue, Mar 16, 2021 at 12:49 PM Harald Sitter sitter@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, somethi= ng > > 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 picture= s > > > > dir it crashes on `kd_insert3 (tree=3D0x0,` supposedly because the > > > > geocoder had been deinit()'d while it was still running. most notab= ly > > > > 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 po= ol. > > > > 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 ma= ny > > > > 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 mute= x > > > > 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/d2c51= a814fcc1646003338d1dbcbfb1aa4126d3f > > > > 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/kok= o/-/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 disp= layed > > > 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 tagg= ed? > > Also try to remove `~/.local/share/koko/imageData.sqlite3` and `~/.loca= l/share/koko/fstracker.sqlite3` > > after pulling the project new, this might have been caused by the racin= g > > code from above. > > Works after I wiped config and data to start with a fresh set of data > :shrug: > > HS Le mardi, mars 23, 2021 1:31 PM, Harald Sitter a =C3=A9crit : > On 16.03.21 20:10, Carl Schwan wrote: > > > Le mardi, mars 16, 2021 12:55 PM, Harald Sitter sitter@kde.org a =C3= =A9crit : > > > > > On Tue, Mar 16, 2021 at 12:49 PM Harald Sitter sitter@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, somethi= ng > > 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 prod= uct is really required, we can request one but I can't guarantee that I will lo= ok at it more often than I look at the kde-www bug reports in bugzilla (every 6 mont= hs). > > > > While koko is running, if I copy a file with geodata to the picture= s > > > > dir it crashes on `kd_insert3 (tree=3D0x0,` supposedly because the > > > > geocoder had been deinit()'d while it was still running. most notab= ly > > > > 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 po= ol. > > > > 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 ma= ny > > > > 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 mute= x > > > > 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/d2c51= a814fcc1646003338d1dbcbfb1aa4126d3f > > > > 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/kok= o/-/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 disp= layed > > > 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 tagg= ed? > > Also try to remove `~/.local/share/koko/imageData.sqlite3` and `~/.loca= l/share/koko/fstracker.sqlite3` > > after pulling the project new, this might have been caused by the racin= g > > code from above. > > Works after I wiped config and data to start with a fresh set of data > :shrug: > > HS