D6281: Prevent folders from drag and dropping onto themselves in dolphin main view

[Elvis Angelaccio]

elvisangelaccio requested changes to this revision.
elvisangelaccio added inline comments.
This revision now requires changes to proceed.

INLINE COMMENTS

> kitemmodelbase.h:188
> +     */
> +    virtual QUrl indexGetUrl(int index) const;
> +

How about calling it just `getUrl(int index)`?

> kitemmodelbase.h:193
> +     */
> +    virtual bool indexIsDir(int index) const = 0;
> +

Why are we adding this method in the first place? Can't we use `data()` with the `"isDir"` role?

If there is a reason that I'm missing, then this method should be called `isDir()`, for consistency with `isExpanded()` and `isExpandable()`.

Also, you said you made it pure virtual because in the base class we don't know whether to return true or false. But that's not a problem, we can just say in the documentation that the default value is false (because we don't know any better). That's what we are already doing in e.g. `isExpanded()`/`isExpandable()`.

> draganddrophelper.cpp:35
> +{
> +    foreach(const QUrl& url, urls) {
> +        if (url.matches(destUrl, QUrl::StripTrailingSlash)) {

We can use the C++11 `for` loop here (`foreach` will be eventually deprecated).

> draganddrophelper.cpp:57
>      } else {
>          // Drop into a directory or a desktop-file
> +        if (urlListMatchesUrl(event->mimeData()->urls(), destUrl)) {

This comment should be either adjusted (e.g. add "unless we are dropping a folder onto itself"), or moved below the `if()`.

> draganddrophelper.h:29
>  
>  class QUrl;

This forward declaration can be removed if we are including the full QUrl header.

REVISION DETAIL
  https://phabricator.kde.org/D6281

To: emateli, #dolphin, elvisangelaccio, ngraham, rkflx
Cc: rkflx, ngraham, elvisangelaccio, dfaure, anthonyfieroni, #konqueror, spoorun, navarromorales, firef, andrebarros, emmanuelp
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.kde.org/mailman/private/kfm-devel/attachments/20171103/fb5d4d62/attachment.htm>