Details

Reviewers

broulik
ngraham
dfaure
elvisangelaccio

Group Reviewers

Frameworks
Dolphin

Commits

R241:3f5341d3d7e2: Upstream Dolphin's file rename dialog

Summary

KIO didn't actually have its own rename dialog, which is necessary to implement the requested rename-from-the-file-dialog feature (189482). This patch upstreams Dolphin's dialog so all KIO::FileWidgets users can use it.

The file's contents are copied from Dolphin and updated, but the name is changed from RenameDialog to RenameFileDialog because RenameDialog already exists in KIO (and is actually an overwrite dialog, but we can't rename it until KF6).

CCBUG: 189482

Test Plan

Apply D17596 or D17597 and initiate a rename operation for one or more files. See that it works as expected.

Tests still pass.

Diff Detail

Repository

R241 KIO

Lint

Automatic diff as part of commit; lint not applicable.

Unit

Automatic diff as part of commit; unit tests not applicable.

There are a very large number of changes, so older changes are hidden. Show Older Changes

Add context about filename #

Harbormaster completed remote builds in B14114: Diff 62026.Jul 19 2019, 7:14 AM

sitter added a subscriber: sitter.Jul 19 2019, 9:13 AM

sitter added inline comments.

src/widgets/renamefiledialog.h
73	You'll also want to override `event` as per https://community.kde.org/Policies/Binary_Compatibility_Issues_With_C%2B%2B
76	This is now forward declared twice.
77	You can in fact make the class forward declaration and the member declaration one line `class RenameFileDialogPrivate *const d;`

meven marked 4 inline comments as done.Jul 19 2019, 10:37 AM

meven added inline comments.

src/widgets/renamefiledialog.h
73	Thanks This is really arcane, I am really not found of C++.
77	I tried it but it did not work.

meven marked 4 inline comments as done.Jul 19 2019, 10:37 AM

dfaure added a subscriber: dfaure.Jul 20 2019, 10:07 AM

dfaure added inline comments.

src/widgets/renamefiledialog.cpp
60	group the bools together for less padding
61	Just run `uncrustify-kf5` from the repo kde-dev-scripts.

Group boolean fields together

Harbormaster completed remote builds in B15135: Diff 63796.Aug 15 2019, 12:31 PM

meven marked an inline comment as done.Aug 15 2019, 12:32 PM

meven added inline comments.

src/widgets/renamefiledialog.cpp
61	I am very happy to learn about this one. This script cannot work with a single file it seems and most of the files in this directory are modified by the script. So I won't use it here.

dfaure requested changes to this revision.Aug 16 2019, 7:04 AM

dfaure added inline comments.

src/widgets/renamefiledialog.cpp
61	You can always revert the changes to the other files ;) (commit any other changes, run uncrustify, git commit --amend renamefiledialog.*, git checkout .) Or do you mean it would really make the coding style inconsistent with the other files? I would be surprised that the difference would be major.
68	This returns an invalid size (because nobody called setMinimumSize at this point yet, and this is not to be confused with minimumSizeHint), i.e. this code uses a convoluted way to pass -1 as minimum height. Just use setMinimumWidth(320) instead.
120	items.first().path() would be simpler and faster. (that doesn't give the same result, but it's sufficient for calling suffixForFileName with it)
122	why toLower()? Should work without.
144	Same here, suffixForFileName(item.url().path()) --- and this should be done the same way in both places, e.g. no intermediate variable in either location.
209	Does Dolphin really want that? I thought a GUI design principle in Dolphin was to avoid messageboxes and show errors in an embedded widget instead. So I was expecting this dialog to emit an error signal so that the app can decide on how to handle errors, instead.
223	s/Assure/Ensure/ ?
247	Isn't it enough to focus the lineEdit in the constructor? Doing it here in showEvent (without a test for event->spontaneous()) means that if you focus another widget, switch virtual desktops, and switch back, the focus will unexpectedly go to the lineedit again.
src/widgets/renamefiledialog.h
48	@since 5.62
60	Generally the parent widget argument goes last.
77	Yeah, it's more complicated inside namespaces.

This revision now requires changes to proceed.Aug 16 2019, 7:04 AM

uncrustify file, typo fixes, add error slot

Harbormaster completed remote builds in B15173: Diff 63854.Aug 16 2019, 10:02 AM

Btw the licensing issue is not resolved yet.
I will update the license once this has cleared.

src/widgets/renamefiledialog.cpp
61	I just did not bother going through those hoops, but this worked out great. Thanks
120	KFileItem has no path() method, but here items.first().url().fileName() should be equivalent.
122	I guess the database doesn't work for JPEG file extension for instance.
209	Apparently here it did use the default error dialog. I have changed this to expose an error signal to let the class user handle it. I wonder if exposing a setAutoErrorHandling setter for this dialog would be a good idea.

meven marked 4 inline comments as done.Aug 16 2019, 10:31 AM

dfaure requested changes to this revision.Aug 16 2019, 11:13 AM

dfaure added inline comments.

src/widgets/renamefiledialog.cpp
120	Oops yes I meant items.first().url().path(). OK for fileName(), path() is faster but this code path isn't speed critical.
122	Wrong, QMimeDatabase handles this correctly, extensions are case insensitive. Proof: $ kmimetypefinder5 -f foo.JPEG image/jpeg
209	Good question, I don't know. I'd say, let the first person who needs it, add it :)
216	This can't possibly work, KJob::error is a getter, not a signal. Remove slotError, and emit error() in slotResult.
247	I see you used the more complicated solution of keeping showEvent and testing for spontaneous. Any reason? It didn't work from the constructor?

This revision now requires changes to proceed.Aug 16 2019, 11:13 AM

bruns added inline comments.Aug 17 2019, 3:22 AM

src/widgets/renamefiledialog.cpp
120	anything wrong with `KFileItem::name()`?

Emit error correctly use name() function of KFileItem

Harbormaster completed remote builds in B15201: Diff 63891.Aug 17 2019, 6:51 AM

meven marked 6 inline comments as done.Aug 17 2019, 6:52 AM

Fix dialog where the private class was missing the renamed files items

Harbormaster completed remote builds in B15202: Diff 63892.Aug 17 2019, 8:11 AM

Call setFocus() in dialog constructor

Harbormaster completed remote builds in B15205: Diff 63895.Aug 17 2019, 9:11 AM

meven marked an inline comment as done.Aug 17 2019, 9:13 AM

meven added inline comments.

src/widgets/renamefiledialog.cpp
247	After some testing using !spontaneous() here is pointless.

meven marked 2 inline comments as done.Aug 17 2019, 9:13 AM

I received the agreements of all contributors including the original code author, to relicense this code and hence upstream it.
It reminds of the importance of signing FLA to KDE e.V.

So this is good to follow through the normal review process.
@dfaure

I would like to add a KF6 TODO : rename the class RenameFileDialog to RenameDialog and the class RenameFileDialog to RenameFileOverwrittenDialog or similar.

In D17595#542142, @meven wrote:

I received the agreements of all contributors including the original code author, to relicense this code and hence upstream it.
It reminds of the importance of signing FLA to KDE e.V.

So this is good to follow through the normal review process.
@dfaure

I appreciate your doggedness here. It'll be very nice to get this in.

I would like to add a KF6 TODO : rename the class RenameFileDialog to RenameDialog and the class RenameFileDialog to RenameFileOverwrittenDialog or similar.

Update copyright/licensing, update @since, add a TODO KF6

Harbormaster completed remote builds in B19103: Diff 70224.Nov 23 2019, 4:35 PM

In D17595#542142, @meven wrote:

I received the agreements of all contributors including the original code author, to relicense this code and hence upstream it.

Playing the bad guy here: where are these agreements? On a public mailing list, or in your private inbox? If on a public mailing list, you could reference these mails on our mail archives in your commit message.

In D17595#566800, @dhaumann wrote:

In D17595#542142, @meven wrote:

I received the agreements of all contributors including the original code author, to relicense this code and hence upstream it.

Playing the bad guy here: where are these agreements? On a public mailing list, or in your private inbox? If on a public mailing list, you could reference these mails on our mail archives in your commit message.

On kde-ev-board cc'd private mailing list.
I'd be happy to add some paper trail in the files as @dhaumann suggested, but I don't what form it might take. @lydia any suggestion ?

And by the way If you haven't done already Sign the FLA https://ev.kde.org/resources/FLA.pdf

For posterity's sake:

I confirm that we have confirmation to the board list from (hope I didn't overlook anyone):

Emmanuel Pescosta
Roman Inflianskas
Alexander Richardson
David Faure
Matthias Fuchs
Artur Puzio
Kevin Funk
Aldo Mateli
Kevin Ottens
Chinmoy Ranjan
Elvis Angelaccio
Laurent Montel
Jeff Mitchell

And that I have confirmation from Peter Penz' wife via Facebook that he is fine with it.

In D17595#569924, @lydia wrote:

For posterity's sake:

I confirm that we have confirmation to the board list from (hope I didn't overlook anyone):

Emmanuel Pescosta

Roman Inflianskas

Alexander Richardson

David Faure

Matthias Fuchs

Artur Puzio

Kevin Funk

Aldo Mateli

Kevin Ottens

Chinmoy Ranjan

Elvis Angelaccio

Laurent Montel

Jeff Mitchell

And that I have confirmation from Peter Penz' wife via Facebook that he is fine with it.

Thank you Lydia, so we can get to code review.

Awesome. Does this need a rebase?

In D17595#580655, @ngraham wrote:

Awesome. Does this need a rebase?

It should not, the API has not changed and this is new code.

The @since values need to be 5.66 now. Works great though, and the code looks sane to me now.

dfaure accepted this revision.Jan 3 2020, 9:37 AM

dfaure added inline comments.

src/widgets/renamefiledialog.h
48	I hope this comment (between docu and class) doesn't break apidox generation.

This revision is now accepted and ready to land.Jan 3 2020, 9:37 AM

elvisangelaccio added a subscriber: elvisangelaccio.Jan 6 2020, 12:40 PM

elvisangelaccio added inline comments.

src/widgets/renamefiledialog.h
49	So, what should we rename `RenameFileDialog` to? `RenameFileDialog` or `RenameFileOverwrittenDialog` ? ;) Anyway, why wait for KF6 since this is new API? Can't we rename it now?

elvisangelaccio added inline comments.Jan 6 2020, 12:44 PM

src/widgets/renamefiledialog.h
49	Ah sorry, I've read now the commit message. Which means there is a typo in this TODO: "... and the class RenameDialog to RenameFileOverwrittenDialog or similar."

meven edited the summary of this revision. (Show Details)Jan 6 2020, 1:00 PM

Rebase, amend, update @since, fix typo in TODO KF6

Harbormaster completed remote builds in B20702: Diff 72881.Jan 6 2020, 1:04 PM

Update Copyright year

Harbormaster completed remote builds in B20703: Diff 72882.Jan 6 2020, 1:07 PM

elvisangelaccio accepted this revision.Jan 6 2020, 1:08 PM

This will be so nice to finally have done!

last days for last minute review
@dfaure

Last day before what? The world explodes soon? :)

KF 5.66 is already tagged, as planned (first saturday of the month).

Closed by commit R241:3f5341d3d7e2: Upstream Dolphin's file rename dialog (authored by meven). · Explain WhyJan 6 2020, 7:40 PM

This revision was automatically updated to reflect the committed changes.

		Path
M		src/widgets/CMakeLists.txt (2 lines)
A	M	src/widgets/renamefiledialog.h (79 lines)
A	M	src/widgets/renamefiledialog.cpp (256 lines)

Diff	ID	Base	Description	Created	Lint	Unit
Base			Base
Diff 1	47606	9f1b7e8		Dec 15 2018, 2:46 AM	★	★
Diff 2	59255	9f1b7e8	Add a Private class to RenameFileDialog, code fixes	Jun 6 2019, 11:28 AM	★	★
Diff 3	59292	9f1b7e8	Improve localization context	Jun 6 2019, 7:49 PM	★	★
Diff 4	62005	928352e	Fix email in copyright declaration	Jul 18 2019, 7:52 PM	★	★
Diff 5	62026	928352e	Add context about filename #	Jul 19 2019, 7:14 AM	★	★
Diff 6	63796	7d888cb	Group boolean fields together	Aug 15 2019, 12:30 PM	★	★
Diff 7	63854	7d888cb	uncrustify file, typo fixes, add error slot	Aug 16 2019, 10:02 AM	★	★
Diff 8	63891	7d888cb	Emit error correctly use name() function of KFileItem	Aug 17 2019, 6:51 AM	★	★
Diff 9	63892	7d888cb	Fix dialog where the private class was missing the renamed files items	Aug 17 2019, 8:11 AM	★	★
Diff 10	63895	7d888cb	Call setFocus() in dialog constructor	Aug 17 2019, 9:11 AM	★	★
Diff 11	70224	53d562e	Update copyright/licensing, update @since, add a TODO KF6	Nov 23 2019, 4:35 PM	★	★
Diff 12	72881	9225855	Rebase, amend, update @since, fix typo in TODO KF6	Jan 6 2020, 1:04 PM	★	★
Diff 13	72882	9225855	Update Copyright year	Jan 6 2020, 1:07 PM	★	★
Diff 14	72922	bdc2df9	R241:3f5341d3d7e23e3d1f160cd6f4d8188411d75f80	Jan 6 2020, 7:40 PM	★	★

Status	Author	Revision
Closed	ngraham	D17596 [KDirOperator] Allow renaming files from the context menu
Abandoned	ngraham	D17597 Use newly-upstreamed rename dialog from KIO
Closed	meven	D17595 Upstream Dolphin's file rename dialog

Upstream Dolphin's file rename dialog
ClosedPublic
Actions

Details

Diff Detail

Revision Contents
Changeset List

Diff 72922

src/widgets/CMakeLists.txt

src/widgets/renamefiledialog.h

src/widgets/renamefiledialog.cpp

Upstream Dolphin's file rename dialogClosedPublicActions

Details

Diff Detail

Revision ContentsChangeset List

Diff 72922

src/widgets/CMakeLists.txt

src/widgets/renamefiledialog.h

src/widgets/renamefiledialog.cpp

Upstream Dolphin's file rename dialog
ClosedPublic
Actions

Revision Contents
Changeset List