[prev in list] [next in list] [prev in thread] [next in thread]
List: kde-core-devel
Subject: Re: Kup in KDE Review
From: Nicolas Fella <nicolas.fella () gmx ! de>
Date: 2020-04-06 22:01:26
Message-ID: 3269815.e9J7NaK4W3 () madeye
[Download RAW message or body]
On Montag, 6. April 2020 12:32:54 CEST Simon Persson wrote:
> Hello!
>
>
> Please help to review kup.
>
> It is a backup scheduler tightly integrated with plasma (has system
> setting kcm, systray plasmoid, kioslave). It supports saving backups
> either with bup or with rsync.
>
> It has been developed outside of KDE for many years and only now is
> being incubated.
>
> I am unsure if it should end up in extragear or some release service
> bundle. Perhaps people have an opinion on this?
>
> https://invent.kde.org/kde/kup
>
>
> Thanks,
>
> Simon
Hi,
I briefly skimmed trough the codebase. Looks all sane to me. A few minor o=
bservations:
- You may want to look into KConfigXT. It should be able to generate the c=
lasses from
settings/ from an XML description.
- You are using KInit for the daemon executable. We are looking into killi=
ng that in the KF6
transition (https://phabricator.kde.org/T12140[1] ) so consider it depreca=
ted (although it is
not offically yet). If you do not care about using kup outside of Plasma y=
ou might also
consider implementing the daemon part as a kded module.
- In https://invent.kde.org/kde/kup/-/blob/master/filedigger/mergedvfsmode=
l.cpp#L60[2]
please check if instead of calling loadMimeTypeIcon simply returning the i=
conname is
enough, or return QIcon::fromTheme(name). loadMimeTypeIcon looks like a li=
kely
candidate for deprecation/removal to me. That would also get rid of the KI=
conThemes
dependency.
Cheers
Nico
=2D-------
[1] https://phabricator.kde.org/T12140
[2] https://invent.kde.org/kde/kup/-/blob/master/filedigger/mergedvfsmodel=
.cpp#L60
[Attachment #3 (unknown)]
<html>
<head>
<meta http-equiv="content-type" content="text/html; charset=UTF-8">
</head>
<body><p>On Montag, 6. April 2020 12:32:54 CEST Simon Persson wrote:</p>
<p>> Hello!</p>
<p>> </p>
<p>> </p>
<p>> Please help to review kup.</p>
<p>> </p>
<p>> It is a backup scheduler tightly integrated with plasma (has \
system</p> <p>> setting kcm, systray plasmoid, kioslave). It supports \
saving backups</p> <p>> either with bup or with rsync.</p>
<p>> </p>
<p>> It has been developed outside of KDE for many years and only now \
is</p> <p>> being incubated.</p>
<p>> </p>
<p>> I am unsure if it should end up in extragear or some release \
service</p> <p>> bundle. Perhaps people have an opinion on this?</p>
<p>> </p>
<p>> https://invent.kde.org/kde/kup</p>
<p>> </p>
<p>> </p>
<p>> Thanks,</p>
<p>> </p>
<p>> Simon</p>
<p> <p>Hi,</p>
<p> <p>I briefly skimmed trough the codebase. Looks all sane to me. A \
few minor observations:</p> <p>- You may want to look into KConfigXT. It \
should be able to generate the classes from settings/ from an XML \
description.</p> <p>- You are using KInit for the daemon executable. We are \
looking into killing that in the KF6 transition (<a \
href="https://phabricator.kde.org/T12140">https://phabricator.kde.org/T12140</a> ) \
so consider it deprecated (although it is not offically yet). If you do not \
care about using kup outside of Plasma you might also consider implementing \
the daemon part as a kded module.</p> <p>- In <a \
href="https://invent.kde.org/kde/kup/-/blob/master/filedigger/mergedvfsmodel \
.cpp#L60">https://invent.kde.org/kde/kup/-/blob/master/filedigger/mergedvfsmodel.cpp#L60</a> please \
check if instead of calling loadMimeTypeIcon simply returning the iconname \
is enough, or return QIcon::fromTheme(name). loadMimeTypeIcon looks like a \
likely candidate for deprecation/removal to me. That would also get rid of \
the KIconThemes dependency.</p> <p> <p>Cheers</p>
<p> <p>Nico</p>
<p> </p>
<p> </body>
</html>
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic