| elvisangelaccio | |
| rkflx | |
| dfaure | |
| ngraham |
| Dolphin |
This patch aims to improve user experience by not allowing the user to drag and drop a folder into itself.
The current behavior shows a red message at the top which can then be closed by the user, instead of relying on that, this patch disables the option of dropping onto self and uses the "Invalid drop target cursor" to highlight the behavior.
BUG: 307747
Since spectacle is unable to screenshot the cursor overlay, find attached a photo of the screen.
| Automatic diff as part of commit; lint not applicable. |
| Automatic diff as part of commit; unit tests not applicable. |
Should we disable this completely when Open folders during drag operations is enabled? Thoughts?
I think so. GeneralSettings::autoExpandFolders() is false by default, so not a big deal.
Hmm, I don't think we want this entire thing turned off when GeneralSettings::autoExpandFolders() is true. Seems kind of pointless to turn off this very nice behavioral improvement if you have auto-expanding folders on. In that case you do want folders to auto-expand... just not when one is dragged on top of itself! :)
What if I want to copy the folder into some child folder? The auto-expand feature currently allows to do that, this patch would break it.
The issue is that by disallowing that you lose a bit of funcionality(link and copy here actions).
I'll see what we can do about it though.
Allow folder to be expanded when using AutoExpand setting but disallow dropping once inside.
@ngraham @elvisangelaccio I think this might work a bit better with regards to the auto expand folder setting. It will allow you to open a folder with auto expand but disallow drop once inside of it.
I'm not sure I understand. Can you show an example of a use case where it would be a valid interaction to move or copy a folder into itself, or a child folder of itself? That seems like it would be a recursive impossibility...
@emateli I'm afraid your latest changes do not build for me, due making indexIsDir pure virtual and guarding it with HAVE_BALOO. I needed the following patch, could you have a look?
diff --git a/src/kitemviews/kfileitemmodel.cpp b/src/kitemviews/kfileitemmodel.cpp index 195e86eba..fe97343c9 100644 --- a/src/kitemviews/kfileitemmodel.cpp +++ b/src/kitemviews/kfileitemmodel.cpp @@ -2419,7 +2419,7 @@ QUrl KFileItemModel::indexGetUrl(int index) const return fileItem(index).url(); } -bool KFileItemModel::indexIsDir(int index) +bool KFileItemModel::indexIsDir(int index) const { return fileItem(index).isDir(); } diff --git a/src/kitemviews/kfileitemmodel.h b/src/kitemviews/kfileitemmodel.h index 027ed37c2..6cd9ec500 100644 --- a/src/kitemviews/kfileitemmodel.h +++ b/src/kitemviews/kfileitemmodel.h @@ -146,7 +146,7 @@ public: KFileItem rootItem() const; QUrl indexGetUrl(int index) const Q_DECL_OVERRIDE; - bool indexIsDir(int index) Q_DECL_OVERRIDE; + bool indexIsDir(int index) const Q_DECL_OVERRIDE; /** * Clears all items of the model. diff --git a/src/kitemviews/kitemmodelbase.cpp b/src/kitemviews/kitemmodelbase.cpp index 251b57dd6..8086c595e 100644 --- a/src/kitemviews/kitemmodelbase.cpp +++ b/src/kitemviews/kitemmodelbase.cpp @@ -171,6 +171,12 @@ QUrl KItemModelBase::indexGetUrl(int index) const return QUrl(); } +bool KItemModelBase::indexIsDir(int index) const +{ + Q_UNUSED(index); + return false; +} + QUrl KItemModelBase::directory() const { return QUrl(); diff --git a/src/kitemviews/kitemmodelbase.h b/src/kitemviews/kitemmodelbase.h index 844171343..7dc33a995 100644 --- a/src/kitemviews/kitemmodelbase.h +++ b/src/kitemviews/kitemmodelbase.h @@ -190,7 +190,7 @@ public: /** * @return True, if item at specified index is a directory */ - virtual bool indexIsDir(int index) = 0; + virtual bool indexIsDir(int index) const; /** * @return Parent directory of the items that are shown diff --git a/src/panels/places/placesitemmodel.cpp b/src/panels/places/placesitemmodel.cpp index 5e64e1551..193a8615b 100644 --- a/src/panels/places/placesitemmodel.cpp +++ b/src/panels/places/placesitemmodel.cpp @@ -1202,14 +1202,13 @@ QUrl PlacesItemModel::searchUrlForType(const QString& type) return query.toSearchUrl(); } +#endif -bool PlacesItemModel::indexIsDir(int index) +bool PlacesItemModel::indexIsDir(int index) const { return true; } -#endif - #ifdef PLACESITEMMODEL_DEBUG void PlacesItemModel::showModelState() { diff --git a/src/panels/places/placesitemmodel.h b/src/panels/places/placesitemmodel.h index 805de3af9..1a2c0d315 100644 --- a/src/panels/places/placesitemmodel.h +++ b/src/panels/places/placesitemmodel.h @@ -133,7 +133,7 @@ public: void saveBookmarks(); QUrl indexGetUrl(int index) const Q_DECL_OVERRIDE; - bool indexIsDir(int index) Q_DECL_OVERRIDE; + bool indexIsDir(int index) const Q_DECL_OVERRIDE; signals: void errorMessage(const QString& message);
Unfortunately, this problem is still not fixed with the latest iteration (and probably unrelated to the build fix).
As this patch is about replacing the KMessageWidget with a do-not-enter cursor, we probably should not change the behaviour too, so I guess descending is still wanted and additional changes should go in an extra patch.
(That said, it is still arguably how much of a power user feature copying directories into themselves is and whether Dolphin should allow it. While people on Stackoverflow are asking for this, it would only make sense if a list of all files to copy is made beforehand, so this does not result in a recursive loop – which KIO does already? Also note how Dolphin currently forbids copying A to A/A and only allows copying A to A/A/A, which seems strange.)
Since spectacle is unable to screenshot the cursor overlay, find attached a photo of the screen.
@emateli You can use Spectacle's Delay option for that, BTW.
Fix issue when prematurely releasing drop on a folder that was about to auto-open.
Unfortunately, this problem is still not fixed with the latest iteration (and probably unrelated to the build fix).
Should be Okay now, had completely missed this point.
Also note how Dolphin currently forbids copying A to A/A and only allows copying A to A/A/A, which seems strange.
Don't quote me on this, but I believe that is handled by KIO.
Edit: @rkflx: I'm not sure about HAVE_BALOO guarding since I can't seem to spot it near the regions affected by this patch. The decision to have a pure virtual function is that in the KItemModelBase there is no way to evaluate the indexIsDir function correctly, thus returning either true or false is technically incorrect.
Thanks for fixing, can confirm this now works very well for me.
I'm not sure about HAVE_BALOO guarding since I can't seem to spot it near the regions affected by this patch.
I can only recommend setting up Arcanist and using arc diff to upload the patch instead of Phabricator's web upload, as this would allow to see (and comment on) the full context of a file.
Anyway, have a look at how my patch moves the #endif around and see how this relates to the lines of code guarded by HAVE_BALOO. Do you see what I mean? I cannot imagine how your patch is related to baloo, this should compile for me even if I do not have baloo.
The decision to have a pure virtual function is that in the KItemModelBase there is no way to evaluate the indexIsDir function correctly, thus returning either true or false is technically incorrect.
Well, I'm all for pure virtuals (and more const, BTW), but this breaks compiling the tests for me:
src/tests/kitemlistselectionmanagertest.cpp:94:30: error: invalid new-expression of abstract class type ‘DummyModel’
m_model = new DummyModel();
^
src/tests/kitemlistselectionmanagertest.cpp:27:7: note: because the following virtual functions are pure within ‘DummyModel’:
class DummyModel : public KItemModelBase
^~~~~~~~~~
In file included from src/tests/kitemlistselectionmanagertest.cpp:21:0:
src/kitemviews/kitemmodelbase.h:193:18: note: virtual bool KItemModelBase::indexIsDir(int)
virtual bool indexIsDir(int index) = 0;
^~~~~~~~~~You could try keeping the pure virtual, but adapting the test, of course.
Fixed code being mistakenly surrounded by HAVE_BALOO directive and adjusted tests for the pure virtual method.
Well, I'm all for pure virtuals (and more const, BTW)
Correct me If I'm wrong, but isn't returning const for objects that assigned by value kind of pointless since it's copied anyways?
adjusted tests for the pure virtual method.
Compiling with GCC 5.4 (KDE Neon) and GCC 7.2 (openSUSE Tumbleweed) still does not succeed:
src/tests/kstandarditemmodeltest.cpp:48:38: error: invalid new-expression of abstract class type ‘KStandardItemModel’
m_model = new KStandardItemModel();
^
In file included from src/tests/kstandarditemmodeltest.cpp:24:0:
src/kitemviews/kstandarditemmodel.h:38:22: note: because the following virtual functions are pure within ‘KStandardItemModel’:
class DOLPHIN_EXPORT KStandardItemModel : public KItemModelBase
^~~~~~~~~~~~~~~~~~
In file included from src/kitemviews/kstandarditemmodel.h:24:0,
from src/tests/kstandarditemmodeltest.cpp:24:
src/kitemviews/kitemmodelbase.h:193:18: note: virtual bool KItemModelBase::indexIsDir(int)
virtual bool indexIsDir(int index) = 0;
^~~~~~~~~~I'm wondering why this is working for you. Are you actually building the tests? Special compiler options, different distro? Note that in the end it also has to build on the various CI configs. Now I'm curious whether you will come up with something different than my patch :)
isn't returning const for objects that assigned by value kind of pointless since it's copied anyways?
Are we talking about the same thing? In general it is good practice to add const to a method's signature if it does not modify its object's member variables amongst other things. At the point of writing the method, it is not really relevant which assignment semantics somebody else is later going to choose.
You're both right: const methods (i.e. the const at the end) = GOOD. Returning const values (i.e. const at the beginning) = BAD (some books recommended it for C++98, but with move semantics in C++11 this is a bad idea, prevents moving from that).
You're both right: const methods (i.e. the const at the end) = GOOD.
That's what is meant in D6281#160094 :) (but still good to know both).
in the KItemModelBase there is no way to evaluate the indexIsDir function correctly, thus returning either true or false is technically incorrect.
@dfaure Any tips for @emateli regarding this? Currently it is a pure virtual, but this does not play well with the tests.
I'm wondering why this is working for you. Are you actually building the tests? Special compiler options, different distro? Note that in the end it also has to build on the various CI configs. Now I'm curious whether you will come up with something different than my patch :)
Turns out I was not building everything :| . Tested it again with all targets and tests run fine.
And about the const thing. I had in mind methonds which just return bool PlacesItemModel::indexIsDir(int index) const this I know to be unnecessary, however you can see it's being used on the other method which returns a QUrl.
Ok, you duplicated it everywhere where necessary. Not elegant, but perhaps the best we can do without introducing more classes. Compiles now for me, which is great.
I had in mind methonds which just return bool PlacesItemModel::indexIsDir(int index) const this I know to be unnecessary
Isn't what you mean const bool f() and not bool g() const, though? The first is about the return type, the second is about what the function is allowed to do in its implementation (i.e. cannot modify members and cannot call non-const member functions).
Marking this as accepted from my side, as you fixed both the unintentional auto-open and the compile failure. I still think adding the "good" const would be better.
Note I did not review the complete Diff, so you should still get approval from the other reviewers.
@elvisangelaccio @ngraham You still have open change requests. Are those solved now?
I'm afraid not. The whole idea of moving a folder inside *itself* makes no sense to me and I can't see a use case where the sort of person who uses a GUI file manager actually intends to do this. I have to imagine that a sort of person who wants to do this is probably wouldn't be caught dead using Dolphin instead of Midnight Commander or something :p
As such, I don't think the feature should be disabled entirely when "open folders during drag operations" is turned on.
Do you mean "I do think" here? If so, you should provide a separate patch – or convince somebody else to write a patch ;). This one is about changing from KMessageWidget to an cursor overlay, which is relatively unrelated to e.g. doing this via keyboard shortcuts.
That said, this is something for Dolphin's maintainers to decide. As mentioned above, I'm not sure whether this is a feature or a bug. Reading Dolphin's vision, we might tend to the latter. Even if we disabled the copy case, users could still achieve their (weird) goal by creating the folder by themselves and then only copying the contents.
Edit:
This is further complicated by the fact that we don't know until after descending the folder hierarchy whether we will move (always error), copy (sometimes error) or link (might actually make sense, e.g. if you want to add a link to the base folder so you can come back to it from some deep subfolder).
Ok, you duplicated it everywhere where necessary. Not elegant, but perhaps the best we can do without introducing more classes.
True, but the virtual method feels technically more correct to me. Maybe the other guys can chime in on this. I'd be happy to change it to whatever feels more appropriate for the project, I just personally prefer the current iteration.
Isn't what you mean const bool f()
Yes, that's what I meant. The bool gets copied anyways so the const wouldn't do much I suppose. @dfaure @elvisangelaccio thoughts on this?
@ngraham as far as disabling the feature, like previously stated I think is outside of the scope of this patch. However I would say for it to remain as a feature, just for Dolphin to handle and not show a Move option when dropping since that one will never work anyways.
That's what I figured. But to repeat myself: That's not what I propose you add. I'd like to see the other ("good") form, i.e. the const at the end.
Can someone try this patch on Wayland? It doesn't seem to work there :(
I also have other comments on the code, I'll do a proper review next week.
Half of it works on Wayland for me: The cursor changes on hovering, but dropping still shows the KMessageWidget instead of doing nothing.
I will try to set up a VM with Wayland in the following days to check this, but generally speaking, shouldn't setting the accepted flag of the drop to false prevent the it from happening? And I checked to see if it was being marked as an accepted drop elsewhere but I couldn't find traces.
No need to install a full VM, from your X11 session just run a nested kwin with:
kwin_wayland --xwayland
and then start dolphin with
dolphin --platform wayland
Fix drop issue in Wayland.
RFC: Should the urlListMatchesUrl method stay at the helper or should I refactor it into a new file?
| src/kitemviews/kitemmodelbase.h | ||
|---|---|---|
| 188 | How about calling it just getUrl(int index)? | |
| 193 | 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(). | |
| src/views/draganddrophelper.cpp | ||
| 35 | We can use the C++11 for loop here (foreach will be eventually deprecated). | |
| 57 | This comment should be either adjusted (e.g. add "unless we are dropping a folder onto itself"), or moved below the if(). | |
| src/views/draganddrophelper.h | ||
| 29 | This forward declaration can be removed if we are including the full QUrl header. | |
| src/kitemviews/kitemmodelbase.h | ||
|---|---|---|
| 188 | Can do. It's just the way that I name things and sometimes you forget to check for naming conventions on the specific project. | |
| 193 | I was unaware of the data() function. Right now it looks like we can avoid all those method overrides by simply implementing them in the base class by using the data function. The getUrl method never gets overriden since every descendant uses the url property, whereas isDir gets overridden only at the places model since it doesn't have an isDir index on the hash. So far it works quite okay, however I'll see to push the patch on the weekend just to make sure that everything is fine. | |
| src/kitemviews/kitemmodelbase.cpp | ||
|---|---|---|
| 16 | one space between return and QUrl was enough, no need to have two. | |
| src/kitemviews/kitemmodelbase.h | ||
| 188 | In Qt/KDE code, for non-static methods, getFoo() is actually not the convention, foo() is. | |
| src/views/draganddrophelper.cpp | ||
| 35 | I would have used std::find_if, actually, but OK ;-) (at least, const auto & would be the more usual way to write this) | |
constBegin/End instead of .begin/end.
Also, so we need some test cases for urlListMatchesUrl?
| src/tests/draganddrophelpertest.cpp | ||
|---|---|---|
| 30 | Usually the _data() function goes above the test function. | |
| 39 | QCOMPARE here | |
| 46 | Maybe "expectedMatch" ? | |
| 48 | A number is not very descriptive as name for a test case. Ideallt it should be a sentence describing what are we testing. Also, please use QUrl::fromLocalFile() instead of hardcoding the file:// scheme. Personal taste: how about declaring the url lists with QList<QUrl> { QUrl(...) } ? | |
| 51–53 | QUrl::fromLocalFile() also here | |
| 82 | Files should always end with a newline | |
| src/tests/draganddrophelpertest.cpp | ||
|---|---|---|
| 30 | I should really configure my IDE for the KDE formatting guidelines | |
@emateli With 5 patches contributed successfully over the last months (and more in the pipeline, perhaps? ;), have you considered applying for a developer account?