When renaming a file, it's new name causes it to scroll out of view Dolphin will not scroll to the location of the new file. This patch aims to address that. This affects all view modes.
BUG: 354330
elvisangelaccio | |
ngraham | |
emmanuelp |
Dolphin |
When renaming a file, it's new name causes it to scroll out of view Dolphin will not scroll to the location of the new file. This patch aims to address that. This affects all view modes.
BUG: 354330
Lint Skipped |
Unit Tests Skipped |
src/views/dolphinview.cpp | ||
---|---|---|
1766 | This is called only when the user clicks the Rename button and items were actually renamed. |
Please remove BUG: .... from the summary line, and replace the line
Relevant bug report: https://bugs.kde.org/show_bug.cgi?id=354330
with
BUG: 354330
src/views/dolphinview.cpp | ||
---|---|---|
1762 | The old code did not have this assignment. Is it really necessary? | |
src/views/dolphinview.h | ||
315 | I'd just call it slotRenamingFinished(). Also, no need to have it public, make it a private slot. | |
src/views/renamedialog.cpp | ||
42 | Not really necessary, default QList ctor is automatically called. | |
246 | Unrelated whitespace change. |
src/views/dolphinview.h | ||
---|---|---|
771 | Maybe forceUrlsSelection() would be more clear |
src/views/dolphinview.cpp | ||
---|---|---|
1762 | As far as I remember until I added that statement when mass renaming the new selection after the rename would include some (seemingly random) items in the directory but I cannot reproduce it reliably. |
-1
FWIW, I don't approve of this patch from a usability perspective. It's not at all clear that changing the view is what the user expects in this case. Especially if you're renaming multiple files in such a manner that they will be re-sorted elsewhere, the view will change after each rename operation and you will need to manually scroll back over and over again, making the multiple-file-rename operation an infuriating experience. macOS finder behaved this way for many years and Apple got a lot of complaints about it, eventually reverting the behavior to match what Dolphin does today.
You could argue the same with pasting. If you paste or drop a file, the Dolphin view will scroll to it.
This patch just fixes a regression/inconsistency, as the feature was already there.
Renaming a single file is probably a more common use case, so I think is safe if Dolphin privileges it over multiple successive renames. The latter use case could be fixed in other ways (for example, using the new stash:/ ioslave).
src/views/dolphinview.cpp | ||
---|---|---|
1754 | No, it will be called only when there are successful rename operations finished. |
Looks good to me now.
@emmanuelp can you have another look? :)
src/views/renamedialog.cpp | ||
---|---|---|
254 | Unrelated whitespace change. |
@elvisangelaccio @emmanuelp since @emateli doesn't have commit access should I commit?
To which branch?
I think it should go to the stable branch. I'll take care of it, I need to push another patch anyway :)
@emateli You may be interested in fixing https://bugs.kde.org/show_bug.cgi?id=354330#c8 ?
@elvisangelaccio: So I was thinking of using KIO::FileUndoManager::undoJobFinished but it unfortunately provides no details as to what job was undone.
A possible option would be to keep the last job recorded so we have the name that the undo-ed item will have after reverting, but this is just hacky and doesn't support more than 1 action. I think best would be if job information was passed from KIO::FileUndoManager and then Dolphin could leverage it. I also tried to use selectionManager but to no avail.
Yeah, looks like we need a new signal in KIO::FileUndoManager that tells us which job was undone...