davidedmundson added a comment.
I've done a code review, they need following up regardless.
---
As you saw in the bug report I don't like the feature, and I'm against it.
Though I can't veto changes.
Repeating myself on bugzilla.
The two things I really hate as justifications for doing anything are
"windows does it" or "many users want". It's our job to do thinking and provide
something that's actually better.
This little menu was one of the first things I saw when I first used
Konqueror and I remember it being one of the things that made me love KDE.
It's excellent thought through usability, drag and drop is inherently
ambiguous. For something so important, always prompting is always safe. It's
consistent and not pseudo-random and the little UI follows fitt's law and is
scarcely slower. I'm aware this is an option, but IMHO an option for something
bad.
INLINE COMMENTS
> dropjob.cpp:366
> + // if the default behavior has been changed to MoveAction
> + const KConfigGroup g =
> KConfigGroup(KSharedConfig::openConfig(QStringLiteral("kdeglobals")),
> QStringLiteral("KDE"));
> + if (g.readEntry("dndToMove", false)) {
there's no need to reopen kdelgobals
Use KSharedConfig::openConfig()
and due to the magic of cascading it will include kdeglobals
> dropjob.cpp:286
> +
> + QStorageInfo sourceStorage;
> + QStorageInfo destStorage(m_destUrl.path());
Why QStorageInfo? We're in kio. There's a KMountPoint which is similar
I suspect this is a recursive list up the tree resolving symlinks. This will
mean blocking stat calls, so this somewhat undermines a lot of the recent work
in that field.
> dropjob.cpp:287
> + QStorageInfo sourceStorage;
> + QStorageInfo destStorage(m_destUrl.path());
> +
If destination is a symlink on the same partition but that symlink points to
another partition what happens?
I haven't checked myself, but if we are doing this I need us to be super super
sure.
> dropjob.cpp:309
> + sourceStorage.setPath(url.path());
> + if (sourceStorage.device() != destStorage.device()) {
> + allItemsAreSameDevice = false;
This will trigger when both storage devices are invalid, which isn't what we
want.
REPOSITORY
R241 KIO
REVISION DETAIL
https://phabricator.kde.org/D27951
To: trmdi, ngraham, dfaure, meven, #vdg, davidedmundson
Cc: elvisangelaccio, davidedmundson, meven, kde-frameworks-devel, LeGast00n,
cblack, GB_2, michaelh, ngraham, bruns
