[prev in list] [next in list] [prev in thread] [next in thread] 

List:       kde-core-devel
Subject:    Re: Koko in KDEReview
From:       Carl Schwan <carl () carlschwan ! eu>
Date:       2021-04-22 23:00:12
Message-ID: 7XBZgMI_S2VuPlLe3-KR3hMAXTId1AbKxk8ULQX_k_1SVRT7tNHR1TFmsruYVcSVAL6i7AiUYFklp1HJb8WC-4NYHio0k_J4s98KFt7Yn6Y= () carlschwan ! eu
[Download RAW message or body]

Le mardi, mars 23, 2021 1:31 PM, Harald Sitter <sitter@kde.org> a \
écrit&nbsp;:

> On 16.03.21 20:10, Carl Schwan wrote:
> 
> > Le mardi, mars 16, 2021 12:55 PM, Harald Sitter sitter@kde.org a \
> > écrit&nbsp;: 
> > > 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, \
> > 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@kde.org>Le mardi, mars 23, 2021 1:31 PM, Harald Sitter \
<sitter@kde.org> a écrit&nbsp;:

> On 16.03.21 20:10, Carl Schwan wrote:
> 
> > Le mardi, mars 16, 2021 12:55 PM, Harald Sitter sitter@kde.org a \
> > écrit&nbsp;: 
> > > 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, \
> > 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@kde.org>


[prev in list] [next in list] [prev in thread] [next in thread]