Make renaming of files/folders faster by clicking a second time on the items text to start renaming.
BUG: 205157
Details
Details
- Reviewers
elvisangelaccio emmanuelp ngraham rkflx markg - Group Reviewers
Dolphin KDE Applications - Commits
- R318:5454283008f2: Two clicks on file/folder to rename
This feature works as follows:
- select an item by single-click, or one is already selected
- wait the "double-click-interval"
- click on the items text
- none of the cancellations (see below) happens within the double-click-interval
- inline-renaming starts
Cancellations:
- open any file/folder
- select different item(s)
- start dragging items
- Dolphin loses focus
This feature is just enabled while "Double-click to open files and folders" in system-settings and "Rename inline" in Dolphin are enabled.
Diff Detail
Diff Detail
- Lint
Lint Skipped - Unit
Unit Tests Skipped
There are a very large number of changes, so older changes are hidden. Show Older Changes
Comment Actions
Great work, Andreas. I just had a look and also compared with Windows, it is nearly perfect now (and I do not think further restrictions as mentioned in D7647#148724 should be added).
| src/views/dolphinview.cpp | ||
|---|---|---|
| 198 | While the initial activation respects the double click interval set in systemsettings, shouldn't this also be the case here (i.e. when double clicking on an already selected item)? | |
Comment Actions
I do have another comment: if Dolphin loses focus the two clicks renaming should be aborted:
Currently if you:
- select some item in dolphin
- focus some other program's window
- click the previously select item in the dolphin view
then the renaming operation would start.
This could be annoying, as I likely wanted to just give focus back to Dolphin, rather than start a renaming operation.
Comment Actions
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?
| src/views/dolphinview.cpp | ||
|---|---|---|
| 198 | I hope I got you right, but yes it also respects double-clicking an already selected item. So it doesn't matter if selected or not, double-clicking will always open the file/folder. | |
| src/views/dolphinview.cpp | ||
|---|---|---|
| 198 | This is about the double click interval, not about double clicking. I set it to 2000ms in systemsettings, but your code waits only 800ms. But maybe such a change would have other implications I have not thought of? | |
Comment Actions
Right, I didn't think about that. Ok, ignore my previous comment then :)
| src/kitemviews/kitemlistcontroller.cpp | ||
|---|---|---|
| 587 | I'd keep this if() in a single line | |
| 589 | Too much indentation here. | |
| src/views/dolphinview.cpp | ||
| 103 | let's use nullptr for new code | |
| 649 | if (!fromTwoClicksRenamingTimer) But actually, if fromTwoClicksRenamingTimer would be declared inside the previous if block, we wouldn't need to check its value in this else if, right? | |
| src/views/dolphinview.cpp | ||
|---|---|---|
| 819 | Please move this comment inside the if() block, above the hideToolTip() call. | |
Comment Actions
I agree with rkflx. IMHO in case of consistency we should also start inline renaming there.
| src/views/dolphinview.cpp | ||
|---|---|---|
| 103 | Ok, but I actually don't know why initializing it as nullptr at this point, because it will be initialized in the same constructor with "new QTimer(this)" afterwards anyway. I just wanted to stick to the current style. | |
| 198 | Ah you mean that the (now) 800ms interval should depend on the double click interval set in systemsettings? | |
| 649 | I assume with "previous if block" you mean: if (items.count() == 1 && GeneralSettings::renameInline()) { ... }That's actually not true, because imagine this case:
I tried to abort the twoClicksRenaming (during that 800ms) if the selected item(s) have changed (like I do on focus-lost for example, line 822) but I couldn't find a proper way (signal/slot) how to do that. So I decided to check here, if the selected items have changed. And if there are more items selected, twoClicksRenaming should never trigger. If I could abort twoClicksRenaming on item-selection-change, I wouldn't need m_twoClicksRenamingItemIndex either. There are signals which reports an item change but I couldn't use them because they were firing even if nothing changed at all. | |
| src/views/dolphinview.cpp | ||
|---|---|---|
| 649 | I see, then there should be a comment explaining this logic. Btw which signals have you tried? What about KItemListSelectionManager::selectionChanged? | |
| src/views/dolphinview.cpp | ||
|---|---|---|
| 103 | We could also do: QTimer* m_twoClicksRenamingTimer = nullptr; in dolphinview.h and drop the variable from the initializer list. | |
| src/views/dolphinview.cpp | ||
|---|---|---|
| 198 | You got what I meant . And I get what you meant regarding the initial activation (my observation was slightly misleading, so I will adapt my suggestion). There are two cases:
To distinguish between both cases, you'll probably need a second timer to wait some amount of time. Maybe something between 1 and 10 seconds (whatever "feels" right), but ideally calculated proportional to the DCI. Note: I only thought about the double clicking case so far. You should probably also consider how this works together with selecting multiple items and the other cancellation reasons. | |
Comment Actions
I'm OK with this feature after the code is correct.
So with probably my suggestion and those of @elvisangelaccio
| src/views/dolphinview.cpp | ||
|---|---|---|
| 103 | Most certainly not! You can change it to: that would be OK imho. | |
| src/views/dolphinview.cpp | ||
|---|---|---|
| 103 | "Most certainly not!" was for the suggestion of initializing the member in the header, the comment from @elvisangelaccio | |
| src/views/dolphinview.cpp | ||
|---|---|---|
| 103 | It think we should stay with the init like in m_selectionChangedTimer to stay consistent. With the next major we could change that kind of stuff. | |
| 198 | I think we should not make that too complicated. I just set the interval from 800ms to the DCI and it works fine for me. Now it always waits the DCI and if no cancellation happened, inline renaming starts. But I have to wait in both of your 2 cases for the DCI. | |
| 649 | I found an rather easy solution to solve this problem by aborting twoClicksRenaming in the corresponding slot DolphinView::slotSelectionChanged. (I don't know why I didn't came up with that solution from the beginning). | |
Comment Actions
- 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.
Comment Actions
I set all comments to "Done" where I know they are done. There are still some left with no further comments from you guys, so they are done too?
Comment Actions
Then it's fine by me.
You could still initialize the m_twoClicksRenamingTimer in the constructor like:
m_twoClicksRenamingTimer(new QTimer(this)), but i'm fine either way.
If you don't do it then that 0 initialize should be a nullptr: m_twoClicksRenamingTimer(nullptr).
Comment Actions
I only looked at the DCI issue, which now LGTM. (I don't think waiting for an implementation of the ideal behaviour I described above should block this patch).
Comment Actions
Hi,
I just tried the patch and I don't think the cancellation works as intended. The cancellation only works for double-click or selection change during the double-click period. The focus lost-abort triggers abortTwoClicksRenaming() before the second click. That means that changing focus window does not cancel the rename.
I have an addition to this patch that adds an other QTimer that is started in slotSelectionChanged() and checked/started in slotSelectedItemTextPressed(). If the timer is not running/active when slotSelectedItemTextPressed() is executed, the renaming is not started but the timer is tarted for the next try.
This removes a _lot_ of mistakenly started renames, but does make it a bit harder to trigger the rename. I set the time to 3xDCT for now.
(How) do you want me to provide the update?
| src/views/dolphinview.cpp | ||
|---|---|---|
| 1166 ↗ | (On Diff #20810) | This aborts the rename only during the double-click delay timeout and not if we select the item, focus another window and then click the item again. |
| src/views/dolphinview.cpp | ||
|---|---|---|
| 818 ↗ | (On Diff #20810) | Is this a left-over? We don't want to cancel when losing focus, and this is a no-op when called from here anyway? |
Comment Actions
The focus lost-abort triggers abortTwoClicksRenaming() before the second click. That means that changing focus window does not cancel the rename.
The focus-lost-cancellation works pretty well for me.
I have an addition to this patch that adds an other QTimer that is started in slotSelectionChanged() and checked/started in slotSelectedItemTextPressed(). If the timer is not running/active when slotSelectedItemTextPressed() is executed, the renaming is not started but the timer is tarted for the next try.
This removes a _lot_ of mistakenly started renames, but does make it a bit harder to trigger the rename. I set the time to 3xDCT for now.
(How) do you want me to provide the update?
Since we are very close to finish this patch, opening a new revision would be appropriate.
| src/views/dolphinview.cpp | ||
|---|---|---|
| 818 ↗ | (On Diff #20810) | We want to cancle on focus lost. This is called every time if dolphin loses focus. |
| src/views/dolphinview.cpp | ||
|---|---|---|
| 818 ↗ | (On Diff #20810) |
We are probably talking about the same thing, but that's not what this line does. After re-examining: This is about cancelling a pending rename after the second click on focus loss, but before the lineedit activated. So the reason is different, but you should keep it :) |
Comment Actions
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 ;)
I have an addition to this patch that adds an other QTimer that is started in slotSelectionChanged() and checked/started in slotSelectedItemTextPressed(). If the timer is not running/active when slotSelectedItemTextPressed() is executed, the renaming is not started but the timer is tarted for the next try.
This removes a _lot_ of mistakenly started renames, but does make it a bit harder to trigger the rename. I set the time to 3xDCT for now.
(How) do you want me to provide the update?
Since we are very close to finish this patch, opening a new revision would be appropriate.
The change is small and fixes problems with starting editing when single-clicking selected items. I don't think this patch should go in without something mitigating the single-click problem.
Comment Actions
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 ;)
That's indeed what we want. Have a look at my reply to your last inline comment :)
The change is small and fixes problems with starting editing when single-clicking selected items. I don't think this patch should go in without something mitigating the single-click problem.
Hm its hard for me to understand the benefit of this implementation. Please send me your patch to andreas.krutzler@gmx.net and i can try to understand it then ;)
Comment Actions
Well, it seems that we just do not agree on this then.
Currently, in Dolphin, if you are in double-click mode, a single click on an item will never do anything else than modify the selection*. This "Two click rename" will introduce something that basically is a "Single click" action in some situations. This is what is "breaking somebody's work-flow" ;)
(* Single clicking the expand/collapse arrow does expand/collapse a folder, but that could be argued to be related to item selection)
We do not need to make an exact copy of the Windows behavior. We can improve it!
Ps. On Windows a focus change does prevent renaming, if the click on the item gives the focus to the window, which this patch does not.
Comment Actions
I resign (for now), i need to retest this.
Sars brought up a very valid point that i didn't even consider yet. You do introduce some hybrid between single- and double click flows while potentially breaking current flows.
To be continued..
Comment Actions
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.
Furthermore, it says (select icons on first click), which is the behaviour of the patch too (remember the icon will have been already selected at this point).
Comment Actions
Exactly, that's how i think about this too. Especially when it comes to the "Places pane": there, it's always execution on single-click, no matter if "single" or "double click" to open stuff is activated. :)
Yes, that differs from the Windows implementation, but this makes it IMO more consistent. ;)
Comment Actions
Here is my solution for the single-click problem. (two different patches on with only the time restraint and the other with everything)
This basically adds a QTimer that is started either when the selection changes or when the selected item is clicked. Then when the selected item is clicked we check if the timer is active. If it is active we start the waiting for the double-click-time and eventually editing. If the timer is not active we just start it. This means that it always requires two clicks to trigger the rename.
Note: The timeout of the timer in this patch is 3*double-click-time, but should probably be a bit longer.
Comment Actions
While it is nice you provide a patch and not just comments, first we need to reach a consensus whether this is the behaviour we want. So far, you just keep repeating your stance without any new convincing arguments (at least to me).
Let me detail why I wrote above that for me this is not something with a net benefit to the user experience. Without your patch, explaining the renaming feature to the casual user is easy:
- Fast double click to open.
- Click to select.
- Second click to rename.
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).
Comment Actions
Wow, I found another issue (independent from the patch above):
- Click to select, make sure to release mouse button.
- After DCI, drag item.
- Renaming unintentionally starts, preventing the drop action to show its context menu.
Probably we should start renaming only on mouse up (if cursor is still above the item) and not already on mouse down? Or abort renaming when dragging mode is entered?
Comment Actions
And again, I totally agree with you!
Thanks! :) It seems to me that you just found a new cancellation. So if one starts to drag an item, renaming needs to be canceled too. I will have a look at it.
Comment Actions
Dragging items cancels twoClicksRenaming now.
Also:
The detection, if the item selection has changed, takes place once after the timeout of twoClicksRenamingTimer now. Before this patch, this was done each time the selection changed.
Comment Actions
Thanks, works great.
@elvisangelaccio Does your LGTM still hold? If so, please accept again, as Phab still shows this Diff as blocked / "Needs Review".
@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?
Also:
The detection, if the item selection has changed, takes place once after the timeout of twoClicksRenamingTimer now. Before this patch, this was done each time the selection changed.
I like how you also changed from index to url. Together, this looks much cleaner now. Your test plan is much improved, too.
Comment Actions
No, I have no rights. :) I am using andreas.krutzler@gmx.net for identity and bugzilla so this one should be used here too. Hm strange, I can find myself at identity.kde.org.
Comment Actions
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! ;-)
Should we wait a bit before landing this? I'm inclined to support doing it now.
Comment Actions
Thanks! :) I would definitely apply for a developer account as soon as I become more familiar with the entire kde development process.
Should I close this revision?
Comment Actions
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.
Should I close this revision?
Phabricator closes the revision automatically once someone commits your patch. I can do that tomorrow, unless Nate beats me to it…
Comment Actions
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 :)
Comment Actions
Fantastic. I'm focusing on Dolphin as well these days. If you want some ideas, here's my list of Dolphin and KIO bugs: https://paste.kde.org/pch6vp01q
Comment Actions
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?
Comment Actions
Great idea to improve the usability even further.
I'd say just open a new Diff, as this has been committed already and most of the discussion here is probably not relevant and just bogs down any reviewer. You can always add a reference to this Diff or even a specific comment by mentioning the Dxxx number in the new Diff, and Phab would even add a note over here that you mentioned this Diff elsewhere (if you used the full URL, Phab would not do that).
Comment Actions
This apparently caused regressions as I noticed:
1.) Long-clicking on a selected file now triggers inline renaming in single-click mode too.
Or was that intentional? The summary states this though:
This feature is just enabled while "Double-click to open files and folders" in system-settings and "Rename inline" in Dolphin are enabled.
In any case, I'd find this confusing as it only happens with selected files, not unselected ones.
2.) Dragging files can trigger inline editing too now, in both single- and double-click mode. (and even worse, it seems this can even cause unrelated files to be renamed to something completely different due to https://bugs.kde.org/show_bug.cgi?id=334533)
See https://bugs.kde.org/show_bug.cgi?id=398375
Reverting this change fixes both.
Comment Actions
I am able to reproduce them both. @wbauer do you want to fix them? If not, I can spend some time on it. :)
Comment Actions
Not that I am aware of.
And I didn't file one because I wasn't sure if it was intentional or not (a wrong commit message is not really a reason for a bug report, is it? ;-) ).
Actually, I'd find it quite ok to be able to rename files by long-clicking, even in single-click mode.
I don't really see why this should be restricted to already selected files though (long-click should be enough of protection against accidentally triggering it I think).
No strong opinion on that though.
TBH, I would prefer if you'd try to fix it.
I'm not really familiar with the code (e.g. I don't know yet what m_dragging is used for originally...).
I was going to investigate further, but haven't find the time for that yet, you can see my findings so far in https://bugs.kde.org/show_bug.cgi?id=398375 though.