Le jeudi, juin 11, 2020 9:43 PM, Albert Astals Cid <aa...@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
publickey - carl@carlschwan.eu - 0x7F564CB5.asc
Description: application/pgp-keys
signature.asc
Description: OpenPGP digital signature
