D7647: Bug 205157 - two clicks on file to rename

[Andreas Krutzler]

akrutzler created this revision.
Restricted Application added a subscriber: Konqueror.

REVISION SUMMARY
Hello,
since my review request is 10 months old (https://git.reviewboard.kde.org/r/128715/) and today was one of these days where a feature like that would be rather handy, I decided to push this feature again :)

1. The single/double-click problem:

To correct myself, I also think this feature just works in double-click mode.
There are some cases to trigger the renaming in single-click mode too, but in my opinion that’s not quite intuitive.

2. The functionality

"funky bomber" described it very well how this should work.
The only thing I would change is that the inline-renaming ONLY triggers if one clicks on the text/name of the highlighted item. (that’s how the windows-file-manager do it too)

3. The additional option

As we apparently break someone's workflow, we should make this feature optional to respect "slow-clickers".

All of this is included in my current patch. It builds against the latest master so it should be easily reviewed.

Best regards
Andreas

REPOSITORY
R318 Dolphin

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

AFFECTED FILES
src/kitemviews/kitemlistcontroller.cpp
src/kitemviews/kitemlistcontroller.h
src/kitemviews/kitemlistview.cpp
src/kitemviews/kitemlistview.h
src/settings/dolphin_generalsettings.kcfg
src/settings/general/behaviorsettingspage.cpp
src/settings/general/behaviorsettingspage.h
src/views/dolphinview.cpp
src/views/dolphinview.h

To: akrutzler, #dolphin, #kde_applications
Cc: #konqueror

[Elvis Angelaccio]

elvisangelaccio requested changes to this revision.
elvisangelaccio added a comment.
This revision now requires changes to proceed.


Thanks for the re-uploading the patch.

I'm against adding new options unless there is a really good reason to.
If you are adding this new option only because you'd break someone else's workflow, then don't break other people's worflows ;)

From a quick test, this is what I don't like:

- I if double-click a selected file, I want to open it, not rename it. The patch currently breaks this use case.
- The fast renaming feature should only work with the inline renaming mode.

REPOSITORY
R318 Dolphin

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

To: akrutzler, #dolphin, #kde_applications, elvisangelaccio
Cc: elvisangelaccio, ngraham, #konqueror

[Elvis Angelaccio]

elvisangelaccio added a reviewer: emmanuelp.

REPOSITORY
R318 Dolphin

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

To: akrutzler, #dolphin, #kde_applications, elvisangelaccio, emmanuelp
Cc: elvisangelaccio, ngraham, #konqueror

[Nathaniel Graham]

ngraham added a comment.


I agree with Elvis that this should not require a new option. The solution to the double-clicking problem is to make the rename only happen if the file has been selected more than, say, 750ms. That will prevent rename mode from being activated when a user double-clicks on the filename part of the file. This is now this feature works in other file managers.

REPOSITORY
R318 Dolphin

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

To: akrutzler, #dolphin, #kde_applications, elvisangelaccio, emmanuelp
Cc: elvisangelaccio, ngraham, #konqueror

[Andreas Krutzler]

akrutzler added a comment.


Thanks for your response! :)

When it comes to the additional option, I am on your side but just because I will turn it on anyway.

Since I have uninstalled my last Windows system month ago I cannot check this, but I think this feature behaves exactly like in Windows Filemanager (this can be good or not).
As I mentioned before, the renaming just gets triggered if you click on the text. Clicking eg on the icon will not triggered it.
Making this time-dependent creates a small inconsistency in my opinion. Because sometimes it gets triggered if selected and sometimes not.

REPOSITORY
R318 Dolphin

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

To: akrutzler, #dolphin, #kde_applications, elvisangelaccio, emmanuelp
Cc: elvisangelaccio, ngraham, #konqueror

[Nathaniel Graham]

ngraham added a comment.


This needs to be time-dependent or else double-clicking on the text of an unselected file will enter rename mode rather than open it, which is not expected behavior with any other file manager on any other platform. Neither Windows Explorer nor MacOS Finder enter rename mode when you double-click on a file's filename. ...Nor do Nautilus, or Nemo, or Caja, Pantheon-Files, or any others that I'm aware of. In all of them, the click-to-rename procedure is as follows:

1. select file/folder by any means
2. wait 500ms - 1s
3. click on the filename and it becomes editable inline

REPOSITORY
R318 Dolphin

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

To: akrutzler, #dolphin, #kde_applications, elvisangelaccio, emmanuelp
Cc: elvisangelaccio, ngraham, #konqueror

[Elvis Angelaccio]

elvisangelaccio added a comment.


Another (radical) way of fixing it could be: enable fast renaming only after a single _long_ click.

- Pro: completely unambiguous. No way to trigger unwanted renames.
- Cons: different behavior with respect to other filemanagers. It would be an "hidden" feature.

REPOSITORY
R318 Dolphin

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

To: akrutzler, #dolphin, #kde_applications, elvisangelaccio, emmanuelp
Cc: elvisangelaccio, ngraham, #konqueror

[Nathaniel Graham]

ngraham added a comment.


I wouldn't oppose adding that in addition, but I think we should make sure we add a mode that mimics the "standard" behavior, which is what people expect when they switch to Plasma from their inferior platforms. :)

REPOSITORY
R318 Dolphin

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

To: akrutzler, #dolphin, #kde_applications, elvisangelaccio, emmanuelp
Cc: elvisangelaccio, ngraham, #konqueror

[Andreas Krutzler]

akrutzler added a comment.


I am currently not able to test it, but by double clicking (within the standard time interval) the text should not trigger fast renaming. If that's currently the case I have to fix my patch.

REPOSITORY
R318 Dolphin

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

To: akrutzler, #dolphin, #kde_applications, elvisangelaccio, emmanuelp
Cc: elvisangelaccio, ngraham, #konqueror

[Andreas Krutzler]

akrutzler added a comment.


Back from holidays :)

I checked the behavior regarding double-clicking the text and I thought right that fast-renaming doesn't get triggered. This only happens if one double-clicks an already selected item.
This may happen if one:

- select an item
- wait the "double-click-interval" (which is 400ms by default, can be changed in system settings)
- double-click the item
- -> fast renaming starts by the first click of the double-click

or

- enters a directory
- leaves this directory (directory item stays selected)
- double click the directory again
- -> fast renaming starts

Is that the thing you are worried about?

I thought you mean an additional time interval which describes how long fast-renaming is enabled. like:
time-intervals: |<--double-click (off)-->|<--fast-rename (on)-->|<--... (off)----

But, as I see it, double-clicking an unselected item works as before :)

REPOSITORY
R318 Dolphin

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

To: akrutzler, #dolphin, #kde_applications, elvisangelaccio, emmanuelp
Cc: elvisangelaccio, ngraham, #konqueror

[Kåre Särs]

sars added a comment.


The problematic things for me are:

1. If I have selected a file, do something else and then come back and click the item "to ensure the window has focus", before doing some action on the item, it goes into "Fast edit mode".

2. (On Windows) I right click the file and then want to cancel the pop-up dialog and I click over the selected file it will enter "fast edit" in stead of just closing the pop-up.

3. If it actually will work as described in the previous comment (and the code suggests) that would be quite bad! (I have not compiled and tried this). If double-clicking an already selected item enters edit-mode that would be very not wanted!

This could be solved by entering edit-mode **only** if the time from the previous click was greater than the double-click-time, but less than say ~2 seconds ago. Was this what you tried to say with "time-intervals: ..."?

REPOSITORY
R318 Dolphin

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

To: akrutzler, #dolphin, #kde_applications, elvisangelaccio, emmanuelp
Cc: sars, elvisangelaccio, ngraham, #konqueror

[Andreas Krutzler]

akrutzler added a comment.

Post by Kåre Särs
1. If I have selected a file, do something else and then come back and click the item "to ensure the window has focus", before doing some action on the item, it goes into "Fast edit mode".
2. (On Windows) I right click the file and then want to cancel the pop-up dialog and I click over the selected file it will enter "fast edit" in stead of just closing the pop-up.
3. If it actually will work as described in the previous comment (and the code suggests) that would be quite bad! (I have not compiled and tried this). If double-clicking an already selected item enters edit-mode that would be very not wanted!
This could be solved by entering edit-mode **only** if the time from the previous click was greater than the double-click-time, but less than say ~2 seconds ago. Was this what you tried to say with "time-intervals: ..."?

ad 1 & 3) I see the problem now. I managed to get an Windows 7 system running and indeed the behavior is different.
Are you cool with the solution they did in Windows? If so, I will figure out how to fix that and update my patch ;)

ad 2) That differs from the Windows solution right now. If you have an pop-up open and you click on the selected item, it will just close the pop-up.

REPOSITORY
R318 Dolphin

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

To: akrutzler, #dolphin, #kde_applications, elvisangelaccio, emmanuelp
Cc: sars, elvisangelaccio, ngraham, #konqueror

[Kåre Särs]

sars added a comment.


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

I think what they do in Windows is to wait the double click time before triggering the single-click action.

My problem here is that if you select "double-click to open" you do not expect a single click to trigger an action on the item.

I would not +1 a solution that is identical to the Windows behavior, but then again I'm just a dolphin user ;)

Making it a long press and/or slow-double-click would be an improvement over the Windows behavior.

REPOSITORY
R318 Dolphin

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

To: akrutzler, #dolphin, #kde_applications, elvisangelaccio, emmanuelp
Cc: sars, elvisangelaccio, ngraham, #konqueror

[Andreas Krutzler]

akrutzler updated this revision to Diff 19664.
akrutzler added a comment.


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

**Cancellations:**

- open any file/folder by double-click
- select different item(s)
- Dolphin loses focus
- anything else?

This feature is just enabled while "Double-click to open files and folders" in system-settings and "Rename inline" in Dolphin are enabled.
I also removed the additional option "Fast renaming" in Dolphin.

REPOSITORY
R318 Dolphin

CHANGES SINCE LAST UPDATE
https://phabricator.kde.org/D7647?vs=19074&id=19664

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

AFFECTED FILES
src/kitemviews/kitemlistcontroller.cpp
src/kitemviews/kitemlistcontroller.h
src/kitemviews/kitemlistview.cpp
src/kitemviews/kitemlistview.h
src/views/dolphinview.cpp
src/views/dolphinview.h

To: akrutzler, #dolphin, #kde_applications, elvisangelaccio, emmanuelp
Cc: sars, elvisangelaccio, ngraham, #konqueror

[Andreas Krutzler]

akrutzler added a comment.

Post by Kåre Särs
The point 2) problem that I described is what I get from a Windows 7 installation.... Wonder why you get a different result.

On Windows 7 I get the same result as you, my patch for Dolphin just closes the pop-up :)

Post by Kåre Särs
Making it a long press and/or slow-double-click would be an improvement over the Windows behavior.

I wonder if a long-press can do the job. I will try this and maybe I like this even more ;)

REPOSITORY
R318 Dolphin

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

To: akrutzler, #dolphin, #kde_applications, elvisangelaccio, emmanuelp
Cc: sars, elvisangelaccio, ngraham, #konqueror

[Nathaniel Graham]

ngraham accepted this revision.
ngraham added a comment.


This works really well in my testing. I like it. Let's implement rename-with-long-press in another patch.

REPOSITORY
R318 Dolphin

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

To: akrutzler, #dolphin, #kde_applications, elvisangelaccio, emmanuelp, ngraham
Cc: sars, elvisangelaccio, ngraham, #konqueror

[Nathaniel Graham]

ngraham requested changes to this revision.
ngraham added a comment.
This revision now requires changes to proceed.


Actually, could you edit the Summary as follows?

1. Add "BUG: 205157"
2. Rewrite to be more factual regarding the change and less conversational

That will make git git log cleaner when we commit the patch.

REPOSITORY
R318 Dolphin

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

To: akrutzler, #dolphin, #kde_applications, elvisangelaccio, emmanuelp, ngraham
Cc: sars, elvisangelaccio, ngraham, #konqueror

[Andreas Krutzler]

akrutzler retitled this revision from "Bug 205157 - two clicks on file to rename" to "Two clicks on file/folder to rename".
akrutzler edited the summary of this revision.

REPOSITORY
R318 Dolphin

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

To: akrutzler, #dolphin, #kde_applications, elvisangelaccio, emmanuelp, ngraham
Cc: sars, elvisangelaccio, ngraham, #konqueror

[Nathaniel Graham]

ngraham accepted this revision.

REPOSITORY
R318 Dolphin

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

To: akrutzler, #dolphin, #kde_applications, elvisangelaccio, emmanuelp, ngraham
Cc: sars, elvisangelaccio, ngraham, #konqueror

[Nathaniel Graham]

ngraham added a comment.


@elvisangelaccio, does this look good now?

REPOSITORY
R318 Dolphin

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

To: akrutzler, #dolphin, #kde_applications, elvisangelaccio, emmanuelp, ngraham
Cc: sars, elvisangelaccio, ngraham, #konqueror

[Kåre Särs]

sars added a comment.


This new iteration is much better :)

I still think it has to be a kind of "slow double click". Meaning that if you wait too long before you click the second time the action should be canceled. The action should only be triggered if the second click is within a reasonable time from the first click.

Scenarios:

DCT = double-click time

1. Click an item to select it, wait more than 3xDCT (or even > 25 seconds), click the same item again once.

In this case I **don't** want the item to enter rename mode.

2. Nothing selected: Click an item to select it, click the item again after 1xDCT, but before 3xDCT.

In this case I would expect a rename.

3. An item is selected more than 3xDCT ago. Click the item, wait a bit longer than 1xDCT, click it again before 3xDCT has elapsed.

In this case I would expect a rename.

I don't think you would need too much changes to this patch to get it working like this. I already have an idea how it could be done ;)
It only needs one more QElapsedTimer.

REPOSITORY
R318 Dolphin

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

To: akrutzler, #dolphin, #kde_applications, elvisangelaccio, emmanuelp, ngraham
Cc: sars, elvisangelaccio, ngraham, #konqueror

[Kåre Särs]

sars added inline comments.

INLINE COMMENTS

dolphinview.h:810
+ QTimer* m_fastRenamingTimer;
+ int m_fastRenamingItemIndex;

QElapsedTimer could be better here.

REPOSITORY
R318 Dolphin

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

To: akrutzler, #dolphin, #kde_applications, elvisangelaccio, emmanuelp, ngraham
Cc: sars, elvisangelaccio, ngraham, #konqueror

[funkybomber]

funkybomber added a comment.

Post by Nathaniel Graham
This works really well in my testing. I like it. Let's implement rename-with-long-press in another patch.

Hello everyone, and thank you for your time and effort to get this bug fixed! :)

I would like to raise some concerns regarding the "single-long-click to rename" approach:

**1) **It might introduce ambiguity / usability issues in the following scenarios:

- 1.1) When a user attempts to do a drag-n-drop and he/she simply is too slow to start moving the mouse. Having noticed the behaviour of novice users (my parents), sometimes they do take their time when they hold down the left click before they start moving the mouse around to drag-n-drop a file. With the "single-long-click to rename" approach, the file they wanted to move would just enter "rename mode".

- 1.2) When a user attempts to do a drag-n-drop using a laptop trackpad. Basically the same issue, but it will affect a lot more people since practically everyone works more slowly/awkwardly with a trackpad.

**2) **It can potentially introduce extra complexity.

Who will determine after how long a rename action is triggered? There will obviously need to be a new slider (or a user input value "long-click-trigger-time" in microseconds) for the users to be able to tweak that. In comparison, the "two clicks to rename" approach doesn't really require ANY extra user input value since the "double-click-interval" value already exists.

**3)** Might not be as efficient as the "two clicks to rename" approach under common multiple file rename scenarios.

Let's consider a typical multiple file rename scenario. Let's say I have a folder with 20 files and I want to rename them all.

Let's see how the "two clicks to rename" approach works:

Steps:
i) Press "Down" key to select --> The file we want to rename is now selected (all good)
ii) Click on the text of the file --> The filename is now editable (all good)
iii) Click on the appropriate place to place the cursor before we start typing (all good)

Let's see now how the "single-long-click to rename" approach works:

Steps:
i) Press "Down" key to select --> The file we want to rename is now selected (all good)
ii) Long click on the text of the file --> The filename is now editable (long click feels like a waste of time here)
iii) Click on the appropriate place to place the cursor before we start typing (all good)

REPOSITORY
R318 Dolphin

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

To: akrutzler, #dolphin, #kde_applications, elvisangelaccio, emmanuelp, ngraham
Cc: funkybomber, sars, elvisangelaccio, ngraham, #konqueror

[Nathaniel Graham]

ngraham added a comment.


I think we should avoid turning this into a discussion of whether and how a long click would do a rename operation and stick to the original patch. Once it's in, then we can have a separate discussion about the long click UX.

REPOSITORY
R318 Dolphin

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

To: akrutzler, #dolphin, #kde_applications, elvisangelaccio, emmanuelp, ngraham
Cc: funkybomber, sars, elvisangelaccio, ngraham, #konqueror

[Mark Gaiser]

markg requested changes to this revision.
markg added a comment.


Please, change the naming of "fast renaming".
You could also argue that pressing F2 on a selected item (or multiple even) is even faster renaming.

Call it like it is, i leave it up to you to decide a good name for it. Also because i can't think of any ^_-
But "fast renaming" certainly isn't.

REPOSITORY
R318 Dolphin

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

To: akrutzler, #dolphin, #kde_applications, elvisangelaccio, emmanuelp, ngraham, markg
Cc: markg, funkybomber, sars, elvisangelaccio, ngraham, #konqueror

[Nathaniel Graham]

ngraham added a comment.


This is a pretty minor point since it's just the name in the code, but Mark is right. This isn't really faster than existing methods; what it is is more consistent and familiar, because it's how other platforms handle renaming via the mouse.

Maybe "clickRenaming"?

REPOSITORY
R318 Dolphin

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

To: akrutzler, #dolphin, #kde_applications, elvisangelaccio, emmanuelp, ngraham, markg
Cc: markg, funkybomber, sars, elvisangelaccio, ngraham, #konqueror

[Nathaniel Graham]

ngraham added a comment.


@akrutzler, any progress? I think we're really close to having this merge-worthy.

REPOSITORY
R318 Dolphin

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

To: akrutzler, #dolphin, #kde_applications, elvisangelaccio, emmanuelp, ngraham, markg
Cc: markg, funkybomber, sars, elvisangelaccio, ngraham, #konqueror

[Andreas Krutzler]

akrutzler added a comment.


Oh I thought we just leave it like that ^^

But yes to be honest, I am also not totally okay with "fast renaming". That was just the first thing which came in mind.
I am cool with "clickRenaming" or maybe something which refers more to the original bug report like "twoClicksRenaming" ?

But yeah I think somebody in charge (@ngraham, @elvisangelaccio, @emmanuelp,..) has to nail that down so I can make the final change :)

REPOSITORY
R318 Dolphin

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

To: akrutzler, #dolphin, #kde_applications, elvisangelaccio, emmanuelp, ngraham, markg
Cc: markg, funkybomber, sars, elvisangelaccio, ngraham, #konqueror

[Nathaniel Graham]

ngraham added a comment.


Not that anybody put me in charge (heaven forbid! >:-) ) but I don't think folks will beat a path to your door with pitchforks and torches if you go with twoClickRenaming.

REPOSITORY
R318 Dolphin

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

To: akrutzler, #dolphin, #kde_applications, elvisangelaccio, emmanuelp, ngraham, markg
Cc: markg, funkybomber, sars, elvisangelaccio, ngraham, #konqueror

[Andreas Krutzler]

akrutzler added a comment.


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

REPOSITORY
R318 Dolphin

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

To: akrutzler, #dolphin, #kde_applications, elvisangelaccio, emmanuelp, ngraham, markg
Cc: markg, funkybomber, sars, elvisangelaccio, ngraham, #konqueror

[Henrik Fehlauer]

rkflx added a comment.


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 https://phabricator.kde.org/D7647#148724 should be added).

INLINE COMMENTS

dolphinview.cpp:198
+ m_fastRenamingTimer->setSingleShot(true);
+ m_fastRenamingTimer->setInterval(800);
+ connect(m_fastRenamingTimer, &QTimer::timeout, this, &DolphinView::renameSelectedItems);

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)?

REPOSITORY
R318 Dolphin

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

To: akrutzler, #dolphin, #kde_applications, elvisangelaccio, emmanuelp, ngraham, markg
Cc: rkflx, markg, funkybomber, sars, elvisangelaccio, ngraham, #konqueror

[Andreas Krutzler]

akrutzler updated this revision to Diff 20692.
akrutzler added a comment.


Changed "fastRenaming" to "twoClicksRenaming".

REPOSITORY
R318 Dolphin

CHANGES SINCE LAST UPDATE
https://phabricator.kde.org/D7647?vs=19664&id=20692

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

AFFECTED FILES
src/kitemviews/kitemlistcontroller.cpp
src/kitemviews/kitemlistcontroller.h
src/kitemviews/kitemlistview.cpp
src/kitemviews/kitemlistview.h
src/views/dolphinview.cpp
src/views/dolphinview.h

To: akrutzler, #dolphin, #kde_applications, elvisangelaccio, emmanuelp, ngraham, markg
Cc: rkflx, markg, funkybomber, sars, elvisangelaccio, ngraham, #konqueror

[Nathaniel Graham]

ngraham added a comment.


@elvisangelaccio and @markg, is this acceptable now?

REPOSITORY
R318 Dolphin

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

To: akrutzler, #dolphin, #kde_applications, elvisangelaccio, emmanuelp, ngraham, markg
Cc: rkflx, markg, funkybomber, sars, elvisangelaccio, ngraham, #konqueror

[Elvis Angelaccio]

elvisangelaccio added a comment.


I do have another comment: if Dolphin loses focus the two clicks renaming should be aborted:

Currently if you:

1. select some item in dolphin
2. focus some other program's window
3. 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.

REPOSITORY
R318 Dolphin

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

To: akrutzler, #dolphin, #kde_applications, elvisangelaccio, emmanuelp, ngraham, markg
Cc: rkflx, markg, funkybomber, sars, elvisangelaccio, ngraham, #konqueror

[Henrik Fehlauer]

rkflx added a comment.

Post by Elvis Angelaccio
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 https://phabricator.kde.org/D7647#inline-34823?

REPOSITORY
R318 Dolphin

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

To: akrutzler, #dolphin, #kde_applications, elvisangelaccio, emmanuelp, ngraham, markg
Cc: rkflx, markg, funkybomber, sars, elvisangelaccio, ngraham, #konqueror

[Andreas Krutzler]

akrutzler added inline comments.

INLINE COMMENTS

rkflx wrote in 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)?

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.

REPOSITORY
R318 Dolphin

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

To: akrutzler, #dolphin, #kde_applications, elvisangelaccio, emmanuelp, ngraham, markg
Cc: rkflx, markg, funkybomber, sars, elvisangelaccio, ngraham, #konqueror

[Henrik Fehlauer]

rkflx added inline comments.

INLINE COMMENTS

akrutzler wrote in 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.

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?

REPOSITORY
R318 Dolphin

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

To: akrutzler, #dolphin, #kde_applications, elvisangelaccio, emmanuelp, ngraham, markg
Cc: rkflx, markg, funkybomber, sars, elvisangelaccio, ngraham, #konqueror

[Elvis Angelaccio]

elvisangelaccio requested changes to this revision.
elvisangelaccio added a comment.
This revision now requires changes to proceed.

Post by Henrik Fehlauer

Post by Elvis Angelaccio
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 :)

INLINE COMMENTS

Post by Henrik Fehlauer
kitemlistcontroller.cpp:587
+
+ if (m_selectionManager->selectedItems().count() == 1 &&
+ m_view->isAboveText(m_pressedIndex, m_pressedMousePos)) {

I'd keep this `if()` in a single line

Post by Henrik Fehlauer
kitemlistcontroller.cpp:589
+ m_view->isAboveText(m_pressedIndex, m_pressedMousePos)) {
+ emit selectedItemTextPressed(m_pressedIndex);
+ }

Too much indentation here.

Post by Henrik Fehlauer
dolphinview.cpp:103
+ m_versionControlObserver(0),
+ m_twoClicksRenamingTimer(0),
+ m_twoClicksRenamingItemIndex(-1)

let's use `nullptr` for new code

Post by Henrik Fehlauer
dolphinview.cpp:649
+ }
+ } else if (fromTwoClicksRenamingTimer == false) {
RenameDialog* dialog = new RenameDialog(this, items);

`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?

REPOSITORY
R318 Dolphin

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

To: akrutzler, #dolphin, #kde_applications, elvisangelaccio, emmanuelp, ngraham, markg
Cc: rkflx, markg, funkybomber, sars, elvisangelaccio, ngraham, #konqueror

[Elvis Angelaccio]

elvisangelaccio added inline comments.

INLINE COMMENTS

dolphinview.cpp:819
*/
if (event->type() == QEvent::WindowDeactivate) {

Please move this comment inside the `if()` block, above the `hideToolTip()` call.

REPOSITORY
R318 Dolphin

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

To: akrutzler, #dolphin, #kde_applications, elvisangelaccio, emmanuelp, ngraham, markg
Cc: rkflx, markg, funkybomber, sars, elvisangelaccio, ngraham, #konqueror

[Andreas Krutzler]

akrutzler added a comment.

Post by Henrik Fehlauer

Post by Elvis Angelaccio
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 https://phabricator.kde.org/D7647#inline-34823?

I agree with rkflx. IMHO in case of consistency we should also start inline renaming there.

REPOSITORY
R318 Dolphin

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

To: akrutzler, #dolphin, #kde_applications, elvisangelaccio, emmanuelp, ngraham, markg
Cc: rkflx, markg, funkybomber, sars, elvisangelaccio, ngraham, #konqueror

[Andreas Krutzler]

akrutzler added inline comments.

INLINE COMMENTS

rkflx wrote in 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?

Ah you mean that the (now) 800ms interval should depend on the double click interval set in systemsettings?
Now, even for the initial activation, the double click interval is ignored. That is obviously bad thanks. :)
How do solve that? Adding a certain amount of ms to the double click interval and use this instead of 800ms?

elvisangelaccio wrote in dolphinview.cpp:103
let's use `nullptr` for new code

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.

elvisangelaccio wrote in dolphinview.cpp: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?

I assume with "previous if block" you mean:

if (items.count() == 1 && GeneralSettings::renameInline()) { ... }

That's actually not true, because imagine this case:

- select one item
- click at text (timer starts 800ms)
- during that 800ms: the user select multiple items
- renaming box appears

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.
Maybe you can help me out there? :)

REPOSITORY
R318 Dolphin

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

To: akrutzler, #dolphin, #kde_applications, elvisangelaccio, emmanuelp, ngraham, markg
Cc: rkflx, markg, funkybomber, sars, elvisangelaccio, ngraham, #konqueror

[Elvis Angelaccio]

elvisangelaccio added inline comments.

INLINE COMMENTS

akrutzler wrote in dolphinview.cpp:649
if (items.count() == 1 && GeneralSettings::renameInline()) { ... }
- select one item
- click at text (timer starts 800ms)
- during that 800ms: the user select multiple items
- renaming box appears
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.
Maybe you can help me out there? :)

I see, then there should be a comment explaining this logic.

Btw which signals have you tried? What about `KItemListSelectionManager::selectionChanged`?

REPOSITORY
R318 Dolphin

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

To: akrutzler, #dolphin, #kde_applications, elvisangelaccio, emmanuelp, ngraham, markg
Cc: rkflx, markg, funkybomber, sars, elvisangelaccio, ngraham, #konqueror

[Elvis Angelaccio]

elvisangelaccio added inline comments.

INLINE COMMENTS

akrutzler wrote in 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.

We could also do:

QTimer* m_twoClicksRenamingTimer = nullptr;

in `dolphinview.h` and drop the variable from the initializer list.

REPOSITORY
R318 Dolphin

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

To: akrutzler, #dolphin, #kde_applications, elvisangelaccio, emmanuelp, ngraham, markg
Cc: rkflx, markg, funkybomber, sars, elvisangelaccio, ngraham, #konqueror

[Henrik Fehlauer]

rkflx added inline comments.

INLINE COMMENTS

akrutzler wrote in dolphinview.cpp:198
Ah you mean that the (now) 800ms interval should depend on the double click interval set in systemsettings?
Now, even for the initial activation, the double click interval is ignored. That is obviously bad thanks. :)
How do solve that? Adding a certain amount of ms to the double click interval and use this instead of 800ms?

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:

- Item not activated, first click activates and second click (after DCI has implicitly passed) starts renaming: Set interval to 0. (This should also get rid of the annoying delay after the second click. In my testing, this did not impact the DCI of the initial click.)
- Item already activated: Now we have to wait for at least the DCI. (If less than DCI, "open" was meant.)

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.

REPOSITORY
R318 Dolphin

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

To: akrutzler, #dolphin, #kde_applications, elvisangelaccio, emmanuelp, ngraham, markg
Cc: rkflx, markg, funkybomber, sars, elvisangelaccio, ngraham, #konqueror

[Mark Gaiser]

markg requested changes to this revision.
markg added a comment.


I'm OK with this feature after the code is correct.
So with probably my suggestion and those of @elvisangelaccio

INLINE COMMENTS

elvisangelaccio wrote in dolphinview.cpp:103
QTimer* m_twoClicksRenamingTimer = nullptr;
in `dolphinview.h` and drop the variable from the initializer list.

Most certainly not!
You would break initializing consistency by some being initialized in the header, most in the source.
By default (and by coding guidelines) we do the initializing part in the constructor.

You can change it to:
m_twoClicksRenamingTimer(new QTimet(this))

that would be OK imho.

REPOSITORY
R318 Dolphin

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

To: akrutzler, #dolphin, #kde_applications, elvisangelaccio, emmanuelp, ngraham, markg
Cc: rkflx, markg, funkybomber, sars, elvisangelaccio, ngraham, #konqueror

[Mark Gaiser]

markg added inline comments.

INLINE COMMENTS

markg wrote in dolphinview.cpp:103
Most certainly not!
You would break initializing consistency by some being initialized in the header, most in the source.
By default (and by coding guidelines) we do the initializing part in the constructor.
m_twoClicksRenamingTimer(new QTimet(this))
that would be OK imho.

"Most certainly not!" was for the suggestion of initializing the member in the header, the comment from @elvisangelaccio

REPOSITORY
R318 Dolphin

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

To: akrutzler, #dolphin, #kde_applications, elvisangelaccio, emmanuelp, ngraham, markg
Cc: rkflx, markg, funkybomber, sars, elvisangelaccio, ngraham, #konqueror

[Elvis Angelaccio]

elvisangelaccio added a comment.

m_twoClicksRenamingTimer(new QTimer(this))

+1

REPOSITORY
R318 Dolphin

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

To: akrutzler, #dolphin, #kde_applications, elvisangelaccio, emmanuelp, ngraham, markg
Cc: rkflx, markg, funkybomber, sars, elvisangelaccio, ngraham, #konqueror

[Andreas Krutzler]

akrutzler added inline comments.

INLINE COMMENTS

rkflx wrote in 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).
- Item not activated, first click activates and second click (after DCI has implicitly passed) starts renaming: Set interval to 0. (This should also get rid of the annoying delay after the second click. In my testing, this did not impact the DCI of the initial click.)
- Item already activated: Now we have to wait for at least the DCI. (If less than DCI, "open" was meant.)
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.

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.

markg wrote in 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.

elvisangelaccio wrote in dolphinview.cpp:649
I see, then there should be a comment explaining this logic.
Btw which signals have you tried? What about `KItemListSelectionManager::selectionChanged`?

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).
With that, the DolphinView::renameSelectedItems stays unaffected.

REPOSITORY
R318 Dolphin

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

To: akrutzler, #dolphin, #kde_applications, elvisangelaccio, emmanuelp, ngraham, markg
Cc: rkflx, markg, funkybomber, sars, elvisangelaccio, ngraham, #konqueror

[Andreas Krutzler]

akrutzler updated this revision to Diff 20810.
akrutzler added a comment.


- 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.

REPOSITORY
R318 Dolphin

CHANGES SINCE LAST UPDATE
https://phabricator.kde.org/D7647?vs=20692&id=20810

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

AFFECTED FILES
src/kitemviews/kitemlistcontroller.cpp
src/kitemviews/kitemlistcontroller.h
src/kitemviews/kitemlistview.cpp
src/kitemviews/kitemlistview.h
src/views/dolphinview.cpp
src/views/dolphinview.h

To: akrutzler, #dolphin, #kde_applications, elvisangelaccio, emmanuelp, ngraham, markg
Cc: rkflx, markg, funkybomber, sars, elvisangelaccio, ngraham, #konqueror

[David Faure]

dfaure removed a subscriber: Konqueror.

REPOSITORY
R318 Dolphin

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

To: akrutzler, #dolphin, #kde_applications, elvisangelaccio, emmanuelp, ngraham, markg
Cc: rkflx, markg, funkybomber, sars, elvisangelaccio, ngraham

[Nathaniel Graham]

ngraham added a comment.


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

REPOSITORY
R318 Dolphin

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

To: akrutzler, #dolphin, #kde_applications, elvisangelaccio, emmanuelp, ngraham, markg
Cc: rkflx, markg, funkybomber, sars, elvisangelaccio, ngraham

[Mark Gaiser]

markg added a comment.

Post by Nathaniel Graham
@elvisangelaccio and @markg, how's this looking now?

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

REPOSITORY
R318 Dolphin

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

To: akrutzler, #dolphin, #kde_applications, elvisangelaccio, emmanuelp, ngraham, markg
Cc: rkflx, markg, funkybomber, sars, elvisangelaccio, ngraham

[Andreas Krutzler]

akrutzler marked 4 inline comments as done.

REPOSITORY
R318 Dolphin

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

To: akrutzler, #dolphin, #kde_applications, elvisangelaccio, emmanuelp, ngraham, markg
Cc: rkflx, markg, funkybomber, sars, elvisangelaccio, ngraham

[Andreas Krutzler]

akrutzler added a comment.

Post by Mark Gaiser

Post by Nathaniel Graham
@elvisangelaccio and @markg, how's this looking now?

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

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?

REPOSITORY
R318 Dolphin

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

To: akrutzler, #dolphin, #kde_applications, elvisangelaccio, emmanuelp, ngraham, markg
Cc: rkflx, markg, funkybomber, sars, elvisangelaccio, ngraham

[Elvis Angelaccio]

elvisangelaccio accepted this revision as: elvisangelaccio.
elvisangelaccio added a comment.


LGTM now. Please wait for the other reviewers.

REPOSITORY
R318 Dolphin

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

To: akrutzler, #dolphin, #kde_applications, elvisangelaccio, emmanuelp, ngraham, markg
Cc: rkflx, markg, funkybomber, sars, elvisangelaccio, ngraham

[Mark Gaiser]

markg accepted this revision.
markg added a comment.

Post by Andreas Krutzler

Post by Mark Gaiser

Post by Nathaniel Graham
@elvisangelaccio and @markg, how's this looking now?

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

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?

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).

REPOSITORY
R318 Dolphin

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

To: akrutzler, #dolphin, #kde_applications, elvisangelaccio, emmanuelp, ngraham, markg
Cc: rkflx, markg, funkybomber, sars, elvisangelaccio, ngraham

[Henrik Fehlauer]

rkflx accepted this revision.
rkflx added a comment.


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).

REPOSITORY
R318 Dolphin

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

To: akrutzler, #dolphin, #kde_applications, elvisangelaccio, emmanuelp, ngraham, markg, rkflx
Cc: rkflx, markg, funkybomber, sars, elvisangelaccio, ngraham

[Kåre Särs]

sars added a comment.


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?

INLINE COMMENTS

dolphinview.cpp:1166
+ if ((currentCount == 1 && current.first() != m_twoClicksRenamingItemIndex) || currentCount > 1) {
+ abortTwoClicksRenaming();
+ }

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.

REPOSITORY
R318 Dolphin

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

To: akrutzler, #dolphin, #kde_applications, elvisangelaccio, emmanuelp, ngraham, markg, rkflx
Cc: rkflx, markg, funkybomber, sars, elvisangelaccio, ngraham

[Andreas Krutzler]

akrutzler marked 3 inline comments as done.
akrutzler added a comment.

Post by Elvis Angelaccio

Post by Henrik Fehlauer

Post by Elvis Angelaccio
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 :)

INLINE COMMENTS

Post by Elvis Angelaccio
sars wrote in dolphinview.cpp:1166
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.

Post by Henrik Fehlauer

Post by Elvis Angelaccio
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 :)

REPOSITORY
R318 Dolphin

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

To: akrutzler, #dolphin, #kde_applications, elvisangelaccio, emmanuelp, ngraham, markg, rkflx
Cc: rkflx, markg, funkybomber, sars, elvisangelaccio, ngraham

[Andreas Krutzler]

akrutzler marked an inline comment as done.

REPOSITORY
R318 Dolphin

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

To: akrutzler, #dolphin, #kde_applications, elvisangelaccio, emmanuelp, ngraham, markg, rkflx
Cc: rkflx, markg, funkybomber, sars, elvisangelaccio, ngraham

[Henrik Fehlauer]

rkflx added inline comments.

INLINE COMMENTS

dolphinview.cpp:818
hideToolTip();
+ abortTwoClicksRenaming();
}

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?

REPOSITORY
R318 Dolphin

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

To: akrutzler, #dolphin, #kde_applications, elvisangelaccio, emmanuelp, ngraham, markg, rkflx
Cc: rkflx, markg, funkybomber, sars, elvisangelaccio, ngraham

[Andreas Krutzler]

akrutzler added a comment.

Post by Kåre Särs
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.

Post by Kåre Särs
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.

REPOSITORY
R318 Dolphin

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

To: akrutzler, #dolphin, #kde_applications, elvisangelaccio, emmanuelp, ngraham, markg, rkflx
Cc: rkflx, markg, funkybomber, sars, elvisangelaccio, ngraham

[Andreas Krutzler]

akrutzler added inline comments.

INLINE COMMENTS

rkflx wrote in dolphinview.cpp:818
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?

We want to cancle on focus lost.
We don't want to prevent twoClicksRenaming to start if clicking on an already selected item after dolphin got focus again.

This is called every time if dolphin loses focus.

REPOSITORY
R318 Dolphin

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

To: akrutzler, #dolphin, #kde_applications, elvisangelaccio, emmanuelp, ngraham, markg, rkflx
Cc: rkflx, markg, funkybomber, sars, elvisangelaccio, ngraham

[Henrik Fehlauer]

rkflx added inline comments.

INLINE COMMENTS

akrutzler wrote in dolphinview.cpp:818
We want to cancle on focus lost.
We don't want to prevent twoClicksRenaming to start if clicking on an already selected item after dolphin got focus again.
This is called every time if dolphin loses focus.
We don't want to prevent twoClicksRenaming to start if clicking on an already selected item after dolphin got focus again.

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 :)

REPOSITORY
R318 Dolphin

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

To: akrutzler, #dolphin, #kde_applications, elvisangelaccio, emmanuelp, ngraham, markg, rkflx
Cc: rkflx, markg, funkybomber, sars, elvisangelaccio, ngraham

[Andreas Krutzler]

akrutzler marked an inline comment as done.

REPOSITORY
R318 Dolphin

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

To: akrutzler, #dolphin, #kde_applications, elvisangelaccio, emmanuelp, ngraham, markg, rkflx
Cc: rkflx, markg, funkybomber, sars, elvisangelaccio, ngraham

[Kåre Särs]

sars added a comment.

Post by Andreas Krutzler

Post by Kåre Särs
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.

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 ;)

Post by Andreas Krutzler

Post by Kåre Särs
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.

REPOSITORY
R318 Dolphin

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

To: akrutzler, #dolphin, #kde_applications, elvisangelaccio, emmanuelp, ngraham, markg, rkflx
Cc: rkflx, markg, funkybomber, sars, elvisangelaccio, ngraham

[Andreas Krutzler]

akrutzler added a comment.

Post by Kåre Särs
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 :)

Post by Kåre Särs
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 ***@gmx.net and i can try to understand it then ;)

REPOSITORY
R318 Dolphin

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

To: akrutzler, #dolphin, #kde_applications, elvisangelaccio, emmanuelp, ngraham, markg, rkflx
Cc: rkflx, markg, funkybomber, sars, elvisangelaccio, ngraham

[Kåre Särs]

sars added a comment.

Post by Andreas Krutzler

Post by Kåre Särs
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 :)

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.

REPOSITORY
R318 Dolphin

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

To: akrutzler, #dolphin, #kde_applications, elvisangelaccio, emmanuelp, ngraham, markg, rkflx
Cc: rkflx, markg, funkybomber, sars, elvisangelaccio, ngraham

[Mark Gaiser]

markg resigned from this revision.
markg added a comment.


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..

REPOSITORY
R318 Dolphin

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

To: akrutzler, #dolphin, #kde_applications, elvisangelaccio, emmanuelp, ngraham, markg, rkflx
Cc: rkflx, markg, funkybomber, sars, elvisangelaccio, ngraham

[Henrik Fehlauer]

rkflx added a comment.


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. 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).

REPOSITORY
R318 Dolphin

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

To: akrutzler, #dolphin, #kde_applications, elvisangelaccio, emmanuelp, ngraham, markg, rkflx
Cc: rkflx, markg, funkybomber, sars, elvisangelaccio, ngraham

[Andreas Krutzler]

akrutzler added a comment.

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.

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. :)

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.

Yes, that differs from the Windows implementation, but this makes it IMO more consistent. ;)

REPOSITORY
R318 Dolphin

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

To: akrutzler, #dolphin, #kde_applications, elvisangelaccio, emmanuelp, ngraham, markg, rkflx
Cc: rkflx, markg, funkybomber, sars, elvisangelaccio, ngraham

[Kåre Särs]

sars added a comment.

Post by Mark Gaiser
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..

Here is my solution for the single-click problem. (two different patches on with only the time restraint and the other with everything)

F5450648: D7647-and-add-two-click-rename-timeout.diff <https://phabricator.kde.org/F5450648>
F5450647: add-two-click-edit-timeout.diff <https://phabricator.kde.org/F5450647>

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.

REPOSITORY
R318 Dolphin

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

To: akrutzler, #dolphin, #kde_applications, elvisangelaccio, emmanuelp, ngraham, markg, rkflx
Cc: rkflx, markg, funkybomber, sars, elvisangelaccio, ngraham

[Henrik Fehlauer]

rkflx added a comment.


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).

REPOSITORY
R318 Dolphin

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

To: akrutzler, #dolphin, #kde_applications, elvisangelaccio, emmanuelp, ngraham, markg, rkflx
Cc: rkflx, markg, funkybomber, sars, elvisangelaccio, ngraham

[Henrik Fehlauer]

rkflx added a comment.


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 and not already on mouse down?

REPOSITORY
R318 Dolphin

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

To: akrutzler, #dolphin, #kde_applications, elvisangelaccio, emmanuelp, ngraham, markg, rkflx
Cc: rkflx, markg, funkybomber, sars, elvisangelaccio, ngraham

[Andreas Krutzler]

akrutzler added a comment.

Post by Henrik Fehlauer
- Fast double click to open.
- Click to select.
- 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).

And again, I totally agree with you!

Post by Henrik Fehlauer
- 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?

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.

REPOSITORY
R318 Dolphin

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

To: akrutzler, #dolphin, #kde_applications, elvisangelaccio, emmanuelp, ngraham, markg, rkflx
Cc: rkflx, markg, funkybomber, sars, elvisangelaccio, ngraham

[Andreas Krutzler]

akrutzler updated this revision to Diff 21387.
akrutzler edited the test plan for this revision.
akrutzler added a comment.


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.

CHANGES SINCE LAST UPDATE
https://phabricator.kde.org/D7647?vs=20810&id=21387

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

AFFECTED FILES
src/kitemviews/kitemlistcontroller.cpp
src/kitemviews/kitemlistcontroller.h
src/kitemviews/kitemlistview.cpp
src/kitemviews/kitemlistview.h
src/views/dolphinview.cpp
src/views/dolphinview.h

To: akrutzler, #dolphin, #kde_applications, elvisangelaccio, emmanuelp, ngraham, markg, rkflx
Cc: rkflx, markg, funkybomber, sars, elvisangelaccio, ngraham

[Henrik Fehlauer]

rkflx accepted this revision.
rkflx added a comment.

Post by Andreas Krutzler
Dragging items cancels twoClicksRenaming now.

Thanks, works great.

@elvisangelaccio Does your LGTM still hold?

@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
)

Post by Andreas Krutzler
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.

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

To: akrutzler, #dolphin, #kde_applications, elvisangelaccio, emmanuelp, ngraham, markg, rkflx
Cc: rkflx, markg, funkybomber, sars, elvisangelaccio, ngraham

[Andreas Krutzler]

akrutzler added a comment.

@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?

No, I have no rights. :) I am using `***@gmx.net` for identity and bugzilla so this one should be used here too. Hm strange, I can find myself at identity.kde.org.

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

To: akrutzler, #dolphin, #kde_applications, elvisangelaccio, emmanuelp, ngraham, markg, rkflx
Cc: rkflx, markg, funkybomber, sars, elvisangelaccio, ngraham

[Nathaniel Graham]

ngraham added a comment.


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

Andreas, you should consider applying for a developer account with commit access <https://community.kde.org/Infrastructure/Get_a_Developer_Account>. 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.

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

To: akrutzler, #dolphin, #kde_applications, elvisangelaccio, emmanuelp, ngraham, markg, rkflx
Cc: rkflx, markg, funkybomber, sars, elvisangelaccio, ngraham

[Elvis Angelaccio]

elvisangelaccio accepted this revision.
elvisangelaccio added a comment.
This revision is now accepted and ready to land.


Yes, still looks good to me.
I also made @sars 's comment ("focus lost should abort fast renaming"), but indeed @rkflx is right: consistency wins over possible (but unlikely) accidental renames.

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

To: akrutzler, #dolphin, #kde_applications, elvisangelaccio, emmanuelp, ngraham, markg, rkflx
Cc: rkflx, markg, funkybomber, sars, elvisangelaccio, ngraham

[Andreas Krutzler]

akrutzler added a comment.

Post by Nathaniel Graham
Big +1 from me. Ship it! This is great work.
Andreas, you should consider applying for a developer account with commit access <https://community.kde.org/Infrastructure/Get_a_Developer_Account>. The standards can't be too high, since they let a bozo like me in! ;-)

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?

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

To: akrutzler, #dolphin, #kde_applications, elvisangelaccio, emmanuelp, ngraham, markg, rkflx
Cc: rkflx, markg, funkybomber, sars, elvisangelaccio, ngraham

[Henrik Fehlauer]

rkflx added a comment.


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.

Post by Andreas Krutzler
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


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

To: akrutzler, #dolphin, #kde_applications, elvisangelaccio, emmanuelp, ngraham, markg, rkflx
Cc: rkflx, markg, funkybomber, sars, elvisangelaccio, ngraham

[Nathaniel Graham]

This revision was automatically updated to reflect the committed changes.
Closed by commit R318:5454283008f2: Two clicks on file/folder to rename (authored by akrutzler, committed by ngraham).

REPOSITORY
R318 Dolphin

CHANGES SINCE LAST UPDATE
https://phabricator.kde.org/D7647?vs=21387&id=21430

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

AFFECTED FILES
src/kitemviews/kitemlistcontroller.cpp
src/kitemviews/kitemlistcontroller.h
src/kitemviews/kitemlistview.cpp
src/kitemviews/kitemlistview.h
src/views/dolphinview.cpp
src/views/dolphinview.h

To: akrutzler, #dolphin, #kde_applications, elvisangelaccio, emmanuelp, ngraham, markg, rkflx
Cc: rkflx, markg, funkybomber, sars, elvisangelaccio, ngraham

[Nathaniel Graham]

ngraham added a comment.

Post by Henrik Fehlauer
I can do that tomorrow, unless Nate beats me to it


:-)

REPOSITORY
R318 Dolphin

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

To: akrutzler, #dolphin, #kde_applications, elvisangelaccio, emmanuelp, ngraham, markg, rkflx
Cc: rkflx, markg, funkybomber, sars, elvisangelaccio, ngraham

[Andreas Krutzler]

akrutzler added a comment.

Post by Henrik Fehlauer
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 :)

REPOSITORY
R318 Dolphin

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

To: akrutzler, #dolphin, #kde_applications, elvisangelaccio, emmanuelp, ngraham, markg, rkflx
Cc: rkflx, markg, funkybomber, sars, elvisangelaccio, ngraham

[Nathaniel Graham]

ngraham added a comment.

Post by Andreas Krutzler
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 :)

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

REPOSITORY
R318 Dolphin

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

To: akrutzler, #dolphin, #kde_applications, elvisangelaccio, emmanuelp, ngraham, markg, rkflx
Cc: rkflx, markg, funkybomber, sars, elvisangelaccio, ngraham

[Andreas Krutzler]

akrutzler added a comment.


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?

REPOSITORY
R318 Dolphin

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

To: akrutzler, #dolphin, #kde_applications, elvisangelaccio, emmanuelp, ngraham, markg, rkflx
Cc: rkflx, markg, funkybomber, sars, elvisangelaccio, ngraham

[Henrik Fehlauer]

rkflx added a comment.


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).

REPOSITORY
R318 Dolphin

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

To: akrutzler, #dolphin, #kde_applications, elvisangelaccio, emmanuelp, ngraham, markg, rkflx
Cc: rkflx, markg, funkybomber, sars, elvisangelaccio, ngraham

[Andreas Krutzler]

akrutzler added a dependent revision: D8740: Prevent "Two clicks renaming" if the selected file/folder is not movable.

REPOSITORY
R318 Dolphin

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

To: akrutzler, #dolphin, #kde_applications, elvisangelaccio, emmanuelp, ngraham, markg, rkflx
Cc: rkflx, markg, funkybomber, sars, elvisangelaccio, ngraham

[Wolfgang Bauer]

wbauer added a comment.
Herald added a project: Dolphin.
Herald added a subscriber: kfm-devel.


This apparently caused regressions as I noticed:

1.) Long-clicking on a selected file now triggers inline editing in single-click mode too.

Post by Andreas Krutzler
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 , see https://bugs.kde.org/show_bug.cgi?id=398375

Reverting this change fixes both.

REPOSITORY
R318 Dolphin

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

To: akrutzler, #dolphin, #kde_applications, elvisangelaccio, emmanuelp, ngraham, rkflx
Cc: kfm-devel, wbauer, rkflx, markg, funkybomber, sars, elvisangelaccio, ngraham, feverfew, spoorun, navarromorales, firef, andrebarros, emmanuelp

[Elvis Angelaccio]

elvisangelaccio added a comment.

1.) Long-clicking on a selected file now triggers inline renaming in single-click mode too.

That's definitely not intentional. Is there a bugreport for this?

REPOSITORY
R318 Dolphin

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

To: akrutzler, #dolphin, #kde_applications, elvisangelaccio, emmanuelp, ngraham, rkflx
Cc: kfm-devel, wbauer, rkflx, markg, funkybomber, sars, elvisangelaccio, ngraham, feverfew, spoorun, navarromorales, firef, andrebarros, emmanuelp

[Andreas Krutzler]

akrutzler added a comment.


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

REPOSITORY
R318 Dolphin

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

To: akrutzler, #dolphin, #kde_applications, elvisangelaccio, emmanuelp, ngraham, rkflx
Cc: kfm-devel, wbauer, rkflx, markg, funkybomber, sars, elvisangelaccio, ngraham, feverfew, spoorun, navarromorales, firef, andrebarros, emmanuelp

[Wolfgang Bauer]

wbauer added a comment.

Post by Elvis Angelaccio

1.) Long-clicking on a selected file now triggers inline renaming in single-click mode too.

That's definitely not intentional. Is there a bugreport for this?

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 accidently 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.

REPOSITORY
R318 Dolphin

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

To: akrutzler, #dolphin, #kde_applications, elvisangelaccio, emmanuelp, ngraham, rkflx
Cc: kfm-devel, wbauer, rkflx, markg, funkybomber, sars, elvisangelaccio, ngraham, feverfew, spoorun, navarromorales, firef, andrebarros, emmanuelp

[Elvis Angelaccio]

elvisangelaccio added a comment.

1.) Long-clicking on a selected file now triggers inline renaming in single-click mode too.

Post by Andreas Krutzler
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.

For the record, the second issue was fixed in D15904 <https://phabricator.kde.org/D15904>. D16460 <https://phabricator.kde.org/D16460> fixes the first one instead.

REPOSITORY
R318 Dolphin

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

To: akrutzler, #dolphin, #kde_applications, elvisangelaccio, emmanuelp, ngraham, rkflx
Cc: kfm-devel, wbauer, rkflx, markg, funkybomber, sars, elvisangelaccio, ngraham, alexde, sourabhboss, feverfew, spoorun, navarromorales, firef, andrebarros, emmanuelp