Kup in KDE Review

[Nicolas Fella]

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 observations:
- You may want to look into KConfigXT. It should be able to generate the classes from
settings/ from an XML description.
- You are using KInit for the daemon executable. We are looking into killing that in the KF6
transition (https://phabricator.kde.org/T12140[1] ) 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.
- In https://invent.kde.org/kde/kup/-/blob/master/filedigger/mergedvfsmodel.cpp#L60[2]
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.

Cheers

Nico



--------
[1] https://phabricator.kde.org/T12140
[2] https://invent.kde.org/kde/kup/-/blob/master/filedigger/mergedvfsmodel.cpp#L60
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-core-devel/attachments/20200407/b3e39797/attachment.htm>