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
