Move Koko to KDEReview

[Carl Schwan]

Le jeudi, juin 11, 2020 9:43 PM, Albert Astals Cid <aacid at kde.org> a écrit :

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

> > 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 images.
> Ah no, it only lists images from the "Pictures" folder, i see, kind of weird for a desktop app.

This is the default behavior in Digikam and KPhotoAlbum, the only difference 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 external drive
for example) but this is not my priority yet.

> I think you have a memory leak in FileSystemTracker::reindexSubFolder, there'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::setApplicationDomain("koko"); in your main.cpp

This was added in https://invent.kde.org/plasma-mobile/koko/-/merge_requests/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 download
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 also
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 know
them I figure out an elegant solution.

> 

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

Fixed using the same hack as in Discover, the current problem is that Kirigami
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 need to
hardcode size that can break on different locales.

> Cheers,
> Albert
> 

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

-------------- next part --------------
A non-text attachment was scrubbed...
Name: publickey - carl at carlschwan.eu - 0x7F564CB5.asc
Type: application/pgp-keys
Size: 3196 bytes
Desc: not available
URL: <http://mail.kde.org/pipermail/kde-core-devel/attachments/20200612/c25a0b9c/attachment.key>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 823 bytes
Desc: OpenPGP digital signature
URL: <http://mail.kde.org/pipermail/kde-core-devel/attachments/20200612/c25a0b9c/attachment.sig>