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