Kup in KDE Review

[Simon Persson]

Hello!

On 2020-04-07 06:01, Nicolas Fella wrote:
>
> 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.
>
I think that I looked at it when I started Kup, about 10 years ago... 
don't remember exactly but I think the issue was about dynamic 
configuration entries. Kup allows user to create as many backup plans as 
they want and each will have some settings.
>
> - You are using KInit for the daemon executable. We are looking into 
> killing that in the KF6 transition 
> (https://phabricator.kde.org/T12140 ) 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.
>
Ok, thanks for the info. And yes, moving it to kded might be possible. 
Again, I think I looked at doing that in the beginning but don't 
remember the reason that I went for a standalone executable instead.
>
> - In 
> https://invent.kde.org/kde/kup/-/blob/master/filedigger/mergedvfsmodel.cpp#L60 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.
>
Excellent finding! I have pushed a commit now to remove this dependency.

Simon