Re: Move Koko to KDEReview
Carl Schwan wrote: > 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. Then why don't you put them into the tarball? Or should they get packaged as a separate package that you can then depend on? > But they also said that it would be way better to have it fetched as run > time, While that would at least not fail the build in Fedora, packages downloading additional files at runtime that they need to be able to do anything is somewhat frowned upon around here. (Whether you will get away with it depends on what the files do and what license they fall under.) I personally do not think that it is acceptable for an image viewer to require an Internet connection at runtime. Your proposed solution only moves the problem instead of solving it. Kevin Kofler
Re: Move Koko to KDEReview
Adriaan de Groot wrote: > On Thursday, 11 June 2020 23:43:52 CEST Albert Astals Cid wrote: >> I'm kind of unsure how i feel about it downloading things on cmake time. > > A fair number of distro's / packagers will go "um nope" (if the package > building machines even *have* an internet connection during configure / > build stages). Yep, this is an absolute no go in Fedora. The build system (Koji) has all Internet connection deliberately blocked. Packages MUST NOT attempt to access the Internet during builds. No exceptions. Kevin Kofler
Re: Move Koko to KDEReview
On Friday, 12 June 2020 18:05:36 CEST Carl Schwan wrote:
> > 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
There's a couple of angles here:
- if your CMakeLists.txt tries to download something, it will fail.
- if you expect packagers to download additional files then put that in GIANT
LETTERS in several places; also make any attempted download fail gracefully
with a useful message ("additional source files are needed ..").
It's not that unusual for a single package to require multiple source tarballs
or additional source files, but you need to support it *and* be clear about
what's needed.
Koko does the right thing in accepting an already-downloaded file; it'd be
nice to fail more gracefully, and you should make it more prominent rather
than hidden on line 106 of CMakeLists.txt in a subdirectory.
[ade]
signature.asc
Description: This is a digitally signed message part.
Re: Move Koko to KDEReview
Il giorno Fri, 12 Jun 2020 16:05:36 + Carl Schwan ha scritto: > 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. The README should state what purpose those files have. I didn't find any comment in the CMakeLists that tell what they are needed for. Also, under what license do these files fall under?
Re: Move Koko to KDEReview
Le jeudi, juin 11, 2020 9:43 PM, Albert Astals Cid 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
Re: Move Koko to KDEReview
Le mercredi, juin 10, 2020 9:01 AM, Yuri Chornoivan a écrit : > > > 10 червня 2020, 11:35:37, від "Carl Schwan"c...@carlschwan.eu: > > > 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. > > Koko is following the KDE HIG and is build using QtQuick and the > > Kirigami framework. > > The repo is located here: https://invent.kde.org/plasma-mobile/koko/. > > There is only one know big issue, but it is in the progress of > > being resolved1. > > In the future, I plan to add a simple image editor that is currently > > being developed as a separate and reusable library. More info about > > it is available in my latest blog post2. > > I would be happy if it could be reviewed and my goal is to move it to > > the release service, I hope in time for 20.08. > > 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. > > > > Added. Scripty will extract the catalog for translation tomorrow. > > But I bet the translations will not be loaded because of the lack of > KAboutData and friends. See KLook: > > https://invent.kde.org/graphics/klook/-/blob/master/src/main.cpp > > Hope this helps and thanks for your work. > > Best regards, > Yuri Thanks for the Messages.sh commit, I now created a merge request adding the KAboutData bits: https://invent.kde.org/plasma-mobile/koko/-/merge_requests/25 > > > - 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. > > > > > > Carl Schwan > > https://carlschwan.eu publickey - carl@carlschwan.eu - 0x7F564CB5.asc Description: application/pgp-keys signature.asc Description: OpenPGP digital signature
Re: Move Koko to KDEReview
10 червня 2020, 11:35:37, від "Carl Schwan" : > 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. > > Koko is following the KDE HIG and is build using QtQuick and the > Kirigami framework. > > The repo is located here: https://invent.kde.org/plasma-mobile/koko/. > There is only one know big issue, but it is in the progress of > being resolved[1]. > > In the future, I plan to add a simple image editor that is currently > being developed as a separate and reusable library. More info about > it is available in my latest blog post[2]. > > I would be happy if it could be reviewed and my goal is to move it to > the release service, I hope in time for 20.08. > > 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. Added. Scripty will extract the catalog for translation tomorrow. But I bet the translations will not be loaded because of the lack of KAboutData and friends. See KLook: https://invent.kde.org/graphics/klook/-/blob/master/src/main.cpp Hope this helps and thanks for your work. Best regards, Yuri > * 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. > > Carl Schwan > https://carlschwan.eu > > [1]: https://invent.kde.org/plasma-mobile/koko/-/merge_requests/20 > [2]: https://carlschwan.eu/2020/06/06/koko-desktop.html >
Re: Move Koko to KDEReview
On Thursday, 11 June 2020 23:43:52 CEST Albert Astals Cid wrote: > I'm kind of unsure how i feel about it downloading things on cmake time. A fair number of distro's / packagers will go "um nope" (if the package building machines even *have* an internet connection during configure / build stages). [ade] signature.asc Description: This is a digitally signed message part.
Re: Move Koko to KDEReview
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.
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
> * 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.
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
Cheers,
Albert
>
> Carl Schwan
> https://carlschwan.eu
>
> [1]: https://invent.kde.org/plasma-mobile/koko/-/merge_requests/20
> [2]: https://carlschwan.eu/2020/06/06/koko-desktop.html
Re: Move Koko to KDEReview
+1, I've been using Koko and find it quite pleasant. Nate On 6/9/20 5:30 AM, Carl Schwan wrote: 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. Koko is following the KDE HIG and is build using QtQuick and the Kirigami framework. The repo is located here: https://invent.kde.org/plasma-mobile/koko/. There is only one know big issue, but it is in the progress of being resolved[1]. In the future, I plan to add a simple image editor that is currently being developed as a separate and reusable library. More info about it is available in my latest blog post[2]. I would be happy if it could be reviewed and my goal is to move it to the release service, I hope in time for 20.08. 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. * 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. Carl Schwan https://carlschwan.eu [1]: https://invent.kde.org/plasma-mobile/koko/-/merge_requests/20 [2]: https://carlschwan.eu/2020/06/06/koko-desktop.html
Move Koko to KDEReview
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. Koko is following the KDE HIG and is build using QtQuick and the Kirigami framework. The repo is located here: https://invent.kde.org/plasma-mobile/koko/. There is only one know big issue, but it is in the progress of being resolved[1]. In the future, I plan to add a simple image editor that is currently being developed as a separate and reusable library. More info about it is available in my latest blog post[2]. I would be happy if it could be reviewed and my goal is to move it to the release service, I hope in time for 20.08. >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. * 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. Carl Schwan https://carlschwan.eu [1]: https://invent.kde.org/plasma-mobile/koko/-/merge_requests/20 [2]: https://carlschwan.eu/2020/06/06/koko-desktop.html publickey - carl@carlschwan.eu - 0x7F564CB5.asc Description: application/pgp-keys signature.asc Description: OpenPGP digital signature
