Great! Is there anything else left to do @elvisangelaccio and @mlaurent before I land this?

• Rebase
• Reserve memory for lists/vectors in advance.
• Prepend "static" to all class-level functions
• Fix typos

Should I merge Applications/18.08 into master too?

• Use QString() directly instead of QStringLiteral("")
• Use explicit specifier
• Call either finished or error in slave methods

• Rebase
• Remove unrelated changes
• Remove Q_FUNC_INFO
• Remove private static methods from header file
• Use currentDateTimeUtc instead of currentDateTime
• Generate D-Bus interface with qdbusxml2cpp

Thanks for the review. I will address your comments and hopefully have a patch ready in the next days. :)

I am able to reproduce them both. @wbauer do you want to fix them? If not, I can spend some time on it. :)

Excuse my late reply. I have a few more devices for testing and it works pretty well. After a little refactoring of the code, I will create a Diff soon to, as you mentioned, reach more people to review it.

Over the past few weeks, I've been working on a better / more reliable way to access MTP devices with the KIO framework. I'm just about to show you what I have done so far. If you have any ideas that improves my work, don't hesitate and let me know :)

Great, see you later!

Apparently, I'm working on exactly this approach for the last week. :) I moved all the MTP handling to a KDED module. The KIO slaves then connects to this KDED module via dbus. It will still take some time to be finished but I get pretty promising results so far.
I will be at the Akademy today (16.08.18) at around 5pm, so if you are around we can discuss more on this topic. Sadly am not able to take my laptop with me to show you how it works so far...

Good. :) I don't have commit rights yet so I think I can't do it there.

Implement setInfo function for each, the Port and Profile class.

My patched version of the plugin for the KCM wasn't loaded correctly, that's why I saw the same results. I'm still struggling developing KDE libs/plugins but it's getting better :)

Accept profiles which are not unavailable.

In D13694#282499, @nicolasfella wrote:

This breaks the profile selection Combobox in the KCM.

I'm sorry to hear that, on my system I get the same results as in the released kcm.

Rebase to origin/Plasma/5.12.

Not yet, @ngraham did all the commits for me so far. Just in case, my email is andreas.krutzler@gmx.net :)

The gifs were the hardest part :)

In D9711#193793, @elvisangelaccio wrote:

I had a look and this bug is caused by the forceUrlsSelection() call in slotRoleEditingFinished() (added by 478f404b8a).
The problem is that we are marking the item being renamed as the m_currentItemUrl, but that is not necessarily correct as this bug shows.

@emateli is right that forceUrlsSelection() is a better place to put this check in. Adding

if (m_container->controller()->selectionManager()->hasSelection()) {
    return;
}

at the beginning of the method fixes the bug for me.

No problem, thanks for your review! :)

In D9711#193368, @michaelh wrote:

Maybe our dolphins differ. With fresh compile from origin/Applications/17.12 this is unexpected

When I do this, a.tmp is renamed to aaa.tmp and b.tmp remains selected. For me, that's the result one would expect.

In D9711#187679, @emateli wrote:

Unfortunately I do not have as much time as I would like to investigate this, however I suspect it may have to do with slotRenamingFinished and forceUrlsSelection. As in the slot is emitted after the files(s) have finished renaming but at that point the user has already changed the selection. If that's the case it may make a bit more sense to handle this inside the forceUrlSelection method instead.

Fix comments
Rebase

In D8822#169485, @rkflx wrote:

(Interestingly, I just noticed what Andreas fixed here was still working in a KDE4 based Dolphin…)

I think this bug was introduced in Dolphin 2.0(4.8), where he got his new view engine six years ago. Somewhere around f23e9496f303995557b744c03178f5dbd9b35016. Just a guess because i am not able to build this version.

Rename forceRoleEditingFinished() to finishRoleEditing()
Rebase to latest master.

Reverting to previous patch, beacause reacting to KItemListView::scrollOffsetChanged-signal is independent of what/who actual causes the
scrolling.

In D8822#169322, @rkflx wrote:

Catch the scrolling before it propagates (current patch, but maybe needs to be caught in a different place, i.e. you could try to investigate where scrollOffsetChanged is triggered from).

The problem with catching the wheel event in a different place is that i still need a "connection" between this new place and the KItemListRoleEditor (because the new name is stored in there). So i don't know if that makes it simpler. It would be easier/prettier if we cancle the renaming instead of accepting it, because in that case i don't care about KItemListRoleEditor. But accepting it is the right way to go :)
Unless I find a better solution for it, I'll just go back to my previous approach.

In D8822#168505, @rkflx wrote:

@akrutzler Thanks for the analysis, sounds like you are onto another fix :) If you are interested (if not, I can also recommend a few Gwenview bugs): https://bugs.kde.org/show_bug.cgi?id=334533#c7

334533 seems to be a tricky one, but definitely related to this Model/View bug, lets see what i can do. I am not very familiar with gwenview as a user, but yes i can have a look at it :)

This patch is pretty simple now. The behaviour is slightly different. Now the renaming gets accepted once you turn the mouse wheel, even if
there is actually nothing to scroll. But i like it that way too :)

In D8822#168268, @rkflx wrote:

I still don't understand what is the actual problem in the bug, but at least it is not triggered anymore and the floating text input field is gone.

The actual problem behind this is an "miscommunication" between View and Model. If you want to rename something, you tell the KItemListView to change the text-role of one of its items by calling KItemListView::editRole(int index, const QByteArray& role). index defines which item to change. But this index is not persistent! Once you scroll through the list, that index may change (because of some performance optimizations in KItemListView::doLayout()i guess). Because of this, the index of an Item in the View differs from the index in the Model(where necessary data like the URL is stored). But by design they must stay the same!

Removed KItemListRoleEditor::encodedFileName() function.

That's just because you click (anywhere, really, not only on the scrollbar), scrolling with the mouse wheel does not trigger renaming as your video shows and unlike Windows.

Yeah you are right.

Windows Explorer is doing it differently: Renaming is confirmed as soon as scrolling starts. On macOS, Finder allows scrolling and confirms once the file disappears from the viewport.
Finder's implementation doesn't sound too bad, but maybe Dolphin's current behaviour of not moving the rename field is intentional? You'd have to ask the Dolphin folks (IRC, kfm-devel list, tag someone on Phab) about that and what they prefer.

To be honest, I also don't know if that is a bug or a feature, but my first thought was, hm there is something broken. :) Ok, have checked that again and Dolphin also renames it as soon as you scroll with the scroll bar.

item and capabilities are const now.

Maybe I should set up phabricator locally on my machine to figure out all its magic :)

Next try :)

Sorry, first time using arc.

I was using this feature the last few days, and I encountered a small bug: there is no check if we have sufficient permission to rename something.
Reproduce:

• Go to a directory with no write permission (/usr/ for example)
• Two clicks to rename
• -> inline renaming starts
• -> rename
• error message Access denied to xxxx.

So we should only start two clicks renaming if we are allowed to.
That's rather easy to fix, the only question for me is: should I open a new revision or reopen this one?

In D7647#160812, @rkflx wrote:

Thanks again. Looking forward to more patches from you, hopefully with less complicated reviews :) Let us know if you need ideas for what to work on next or any help with the processes…

Not sure whether an application will be sucessful with just a single accepted patch, but judging the quality and responsiveness of your work you should not have a problem submitting one or two more patches.

I like the reviewing, it gave me proof that you're taking care of kde :) I'm looking forward to supply patches for Dolphin / KIO, so do not hesitate to assign bugs / features to me. Otherwise, I will find some on bugzilla :)

In D7647#160644, @ngraham wrote:

Big +1 from me. Ship it! This is great work.

Andreas, you should consider applying for a developer account with commit access. The standards can't be too high, since they let a bozo like me in! ;-)

In D7647#160639, @rkflx wrote:

@akrutzler Assuming you do not have commit rights, what is your email address to be used when committing on your behalf? (identity.kde.org somehow does not know you, even if you are on Phabricator…) The same as the one on bugzilla?

Dragging items cancels twoClicksRenaming now.

In D7647#160250, @rkflx wrote:

With your patch, explaining, understanding and executing the action is much harder:

• Fast double click to open.
• Click to select.
• Second click to rename, but make sure to remember the fine print:
  • There is an additional timing constraints, which cannot be explained by "fast double click". Got to systemsettings, find the module and the setting, multiply by three, practice five times a week to get a feel for this interval.
  • Got it eventually? Great. Now make sure your are not clicking too fast, but also not too slow.
  • Coming back to the window and item is already selected? Well, is this case, for the feature to work you need to "select" again with a first click, without visual feedback though. We know this is inconsistent and for computers the first click does not turn sour like milk, but let's just pretend it does.
    
    I hope you don't mind too much the style I wrote this in. My point is, that in the real world this is a much bigger problem than starting an accidental action (rename in this case), because there is so much else that executes on single click that users are already accustomed to it. The "double click to open" / "single click to select" really is just a feature to make selecting easier, not to require two clicks for every action.
    
    But that are just my two cents. Apart from that your patch works fine and ultimately the maintainers of Dolphin should decide on something final, so this Diff can be closed eventually (it is dragging along for too long already).

In D7647#158673, @rkflx wrote:

I don't think it is "hybrid" in any way. The feature is very well defined, it says Double-click to open files and folders, and that's exactly what we are doing even on focus change. This only targets Dolphin's main view, all other buttons and the Places panel and so on are still single click and also already execute directly on focus change.

If you change focus after selecting an item and then give focus back to dolphin by clicking the selected item it starts the edit. In my book that is not what you want ;)

The focus lost-abort triggers abortTwoClicksRenaming() before the second click. That means that changing focus window does not cancel the rename.

In D7647#155604, @elvisangelaccio wrote:

In D7647#155581, @rkflx wrote:

I likely wanted to just give focus back to Dolphin, rather than start a renaming operation.

On the other hand, executing actions when giving focus is already happening today, e.g. activating Dolphin by clicking on a folder will enter this folder. Shouldn't we rather be consistent here?

Right, I didn't think about that. Ok, ignore my previous comment then :)

In D7647#156773, @markg wrote:

In D7647#156686, @ngraham wrote:

@elvisangelaccio and @markg, how's this looking now?

I don't see the suggested changes so a +1 is a bit premature.

• Better way to abort twoClicksRenaming if the selection of items has changed.
• Use the double-click-interval from Systemsettings instead of the fixed interval of 800ms.
• Fixed some styling.

In D7647#155581, @rkflx wrote:

I likely wanted to just give focus back to Dolphin, rather than start a renaming operation.

On the other hand, executing actions when giving focus is already happening today, e.g. activating Dolphin by clicking on a folder will enter this folder. Shouldn't we rather be consistent here?

Andreas: Could you comment on D7647#inline-34823?

Changed "fastRenaming" to "twoClicksRenaming".

Then "twoClicksRenaming" it should be :) But since I just came home from Qt World Summit, I will change that tomorrow.

Oh I thought we just leave it like that ^^

In D7647#146860, @sars wrote:

The point 2) problem that I described is what I get from a Windows 7 installation.... Wonder why you get a different result.

Now this feature behaves as follows:

1. select an item by single-click, or one is already selected
2. wait the "double-click-interval"
3. click on the items text ( -> 800ms timer gets started)
4. none of the cancellations (see below) happens within that 800ms
5. inline-renaming starts