D27951: Allow users to change dropAction to MoveAction through kdeglobals

[David Edmundson]

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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-frameworks-devel/attachments/20200321/81f07e56/attachment-0001.html>