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

List:       kde-core-devel
Subject:    Re: Move Koko to KDEReview
From:       Carl Schwan <carl () carlschwan ! eu>
Date:       2020-06-12 16:05:36
Message-ID: piPe4I92B9ojN4n5Lja6MX8rsGjOBuULX-K7szxppRGcwlt4XHP6v-vI7eoNY7GftgA9HbIsKVpx4UDH3mKaQIcCePY3D0isb8CsZQdiOG4= () carlschwan ! eu
[Download RAW message or body]

[Attachment #2 (multipart/mixed)]


Le jeudi, juin 11, 2020 9:43 PM, Albert Astals Cid <aacid@kde.org> a =C3=A9=
crit=C2=A0:

> El dimarts, 9 de juny de 2020, a les 13:30:35 CEST, Carl Schwan va escri=
ure:
> =


> > Hi,
> > I would like to move Koko, a convergent image viewer, to KDEReview.
> > Koko is already shipped in the base Plasma mobile image and I was
> > surprised that it was still in playground. The current devs are mostly
> > Nicolas, Marco and me.
> =


> Is this baloo based? I guess it would explain why I can hardly see any i=
mages.
> Ah no, it only lists images from the "Pictures" folder, i see, kind of w=
eird for a desktop app.

This is the default behavior in Digikam and KPhotoAlbum, the only differen=
ce is
that these apps allow changing this behavior. I guess it would make sense =
to make
it configurable (for the case the image collection is located in an extern=
al drive
for example) but this is not my priority yet.

> I think you have a memory leak in FileSystemTracker::reindexSubFolder, t=
here's a FileSystemImageFetcher new'ed and i can't see it being deleted.
> =


> > From the release sanity checklist:
> > =


> > -   licensing should be ok (LGPL-2.1-only or LGPL-3.0-only or
> >     LicenseRef-KDE-Accepted-LGPL), but some headers are missing in the
> >     CMake files :/
> >     =


> > -   A Messages.sh file is missing and help would be welcome to figure
> >     out if Koko need one since translations are regularly being pushed=
 by
> >     scripty.
> >     =


> =


> Yes you need one, Yuri already added it.
> =


> What you also need and you don't have is a call to KLocalizedString::set=
ApplicationDomain("koko"); in your main.cpp

This was added in https://invent.kde.org/plasma-mobile/koko/-/merge_reques=
ts/25 and the
Kirigami AboutPage component was also added at the same time :)

> =


> > -   Screenshot is missing but I plan to add one before the release.
> > -   CI works and there is a .gitlab-ci.yml file.
> > -   There is an AppStream file.
> > -   There is some documentation on userbase: https://userbase.kde.org/=
Koko
> >     I plan to also update it before the next release.
> >     =


> =


> I'm kind of unsure how i feel about it downloading things on cmake time.

I asked a few packagers I know and for them, since the packagers can downl=
oad
the files and put them into the tarball, it should be fine. But they also
said that it would be way better to have it fetched as run time, but it al=
so
means that I will need to do some larger change since currently, the code
expects the files to be there and assert otherwise. I will let you all kno=
w
them I figure out an elegant solution.

> =


> Also the left bar seems to need some layouting fixes, there's an "l" mis=
sing from the button at the bottom and the slider also can go "past" the b=
ar as illustrated by the screenshot https://i.imgur.com/KTo8WmG.png

Fixed using the same hack as in Discover, the current problem is that Kiri=
gami
default sidebar width is too large, but hardcoding a width can break using=
 a
different locale. I believe this will require a discussion in the kirigami=
 and
VDG channel to decide on sane default width, so that applications don't ne=
ed to
hardcode size that can break on different locales.

> Cheers,
> Albert
> =


> > Carl Schwan
> > https://carlschwan.eu


["publickey - carl@carlschwan.eu - 0x7F564CB5.asc" (application/pgp-keys)]
["signature.asc" (application/pgp-signature)]

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