D17595: Upstream Dolphin's file rename dialog

[Méven Car]

This revision was automatically updated to reflect the committed changes.
Closed by commit R241:3f5341d3d7e2: Upstream Dolphins file rename dialog 
(authored by meven).

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D17595?vs=72882=72922

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

AFFECTED FILES
  src/widgets/CMakeLists.txt
  src/widgets/renamefiledialog.cpp
  src/widgets/renamefiledialog.h

To: meven, #frameworks, #dolphin, broulik, ngraham, dfaure, elvisangelaccio
Cc: elvisangelaccio, lydia, dfaure, sitter, mitchell, emmanuelp, ltoscano, 
bruns, meven, dhaumann, pino, kde-frameworks-devel, LeGast00n, GB_2, michaelh, 
ngraham

D17595: Upstream Dolphin's file rename dialog

[David Faure]

dfaure accepted this revision.
dfaure added a comment.


  Last day before what? The world explodes soon? :)
  
  KF 5.66 is already tagged, as planned (first saturday of the month).

REPOSITORY
  R241 KIO

BRANCH
  arcpatch-D17595_2

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

To: meven, #frameworks, #dolphin, broulik, ngraham, dfaure, elvisangelaccio
Cc: elvisangelaccio, lydia, dfaure, sitter, mitchell, emmanuelp, ltoscano, 
bruns, meven, dhaumann, pino, kde-frameworks-devel, LeGast00n, GB_2, michaelh, 
ngraham

D17595: Upstream Dolphin's file rename dialog

[Méven Car]

meven added a comment.


  last days for last minute review
  @dfaure

REPOSITORY
  R241 KIO

BRANCH
  arcpatch-D17595_2

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

To: meven, #frameworks, #dolphin, broulik, ngraham, dfaure, elvisangelaccio
Cc: elvisangelaccio, lydia, dfaure, sitter, mitchell, emmanuelp, ltoscano, 
bruns, meven, dhaumann, pino, kde-frameworks-devel, LeGast00n, GB_2, michaelh, 
ngraham

D17595: Upstream Dolphin's file rename dialog

[Nathaniel Graham]

ngraham added a comment.


  This will be so nice to finally have done!

REPOSITORY
  R241 KIO

BRANCH
  arcpatch-D17595_2

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

To: meven, #frameworks, #dolphin, broulik, ngraham, dfaure, elvisangelaccio
Cc: elvisangelaccio, lydia, dfaure, sitter, mitchell, emmanuelp, ltoscano, 
bruns, meven, dhaumann, pino, kde-frameworks-devel, LeGast00n, GB_2, michaelh, 
ngraham

D17595: Upstream Dolphin's file rename dialog

[Elvis Angelaccio]

elvisangelaccio accepted this revision.

REPOSITORY
  R241 KIO

BRANCH
  arcpatch-D17595_2

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

To: meven, #frameworks, #dolphin, broulik, ngraham, dfaure, elvisangelaccio
Cc: elvisangelaccio, lydia, dfaure, sitter, mitchell, emmanuelp, ltoscano, 
bruns, meven, dhaumann, pino, kde-frameworks-devel, LeGast00n, GB_2, michaelh, 
ngraham

D17595: Upstream Dolphin's file rename dialog

[Méven Car]

meven updated this revision to Diff 72882.
meven added a comment.


  Update Copyright year

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D17595?vs=72881=72882

BRANCH
  arcpatch-D17595_2

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

AFFECTED FILES
  src/widgets/CMakeLists.txt
  src/widgets/renamefiledialog.cpp
  src/widgets/renamefiledialog.h

To: meven, #frameworks, #dolphin, broulik, ngraham, dfaure
Cc: elvisangelaccio, lydia, dfaure, sitter, mitchell, emmanuelp, ltoscano, 
bruns, meven, dhaumann, pino, kde-frameworks-devel, LeGast00n, GB_2, michaelh, 
ngraham

D17595: Upstream Dolphin's file rename dialog

[Méven Car]

meven updated this revision to Diff 72881.
meven marked 2 inline comments as done.
meven added a comment.


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

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D17595?vs=70224=72881

BRANCH
  arcpatch-D17595_2

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

AFFECTED FILES
  src/widgets/CMakeLists.txt
  src/widgets/renamefiledialog.cpp
  src/widgets/renamefiledialog.h

To: meven, #frameworks, #dolphin, broulik, ngraham, dfaure
Cc: elvisangelaccio, lydia, dfaure, sitter, mitchell, emmanuelp, ltoscano, 
bruns, meven, dhaumann, pino, kde-frameworks-devel, LeGast00n, GB_2, michaelh, 
ngraham

D17595: Upstream Dolphin's file rename dialog

[Méven Car]

meven edited the summary of this revision.

REPOSITORY
  R241 KIO

BRANCH
  arcpatch-D17595_1

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

To: meven, #frameworks, #dolphin, broulik, ngraham, dfaure
Cc: elvisangelaccio, lydia, dfaure, sitter, mitchell, emmanuelp, ltoscano, 
bruns, meven, dhaumann, pino, kde-frameworks-devel, LeGast00n, GB_2, michaelh, 
ngraham

D17595: Upstream Dolphin's file rename dialog

[Elvis Angelaccio]

elvisangelaccio added inline comments.

INLINE COMMENTS

> elvisangelaccio wrote in renamefiledialog.h:48
> 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?

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

REPOSITORY
  R241 KIO

BRANCH
  arcpatch-D17595_1

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

To: meven, #frameworks, #dolphin, broulik, ngraham, dfaure
Cc: elvisangelaccio, lydia, dfaure, sitter, mitchell, emmanuelp, ltoscano, 
bruns, meven, dhaumann, pino, kde-frameworks-devel, LeGast00n, GB_2, michaelh, 
ngraham

D17595: Upstream Dolphin's file rename dialog

[Elvis Angelaccio]

elvisangelaccio added inline comments.

INLINE COMMENTS

> renamefiledialog.h:48
> + */
> +// TODO KF6  : rename the class RenameFileDialog to RenameDialog and the 
> class RenameFileDialog to RenameFileOverwrittenDialog or similar.
> +class KIOWIDGETS_EXPORT RenameFileDialog : public QDialog

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?

REPOSITORY
  R241 KIO

BRANCH
  arcpatch-D17595_1

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

To: meven, #frameworks, #dolphin, broulik, ngraham, dfaure
Cc: elvisangelaccio, lydia, dfaure, sitter, mitchell, emmanuelp, ltoscano, 
bruns, meven, dhaumann, pino, kde-frameworks-devel, LeGast00n, GB_2, michaelh, 
ngraham

D17595: Upstream Dolphin's file rename dialog

[David Faure]

dfaure accepted this revision.
dfaure added inline comments.
This revision is now accepted and ready to land.

INLINE COMMENTS

> dfaure wrote in renamefiledialog.h:47
> @since 5.62

I hope this comment (between docu and class) doesn't break apidox generation.

REPOSITORY
  R241 KIO

BRANCH
  arcpatch-D17595_1

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

To: meven, #frameworks, #dolphin, broulik, ngraham, dfaure
Cc: lydia, dfaure, sitter, mitchell, emmanuelp, ltoscano, bruns, meven, 
dhaumann, pino, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham

D17595: Upstream Dolphin's file rename dialog

[Nathaniel Graham]

ngraham accepted this revision.
ngraham added a comment.


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

REPOSITORY
  R241 KIO

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

To: meven, #frameworks, #dolphin, broulik, ngraham, dfaure
Cc: lydia, dfaure, sitter, mitchell, emmanuelp, ltoscano, bruns, meven, 
dhaumann, pino, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham

D17595: Upstream Dolphin's file rename dialog

[Méven Car]

meven added a comment.


  In D17595#580655 , @ngraham wrote:
  
  > Awesome. Does this need a rebase?
  
  
  It should not, the API has not changed and this is new code.

REPOSITORY
  R241 KIO

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

To: meven, #frameworks, #dolphin, broulik, ngraham, dfaure
Cc: lydia, dfaure, sitter, mitchell, emmanuelp, ltoscano, bruns, meven, 
dhaumann, pino, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham

D17595: Upstream Dolphin's file rename dialog

[Nathaniel Graham]

ngraham added a comment.


  Awesome. Does this need a rebase?

REPOSITORY
  R241 KIO

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

To: meven, #frameworks, #dolphin, broulik, ngraham, dfaure
Cc: lydia, dfaure, sitter, mitchell, emmanuelp, ltoscano, bruns, meven, 
dhaumann, pino, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham

D17595: Upstream Dolphin's file rename dialog

[Méven Car]

meven added a comment.


  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.

REPOSITORY
  R241 KIO

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

To: meven, #frameworks, #dolphin, broulik, ngraham, dfaure
Cc: lydia, dfaure, sitter, mitchell, emmanuelp, ltoscano, bruns, meven, 
dhaumann, pino, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham

D17595: Upstream Dolphin's file rename dialog

[Lydia Pintscher]

lydia added a comment.


  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.

REPOSITORY
  R241 KIO

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

To: meven, #frameworks, #dolphin, broulik, ngraham, dfaure
Cc: lydia, dfaure, sitter, mitchell, emmanuelp, ltoscano, bruns, meven, 
dhaumann, pino, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham

D17595: Upstream Dolphin's file rename dialog

[Méven Car]

meven added a subscriber: lydia.
meven added a comment.


  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

REPOSITORY
  R241 KIO

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

To: meven, #frameworks, #dolphin, broulik, ngraham, dfaure
Cc: lydia, dfaure, sitter, mitchell, emmanuelp, ltoscano, bruns, meven, 
dhaumann, pino, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham

D17595: Upstream Dolphin's file rename dialog

[Dominik Haumann]

dhaumann added a comment.


  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.

REPOSITORY
  R241 KIO

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

To: meven, #frameworks, #dolphin, broulik, ngraham, dfaure
Cc: dfaure, sitter, mitchell, emmanuelp, ltoscano, bruns, meven, dhaumann, 
pino, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham

D17595: Upstream Dolphin's file rename dialog

[Méven Car]

meven updated this revision to Diff 70224.
meven added a comment.


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

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D17595?vs=63895=70224

BRANCH
  arcpatch-D17595_1

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

AFFECTED FILES
  src/widgets/CMakeLists.txt
  src/widgets/renamefiledialog.cpp
  src/widgets/renamefiledialog.h

To: meven, #frameworks, #dolphin, broulik, ngraham, dfaure
Cc: dfaure, sitter, mitchell, emmanuelp, ltoscano, bruns, meven, dhaumann, 
pino, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham

D17595: Upstream Dolphin's file rename dialog

[Nathaniel Graham]

ngraham added a comment.


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

REPOSITORY
  R241 KIO

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

To: meven, #frameworks, #dolphin, broulik, ngraham, dfaure
Cc: dfaure, sitter, mitchell, emmanuelp, ltoscano, bruns, meven, dhaumann, 
pino, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham

D17595: Upstream Dolphin's file rename dialog

[Méven Car]

meven added a comment.


  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.

REPOSITORY
  R241 KIO

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

To: meven, #frameworks, #dolphin, broulik, ngraham, dfaure
Cc: dfaure, sitter, mitchell, emmanuelp, ltoscano, bruns, meven, dhaumann, 
pino, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham

D17595: Upstream Dolphin's file rename dialog

[Méven Car]

meven marked 2 inline comments as done.

REPOSITORY
  R241 KIO

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

To: meven, #frameworks, #dolphin, broulik, ngraham, dfaure
Cc: dfaure, sitter, mitchell, emmanuelp, ltoscano, bruns, meven, dhaumann, 
pino, kde-frameworks-devel, LeGast00n, michaelh, ngraham

D17595: Upstream Dolphin's file rename dialog

[Méven Car]

meven marked an inline comment as done.
meven added inline comments.

INLINE COMMENTS

> dfaure wrote in renamefiledialog.cpp:246
> I see you used the more complicated solution of keeping showEvent and testing 
> for spontaneous. Any reason? It didn't work from the constructor?

After some testing using !spontaneous() here is pointless.

REPOSITORY
  R241 KIO

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

To: meven, #frameworks, #dolphin, broulik, ngraham, dfaure
Cc: dfaure, sitter, mitchell, emmanuelp, ltoscano, bruns, meven, dhaumann, 
pino, kde-frameworks-devel, LeGast00n, michaelh, ngraham

D17595: Upstream Dolphin's file rename dialog

[Méven Car]

meven updated this revision to Diff 63895.
meven added a comment.


  Call setFocus() in dialog constructor

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D17595?vs=63892=63895

BRANCH
  arcpatch-D17595

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

AFFECTED FILES
  src/widgets/CMakeLists.txt
  src/widgets/renamefiledialog.cpp
  src/widgets/renamefiledialog.h

To: meven, #frameworks, #dolphin, broulik, ngraham, dfaure
Cc: dfaure, sitter, mitchell, emmanuelp, ltoscano, bruns, meven, dhaumann, 
pino, kde-frameworks-devel, LeGast00n, michaelh, ngraham

D17595: Upstream Dolphin's file rename dialog

[Méven Car]

meven updated this revision to Diff 63892.
meven added a comment.


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

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D17595?vs=63891=63892

BRANCH
  arcpatch-D17595

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

AFFECTED FILES
  src/widgets/CMakeLists.txt
  src/widgets/renamefiledialog.cpp
  src/widgets/renamefiledialog.h

To: meven, #frameworks, #dolphin, broulik, ngraham, dfaure
Cc: dfaure, sitter, mitchell, emmanuelp, ltoscano, bruns, meven, dhaumann, 
pino, kde-frameworks-devel, LeGast00n, michaelh, ngraham

D17595: Upstream Dolphin's file rename dialog

[Méven Car]

meven marked 6 inline comments as done.

REPOSITORY
  R241 KIO

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

To: meven, #frameworks, #dolphin, broulik, ngraham, dfaure
Cc: dfaure, sitter, mitchell, emmanuelp, ltoscano, bruns, meven, dhaumann, 
pino, kde-frameworks-devel, LeGast00n, michaelh, ngraham

D17595: Upstream Dolphin's file rename dialog

[Méven Car]

meven updated this revision to Diff 63891.
meven added a comment.


  Emit error correctly use name() function of KFileItem

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D17595?vs=63854=63891

BRANCH
  arcpatch-D17595

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

AFFECTED FILES
  src/widgets/CMakeLists.txt
  src/widgets/renamefiledialog.cpp
  src/widgets/renamefiledialog.h

To: meven, #frameworks, #dolphin, broulik, ngraham, dfaure
Cc: dfaure, sitter, mitchell, emmanuelp, ltoscano, bruns, meven, dhaumann, 
pino, kde-frameworks-devel, LeGast00n, michaelh, ngraham

D17595: Upstream Dolphin's file rename dialog

[Stefan Brüns]

bruns added inline comments.

INLINE COMMENTS

> dfaure wrote in renamefiledialog.cpp:119
> Oops yes I meant items.first().url().path(). OK for fileName(), path() is 
> faster but this code path isn't speed critical.

anything wrong with `KFileItem::name()`?

REPOSITORY
  R241 KIO

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

To: meven, #frameworks, #dolphin, broulik, ngraham, dfaure
Cc: dfaure, sitter, mitchell, emmanuelp, ltoscano, bruns, meven, dhaumann, 
pino, kde-frameworks-devel, LeGast00n, michaelh, ngraham

D17595: Upstream Dolphin's file rename dialog

[David Faure]

dfaure requested changes to this revision.
dfaure added inline comments.
This revision now requires changes to proceed.

INLINE COMMENTS

> renamefiledialog.cpp:215
> +connect(job, ::result, this, ::deleteLater);
> +connect(job, ::error, this, ::slotError);
> +

This can't possibly work, KJob::error is a getter, not a signal.

Remove slotError, and emit error() in slotResult.

> meven wrote in renamefiledialog.cpp:119
> KFileItem has no path()  method, but here items.first().url().fileName() 
> should be equivalent.

Oops yes I meant items.first().url().path(). OK for fileName(), path() is 
faster but this code path isn't speed critical.

> meven wrote in renamefiledialog.cpp:121
> I guess the database doesn't work for JPEG file extension for instance.

Wrong, QMimeDatabase handles this correctly, extensions are case insensitive.

Proof:
$ kmimetypefinder5 -f foo.JPEG 
image/jpeg

> meven wrote in renamefiledialog.cpp:208
> 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.

Good question, I don't know. I'd say, let the first person who needs it, add it 
:)

> dfaure wrote in renamefiledialog.cpp:246
> 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.

I see you used the more complicated solution of keeping showEvent and testing 
for spontaneous. Any reason? It didn't work from the constructor?

REPOSITORY
  R241 KIO

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

To: meven, #frameworks, #dolphin, broulik, ngraham, dfaure
Cc: dfaure, sitter, mitchell, emmanuelp, ltoscano, bruns, meven, dhaumann, 
pino, kde-frameworks-devel, LeGast00n, michaelh, ngraham

D17595: Upstream Dolphin's file rename dialog

[Méven Car]

meven marked 4 inline comments as done.

REPOSITORY
  R241 KIO

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

To: meven, #frameworks, #dolphin, broulik, ngraham, dfaure
Cc: dfaure, sitter, mitchell, emmanuelp, ltoscano, bruns, meven, dhaumann, 
pino, kde-frameworks-devel, LeGast00n, michaelh, ngraham

D17595: Upstream Dolphin's file rename dialog

[Méven Car]

meven marked 4 inline comments as done.
meven added a comment.


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

INLINE COMMENTS

> dfaure wrote in renamefiledialog.cpp:119
> items.first().path() would be simpler and faster.
> (that doesn't give the same result, but it's sufficient for calling 
> suffixForFileName with it)

KFileItem has no path()  method, but here items.first().url().fileName() should 
be equivalent.

> dfaure wrote in renamefiledialog.cpp:121
> why toLower()? Should work without.

I guess the database doesn't work for JPEG file extension for instance.

> dfaure wrote in renamefiledialog.cpp:208
> 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.

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.

> dfaure wrote in renamefiledialog.cpp:60
> 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.

I just did not bother going through those hoops, but this worked out great. 
Thanks

REPOSITORY
  R241 KIO

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

To: meven, #frameworks, #dolphin, broulik, ngraham, dfaure
Cc: dfaure, sitter, mitchell, emmanuelp, ltoscano, bruns, meven, dhaumann, 
pino, kde-frameworks-devel, LeGast00n, michaelh, ngraham

D17595: Upstream Dolphin's file rename dialog

[Méven Car]

meven updated this revision to Diff 63854.
meven marked 14 inline comments as done.
meven added a comment.


  uncrustify file, typo fixes, add error slot

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D17595?vs=63796=63854

BRANCH
  arcpatch-D17595

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

AFFECTED FILES
  src/widgets/CMakeLists.txt
  src/widgets/renamefiledialog.cpp
  src/widgets/renamefiledialog.h

To: meven, #frameworks, #dolphin, broulik, ngraham, dfaure
Cc: dfaure, sitter, mitchell, emmanuelp, ltoscano, bruns, meven, dhaumann, 
pino, kde-frameworks-devel, LeGast00n, michaelh, ngraham

D17595: Upstream Dolphin's file rename dialog

[David Faure]

dfaure requested changes to this revision.
dfaure added inline comments.
This revision now requires changes to proceed.

INLINE COMMENTS

> renamefiledialog.cpp:67
> +{
> +const QSize minSize = minimumSize();
> +setMinimumSize(QSize(320, minSize.height()));

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.

> renamefiledialog.cpp:119
> +if (!items.first().isDir()) {
> +const QString fileName = items.first().url().toDisplayString();
> +QMimeDatabase db;

items.first().path() would be simpler and faster.
(that doesn't give the same result, but it's sufficient for calling 
suffixForFileName with it)

> renamefiledialog.cpp:121
> +QMimeDatabase db;
> +const QString extension = 
> db.suffixForFileName(fileName.toLower());
> +

why toLower()? Should work without.

> renamefiledialog.cpp:143
> +for (const KFileItem& item : qAsConst(d->items)) {
> +const QString extension = 
> db.suffixForFileName(item.url().toDisplayString().toLower());
> +

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.

> renamefiledialog.cpp:208
> +
> +job->uiDelegate()->setAutoErrorHandlingEnabled(true);
> +

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.

> renamefiledialog.cpp:222
> +} else {
> +// Assure that the new name contains exactly one # (or a 
> connected sequence of #'s)
> +const int first = newName.indexOf(QLatin1Char('#'));

s/Assure/Ensure/ ?

> renamefiledialog.cpp:246
> +{
> +d->lineEdit->setFocus();
> +

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.

> meven wrote in renamefiledialog.cpp:60
> 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.

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.

> renamefiledialog.h:47
> + * The dialog deletes itself when accepted or rejected.
> + */
> +class KIOWIDGETS_EXPORT RenameFileDialog : public QDialog

@since 5.62

> renamefiledialog.h:59
> + */
> +explicit RenameFileDialog(QWidget* parent, const KFileItemList& items);
> +~RenameFileDialog() override;

Generally the parent widget argument goes last.

> meven wrote in renamefiledialog.h:76
> I tried it but it did not work.

Yeah, it's more complicated inside namespaces.

REPOSITORY
  R241 KIO

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

To: meven, #frameworks, #dolphin, broulik, ngraham, dfaure
Cc: dfaure, sitter, mitchell, emmanuelp, ltoscano, bruns, meven, dhaumann, 
pino, kde-frameworks-devel, LeGast00n, michaelh, ngraham

D17595: Upstream Dolphin's file rename dialog

[Méven Car]

meven marked an inline comment as done.
meven added inline comments.

INLINE COMMENTS

> dfaure wrote in renamefiledialog.cpp:60
> Just run `uncrustify-kf5` from the repo kde-dev-scripts.

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.

REPOSITORY
  R241 KIO

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

To: meven, #frameworks, #dolphin, broulik, ngraham
Cc: dfaure, sitter, mitchell, emmanuelp, ltoscano, bruns, meven, dhaumann, 
pino, kde-frameworks-devel, LeGast00n, michaelh, ngraham

D17595: Upstream Dolphin's file rename dialog

[Méven Car]

meven updated this revision to Diff 63796.
meven added a comment.


  Group boolean fields together

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D17595?vs=62026=63796

BRANCH
  arcpatch-D17595

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

AFFECTED FILES
  src/widgets/CMakeLists.txt
  src/widgets/renamefiledialog.cpp
  src/widgets/renamefiledialog.h

To: meven, #frameworks, #dolphin, broulik, ngraham
Cc: dfaure, sitter, mitchell, emmanuelp, ltoscano, bruns, meven, dhaumann, 
pino, kde-frameworks-devel, LeGast00n, michaelh, ngraham

D17595: Upstream Dolphin's file rename dialog

[David Faure]

dfaure added inline comments.

INLINE COMMENTS

> renamefiledialog.cpp:59
> +KFileItemList items;
> +bool allExtensionsDifferent;
> +QSpinBox* spinBox;

group the bools together for less padding

> meven wrote in renamefiledialog.cpp:60
> I really wish we would enforce use of a code formater, community wise ...
> This is something I miss from rust:  `cargo fmt`
> Such as astyle.

Just run `uncrustify-kf5` from the repo kde-dev-scripts.

REPOSITORY
  R241 KIO

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

To: meven, #frameworks, #dolphin, broulik, ngraham
Cc: dfaure, sitter, mitchell, emmanuelp, ltoscano, bruns, meven, dhaumann, 
pino, kde-frameworks-devel, LeGast00n, sbergeron, michaelh, ngraham

D17595: Upstream Dolphin's file rename dialog

[Méven Car]

meven marked 4 inline comments as done.

REPOSITORY
  R241 KIO

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

To: meven, #frameworks, #dolphin, broulik, ngraham
Cc: sitter, mitchell, emmanuelp, ltoscano, bruns, meven, dhaumann, pino, 
kde-frameworks-devel, LeGast00n, sbergeron, michaelh, ngraham

D17595: Upstream Dolphin's file rename dialog

[Méven Car]

meven marked 4 inline comments as done.
meven added inline comments.

INLINE COMMENTS

> sitter wrote in renamefiledialog.h:72
> You'll also want to override `event` as per 
> https://community.kde.org/Policies/Binary_Compatibility_Issues_With_C%2B%2B

Thanks
This is really arcane, I am really not found of C++.

> sitter wrote in renamefiledialog.h:76
> You can in fact make the class forward declaration and the member declaration 
> one line
> 
> `class RenameFileDialogPrivate *const d;`

I tried it but it did not work.

REPOSITORY
  R241 KIO

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

To: meven, #frameworks, #dolphin, broulik, ngraham
Cc: sitter, mitchell, emmanuelp, ltoscano, bruns, meven, dhaumann, pino, 
kde-frameworks-devel, LeGast00n, sbergeron, michaelh, ngraham

D17595: Upstream Dolphin's file rename dialog

[Harald Sitter]

sitter added inline comments.

INLINE COMMENTS

> renamefiledialog.h:72
> +protected:
> +void showEvent(QShowEvent* event) override;
> +

You'll also want to override `event` as per 
https://community.kde.org/Policies/Binary_Compatibility_Issues_With_C%2B%2B

> renamefiledialog.h:75
> +private:
> +class RenameFileDialogPrivate;
> +RenameFileDialogPrivate *const d;

This is now forward declared twice.

> renamefiledialog.h:76
> +class RenameFileDialogPrivate;
> +RenameFileDialogPrivate *const d;
> +};

You can in fact make the class forward declaration and the member declaration 
one line

`class RenameFileDialogPrivate *const d;`

REPOSITORY
  R241 KIO

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

To: meven, #frameworks, #dolphin, broulik, ngraham
Cc: sitter, mitchell, emmanuelp, ltoscano, bruns, meven, dhaumann, pino, 
kde-frameworks-devel, LeGast00n, sbergeron, michaelh, ngraham

D17595: Upstream Dolphin's file rename dialog

[Méven Car]

meven updated this revision to Diff 62026.
meven added a comment.


  Add context about filename #

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D17595?vs=62005=62026

BRANCH
  arcpatch-D17595

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

AFFECTED FILES
  src/widgets/CMakeLists.txt
  src/widgets/renamefiledialog.cpp
  src/widgets/renamefiledialog.h

To: meven, #frameworks, #dolphin, broulik, ngraham
Cc: mitchell, emmanuelp, ltoscano, bruns, meven, dhaumann, pino, 
kde-frameworks-devel, LeGast00n, sbergeron, michaelh, ngraham

D17595: Upstream Dolphin's file rename dialog

[Méven Car]

meven updated this revision to Diff 62005.
meven added a comment.


  Fix email in copyright declaration

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D17595?vs=59292=62005

BRANCH
  arcpatch-D17595

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

AFFECTED FILES
  src/widgets/CMakeLists.txt
  src/widgets/renamefiledialog.cpp
  src/widgets/renamefiledialog.h

To: meven, #frameworks, #dolphin, broulik, ngraham
Cc: mitchell, emmanuelp, ltoscano, bruns, meven, dhaumann, pino, 
kde-frameworks-devel, LeGast00n, sbergeron, michaelh, ngraham

D17595: Upstream Dolphin's file rename dialog

[Méven Car]

meven marked 3 inline comments as done.

REPOSITORY
  R241 KIO

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

To: meven, #frameworks, #dolphin, broulik, ngraham
Cc: mitchell, emmanuelp, ltoscano, bruns, meven, dhaumann, pino, 
kde-frameworks-devel, LeGast00n, sbergeron, michaelh, ngraham

D17595: Upstream Dolphin's file rename dialog

[Méven Car]

meven added a subscriber: mitchell.
meven added a comment.


  In D17595#481526 , @ngraham wrote:
  
  > Who's left?
  
  
  Only three now :
  Jeff Mitchell @mitchell ,  Frank Reininghaus and Peter Penz.
  
  But Peter Penz is the most important one as he was the original author of 
this code.

REPOSITORY
  R241 KIO

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

To: meven, #frameworks, #dolphin, broulik, ngraham
Cc: mitchell, emmanuelp, ltoscano, bruns, meven, dhaumann, pino, 
kde-frameworks-devel, LeGast00n, sbergeron, michaelh, ngraham

D17595: Upstream Dolphin's file rename dialog

[Nathaniel Graham]

ngraham added a comment.


  Who's left?

REPOSITORY
  R241 KIO

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

To: meven, #frameworks, #dolphin, broulik, ngraham
Cc: emmanuelp, ltoscano, bruns, meven, dhaumann, pino, kde-frameworks-devel, 
LeGast00n, michaelh, ngraham

D17595: Upstream Dolphin's file rename dialog

[Méven Car]

meven planned changes to this revision.
meven added a comment.


  Still need 4 agreements from copyrights holders to make the necessary license 
change.

REPOSITORY
  R241 KIO

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

To: meven, #frameworks, #dolphin, broulik, ngraham
Cc: emmanuelp, ltoscano, bruns, meven, dhaumann, pino, kde-frameworks-devel, 
LeGast00n, michaelh, ngraham

D17595: Upstream Dolphin's file rename dialog

[Méven Car]

meven added a comment.


  In D17595#478067 , @emmanuelp 
wrote:
  
  > In D17595#473825 , @meven wrote:
  >
  > > I guess we need now to reach to schwarz...@gmail.com romaninflianskas 
mfuchs chinmoyr mitchell emateli emmanuelp freininghaus to have lgplv2+ 
approval.
  >
  >
  > Added myself to the relicense-check script, including the  lgplv2+ approval.
  
  
  Great to here :)
  
  Unfortunately even with this sort of FLA, it would not suffice here.
  It does not covers GPL to LGPL relicensing only GPL to later version, or LGPL 
to later version, and authorizing the KDE e.V. to do it.

REPOSITORY
  R241 KIO

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

To: meven, #frameworks, #dolphin, broulik, ngraham
Cc: emmanuelp, ltoscano, bruns, meven, dhaumann, pino, kde-frameworks-devel, 
LeGast00n, michaelh, ngraham

D17595: Upstream Dolphin's file rename dialog

[Emmanuel Pescosta]

emmanuelp added a comment.


  In D17595#473825 , @meven wrote:
  
  > I guess we need now to reach to schwarz...@gmail.com romaninflianskas 
mfuchs chinmoyr mitchell emateli emmanuelp freininghaus to have lgplv2+ 
approval.
  
  
  Added myself to the relicense-check script, including the  lgplv2+ approval.

REPOSITORY
  R241 KIO

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

To: meven, #frameworks, #dolphin, broulik, ngraham
Cc: emmanuelp, ltoscano, bruns, meven, dhaumann, pino, kde-frameworks-devel, 
LeGast00n, michaelh, ngraham

D17595: Upstream Dolphin's file rename dialog

[Méven Car]

meven added inline comments.

INLINE COMMENTS

> bruns wrote in renamefiledialog.cpp:105
> The code assumes the "#" is the very last character ("Don't select the # 
> character"), so this requires a translation comment.

Is that better line 105 ?

REPOSITORY
  R241 KIO

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

To: meven, #frameworks, #dolphin, broulik, ngraham
Cc: ltoscano, bruns, meven, dhaumann, pino, kde-frameworks-devel, LeGast00n, 
michaelh, ngraham

D17595: Upstream Dolphin's file rename dialog

[Méven Car]

meven updated this revision to Diff 59292.
meven marked an inline comment as done.
meven added a comment.


  Improve localization context

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D17595?vs=59255=59292

BRANCH
  arcpatch-D17595_1

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

AFFECTED FILES
  src/widgets/CMakeLists.txt
  src/widgets/renamefiledialog.cpp
  src/widgets/renamefiledialog.h

To: meven, #frameworks, #dolphin, broulik, ngraham
Cc: ltoscano, bruns, meven, dhaumann, pino, kde-frameworks-devel, LeGast00n, 
michaelh, ngraham

D17595: Upstream Dolphin's file rename dialog

[Stefan Brüns]

bruns added inline comments.

INLINE COMMENTS

> renamefiledialog.cpp:105
> +} else {
> +newName = i18nc("@info:status", "New name #");
> +editLabel = new QLabel(i18ncp("@label:textbox",

The code assumes the "#" is the very last character ("Don't select the # 
character"), so this requires a translation comment.

REPOSITORY
  R241 KIO

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

To: meven, #frameworks, #dolphin, broulik, ngraham
Cc: ltoscano, bruns, meven, dhaumann, pino, kde-frameworks-devel, LeGast00n, 
michaelh, ngraham

D17595: Upstream Dolphin's file rename dialog

[Méven Car]

meven marked 7 inline comments as done.
meven added inline comments.

INLINE COMMENTS

> bruns wrote in renamefiledialog.cpp:126
> better name - `m_extensionsAreUnique`

The current name seems fine to me.

REPOSITORY
  R241 KIO

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

To: meven, #frameworks, #dolphin, broulik, ngraham
Cc: ltoscano, bruns, meven, dhaumann, pino, kde-frameworks-devel, LeGast00n, 
michaelh, ngraham

D17595: Upstream Dolphin's file rename dialog

[Méven Car]

meven updated this revision to Diff 59255.
meven added a comment.


Add a Private class to RenameFileDialog, code fixes

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D17595?vs=47606=59255

BRANCH
  arcpatch-D17595

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

AFFECTED FILES
  src/widgets/CMakeLists.txt
  src/widgets/renamefiledialog.cpp
  src/widgets/renamefiledialog.h

To: meven, #frameworks, #dolphin, broulik, ngraham
Cc: ltoscano, bruns, meven, dhaumann, pino, kde-frameworks-devel, LeGast00n, 
michaelh, ngraham

D17595: Upstream Dolphin's file rename dialog

[Méven Car]

meven commandeered this revision.
meven added a reviewer: ngraham.
meven added inline comments.

INLINE COMMENTS

> pino wrote in renamefiledialog.cpp:50-51
> why a minimum size is enforced?

For files whose name is short, I think the dialog would not look nice.
This look fine in dolphin.
Was added in 507984415a2953000ef1edb77c9dbc3364846b13 


> pino wrote in renamefiledialog.cpp:54
> assert in a library, especially when checking user input, is never a good 
> idea...

What would be better ?
An ASSERT, removing it altogether ? Printing the message to std-err ?

> bruns wrote in renamefiledialog.cpp:60
> space around `|`

I really wish we would enforce use of a code formater, community wise ...
This is something I miss from rust:  `cargo fmt`
Such as astyle.

> bruns wrote in renamefiledialog.cpp:101
> is this correct for e.g. `foo.jpeg`? - the preferred extension is `jpg`, so 
> this may select to many characters

I have tested it, it works fine with .jpeg files.

REPOSITORY
  R241 KIO

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

To: meven, #frameworks, #dolphin, broulik, ngraham
Cc: ltoscano, bruns, meven, dhaumann, pino, kde-frameworks-devel, LeGast00n, 
michaelh, ngraham

D17595: Upstream Dolphin's file rename dialog

[Nathaniel Graham]

ngraham added a comment.


  Thanks so much! \o/

REPOSITORY
  R241 KIO

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

To: ngraham, #frameworks, #dolphin, broulik
Cc: ltoscano, bruns, meven, dhaumann, pino, kde-frameworks-devel, LeGast00n, 
michaelh, ngraham

D17595: Upstream Dolphin's file rename dialog

[Méven Car]

meven added a comment.


  In D17595#473826 , @ngraham wrote:
  
  > Would you be interested in doing that and taking over this patch? I'm 
afraid I don't think I have the time right now for the kind of deep dive that 
it requires. If not, that's okay. :)
  
  
  I can take over ;)
  
  I still have some unclear area .
  relicensecheck.pl does not seem to allow to re-license from GPL to LGPL, just 
to update the version of the license used.
  Only the +eV option seems to allow this. 
  It seems to me I should ask the author to allow KDE e.V. to re-license their 
work.
  I have contacted KDE e.V board to ask for some guidance.

REPOSITORY
  R241 KIO

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

To: ngraham, #frameworks, #dolphin, broulik
Cc: ltoscano, bruns, meven, dhaumann, pino, kde-frameworks-devel, LeGast00n, 
michaelh, ngraham

D17595: Upstream Dolphin's file rename dialog

[Nathaniel Graham]

ngraham added a comment.


  Would you be interested in doing that and taking over this patch? I'm afraid 
I don't think I have the time right now for the kind of deep dive that it 
requires. If not, that's okay. :)

REPOSITORY
  R241 KIO

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

To: ngraham, #frameworks, #dolphin, broulik
Cc: ltoscano, bruns, meven, dhaumann, pino, kde-frameworks-devel, michaelh, 
ngraham

D17595: Upstream Dolphin's file rename dialog

[Méven Car]

meven added a comment.


  In dolphin repo
  
relicensecheck.pl views/renamedialog.h 
The following emails do not appear in the accounts file:

schwarz...@gmail.com

Need permission for licensing:

- gplv2+: schwarz...@gmail.com mfuchs romaninflianskas mitchell chinmoyr 
freininghaus emmanuelp emateli
   mfuchs (   3 LOC): b51083c12 
romaninflianskas (   2 LOC): 48b58f830 
 chinmoyr (   3 LOC): c5eb4e311 
freininghaus (   1 LOC): e8c4d19b7 
emmanuelp (  10 LOC): 4143a69c0,25751088c,424d20e43,02c083514 
  emateli (   4 LOC): 478f404b8 

- CCBYSA4: chinmoyr mlaurent freininghaus kfunk emateli dfaure ervin 
schwarz...@gmail.com mfuchs romaninflianskas mitchell emmanuelp ppenz craig 
elvisangelaccio
 chinmoyr (   3 LOC): c5eb4e311 
 mlaurent (   3 LOC): cd601a582,10a02f0bc 
freininghaus (   1 LOC): e8c4d19b7 
kfunk (   1 LOC): 464b13f38 
  emateli (   4 LOC): 478f404b8 
   dfaure (   1 LOC): dacded2af,3aa47b789 
   mfuchs (   3 LOC): b51083c12 
romaninflianskas (   2 LOC): 48b58f830 
emmanuelp (  10 LOC): 4143a69c0,25751088c,424d20e43,02c083514 
ppenz (  39 LOC): 
1479e8231,8eb9b508c,828ba8902,a5cf21ff0,c848b945d,8fd40e72a,ba150e573,15b965af3,d8d894d4d,c8a4f1fd8,507984415,d9ac44e08,d8ab89171,5252c12db
 
craig (   1 LOC): ad74c99bb 
elvisangelaccio (   2 LOC): e308985de 

- lgplv2+: schwarz...@gmail.com romaninflianskas mfuchs mitchell chinmoyr 
freininghaus emmanuelp emateli
romaninflianskas (   2 LOC): 48b58f830 
   mfuchs (   3 LOC): b51083c12 
 chinmoyr (   3 LOC): c5eb4e311 
freininghaus (   1 LOC): e8c4d19b7 
emmanuelp (  10 LOC): 4143a69c0,25751088c,424d20e43,02c083514 
  emateli (   4 LOC): 478f404b8 

- lgplv23: schwarz...@gmail.com romaninflianskas mfuchs chinmoyr mitchell 
emateli emmanuelp freininghaus
romaninflianskas (   2 LOC): 48b58f830 
   mfuchs (   3 LOC): b51083c12 
 chinmoyr (   3 LOC): c5eb4e311 
  emateli (   4 LOC): 478f404b8 
emmanuelp (  10 LOC): 4143a69c0,25751088c,424d20e43,02c083514 
freininghaus (   1 LOC): e8c4d19b7 

- gplv23: romaninflianskas mfuchs schwarz...@gmail.com emateli freininghaus 
emmanuelp chinmoyr mitchell
romaninflianskas (   2 LOC): 48b58f830 
   mfuchs (   3 LOC): b51083c12 
  emateli (   4 LOC): 478f404b8 
freininghaus (   1 LOC): e8c4d19b7 
emmanuelp (  10 LOC): 4143a69c0,25751088c,424d20e43,02c083514 
 chinmoyr (   3 LOC): c5eb4e311 

- +eV: chinmoyr mitchell emateli freininghaus emmanuelp 
schwarz...@gmail.com ppenz romaninflianskas mfuchs
 chinmoyr (   3 LOC): c5eb4e311 
  emateli (   4 LOC): 478f404b8 
freininghaus (   1 LOC): e8c4d19b7 
emmanuelp (  10 LOC): 4143a69c0,25751088c,424d20e43,02c083514 
ppenz (  39 LOC): 
1479e8231,8eb9b508c,828ba8902,a5cf21ff0,c848b945d,8fd40e72a,ba150e573,15b965af3,d8d894d4d,c8a4f1fd8,507984415,d9ac44e08,d8ab89171,5252c12db
 
romaninflianskas (   2 LOC): 48b58f830 
   mfuchs (   3 LOC): b51083c12 


Summary:
9 commits preventing relicensing to lgplv2+
9 commits preventing relicensing to gplv23
9 commits preventing relicensing to lgplv23
9 commits preventing relicensing to gplv2+
   23 commits preventing relicensing to +eV
   30 commits preventing relicensing to CCBYSA4
  
  I guess we need now to reach to schwarz...@gmail.com romaninflianskas mfuchs 
chinmoyr mitchell emateli emmanuelp freininghaus to have lgplv2+ approval.

REPOSITORY
  R241 KIO

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

To: ngraham, #frameworks, #dolphin, broulik
Cc: ltoscano, bruns, meven, dhaumann, pino, kde-frameworks-devel, michaelh, 
ngraham

D17595: Upstream Dolphin's file rename dialog

[Méven Car]

meven added inline comments.

INLINE COMMENTS

> ltoscano wrote in renamefiledialog.h:5
> We do, but people sign it on a vuluntary basis:
> 
> https://ev.kde.org/rules/fla.php
> 
> Also, there is this which pre-dates the FLA but it is useful too (and still 
> in use):
> 
> https://cgit.kde.org/kde-dev-scripts.git/tree/relicensecheck.pl

Interesting it happens that Peter Penz is present in the old_license_table :

  ppenz' => ['gplv23', 'lgplv23', 'gplv2+', 'lgplv2+' ],

Which if I understand correctly means Peter Penz has allowed to move his gpl2+ 
contributions to some lgpl2+.

Hence it seems to me this code move and needed re-licensing can proceed.

I would appreciate some second opinion though, I am no FLA specialist.

REPOSITORY
  R241 KIO

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

To: ngraham, #frameworks, #dolphin, broulik
Cc: ltoscano, bruns, meven, dhaumann, pino, kde-frameworks-devel, michaelh, 
ngraham

D17595: Upstream Dolphin's file rename dialog

[Dominik Haumann]

dhaumann added a comment.


  Yes, please ask Peter Penz and CC a public mailing list and/or use the 
relicense check script. I am sure Peter would agree to a license change.

REPOSITORY
  R241 KIO

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

To: ngraham, #frameworks, #dolphin, broulik
Cc: ltoscano, bruns, meven, dhaumann, pino, kde-frameworks-devel, michaelh, 
ngraham

D17595: Upstream Dolphin's file rename dialog

[Luigi Toscano]

ltoscano added inline comments.

INLINE COMMENTS

> meven wrote in renamefiledialog.h:5
> We can email Peter Penz to ask about doing a license change.
> 
> I wish we would ask its contributors to allow the kde foundation to update 
> code distribution license on their behalf, provided the license stays 
> FOSS/mostly compatible and they keep their copyright.
> Some projects do so, but I can't remember which ones.

We do, but people sign it on a vuluntary basis:

https://ev.kde.org/rules/fla.php

Also, there is this which pre-dates the FLA but it is useful too (and still in 
use):

https://cgit.kde.org/kde-dev-scripts.git/tree/relicensecheck.pl

REPOSITORY
  R241 KIO

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

To: ngraham, #frameworks, #dolphin, broulik
Cc: ltoscano, bruns, meven, dhaumann, pino, kde-frameworks-devel, michaelh, 
ngraham

D17595: Upstream Dolphin's file rename dialog

[Stefan Brüns]

bruns added inline comments.

INLINE COMMENTS

> renamefiledialog.cpp:60
> +   i18nc("@title:window", "Rename Items"));
> +QDialogButtonBox *buttonBox = new 
> QDialogButtonBox(QDialogButtonBox::Ok|QDialogButtonBox::Cancel);
> +QVBoxLayout *mainLayout = new QVBoxLayout;

space around `|`

> renamefiledialog.cpp:81
> +if (m_renameOneItem) {
> +m_newName = items.first().name();
> +editLabel = new QLabel(xi18nc("@label:textbox", "Rename the item 
> %1 to:", m_newName),

`m_newName` is only used in the constructor, no need for it to be a member

> renamefiledialog.cpp:101
> +QMimeDatabase db;
> +const QString extension = db.suffixForFileName(fileName.toLower());
> +

is this correct for e.g. `foo.jpeg`? - the preferred extension is `jpg`, so 
this may select to many characters

> renamefiledialog.cpp:104
> +// If the current item is a directory, select the whole file name.
> +if ((extension.length() > 0) && !items.first().isDir()) {
> +// Don't select the extension

the `!isDir()` should apply to the whole block, i.e. create the QMimeDatabase 
instance only if needed

> renamefiledialog.cpp:121
> +QSet extensions;
> +foreach (const KFileItem& item, m_items) {
> +QMimeDatabase db;

`qAsConst(m_items)`

> renamefiledialog.cpp:122
> +foreach (const KFileItem& item, m_items) {
> +QMimeDatabase db;
> +const QString extension = 
> db.suffixForFileName(item.url().toDisplayString().toLower());

move db instance out of loop

> renamefiledialog.cpp:126
> +if (extensions.contains(extension)) {
> +m_allExtensionsDifferent = false;
> +break;

better name - `m_extensionsAreUnique`

> renamefiledialog.cpp:174
> +job = KIO::moveAs(oldUrl, newUrl, KIO::HideProgressInfo);
> +} else {
> +cmdType = KIO::FileUndoManager::BatchRename;

`m_renamedItems.reserve(m_items.count())` in this block

REPOSITORY
  R241 KIO

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

To: ngraham, #frameworks, #dolphin, broulik
Cc: bruns, meven, dhaumann, pino, kde-frameworks-devel, michaelh, ngraham

D17595: Upstream Dolphin's file rename dialog

[Méven Car]

meven added inline comments.

INLINE COMMENTS

> pino wrote in renamefiledialog.h:5
> > The KIO repo's COPYING.LIB file says LGPL 2.1. Is that a mismatch?
> 
> Considering this new code is GPL: yes, it's a mismatch, and it would make the 
> whole kiowidgets as GPL.

We can email Peter Penz to ask about doing a license change.

I wish we would ask its contributors to allow the kde foundation to update code 
distribution license on their behalf, provided the license stays FOSS/mostly 
compatible and they keep their copyright.
Some projects do so, but I can't remember which ones.

REPOSITORY
  R241 KIO

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

To: ngraham, #frameworks, #dolphin, broulik
Cc: meven, dhaumann, pino, kde-frameworks-devel, michaelh, ngraham, bruns

D17595: Upstream Dolphin's file rename dialog

[Pino Toscano]

pino added inline comments.

INLINE COMMENTS

> ngraham wrote in renamefiledialog.h:5
> The KIO repo's COPYING.LIB file says LGPL 2.1. Is that a mismatch?

> The KIO repo's COPYING.LIB file says LGPL 2.1. Is that a mismatch?

Considering this new code is GPL: yes, it's a mismatch, and it would make the 
whole kiowidgets as GPL.

REPOSITORY
  R241 KIO

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

To: ngraham, #frameworks, #dolphin, broulik
Cc: dhaumann, pino, kde-frameworks-devel, michaelh, ngraham, bruns

D17595: Upstream Dolphin's file rename dialog

[Nathaniel Graham]

ngraham added inline comments.

INLINE COMMENTS

> dhaumann wrote in renamefiledialog.h:5
> Is the rest of KIO LGPL or GPL? If we have a license mismatch, we'd have to 
> ask the authors for an OK to relicense.

The KIO repo's COPYING.LIB file says LGPL 2.1. Is that a mismatch?

REPOSITORY
  R241 KIO

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

To: ngraham, #frameworks, #dolphin, broulik
Cc: dhaumann, pino, kde-frameworks-devel, michaelh, ngraham, bruns

D17595: Upstream Dolphin's file rename dialog

[Dominik Haumann]

dhaumann added inline comments.

INLINE COMMENTS

> renamefiledialog.h:5
> + *   This program is free software; you can redistribute it and/or modify  *
> + *   it under the terms of the GNU General Public License as published by  *
> + *   the Free Software Foundation; either version 2 of the License, or *

Is the rest of KIO LGPL or GPL? If we have a license mismatch, we'd have to ask 
the authors for an OK to relicense.

REPOSITORY
  R241 KIO

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

To: ngraham, #frameworks, #dolphin, broulik
Cc: dhaumann, pino, kde-frameworks-devel, michaelh, ngraham, bruns

D17595: Upstream Dolphin's file rename dialog

[Pino Toscano]

pino added inline comments.

INLINE COMMENTS

> renamefiledialog.cpp:50-51
> +{
> +const QSize minSize = minimumSize();
> +setMinimumSize(QSize(320, minSize.height()));
> +

why a minimum size is enforced?

> renamefiledialog.cpp:54
> +const int itemCount = items.count();
> +Q_ASSERT(itemCount >= 1);
> +m_renameOneItem = (itemCount == 1);

assert in a library, especially when checking user input, is never a good 
idea...

> renamefiledialog.h:65-73
> +private:
> +bool m_renameOneItem;
> +QList m_renamedItems;
> +QString m_newName;
> +QLineEdit* m_lineEdit;
> +KFileItemList m_items;
> +bool m_allExtensionsDifferent;

an exported class must use a d-pointer for all the private variables & members

REPOSITORY
  R241 KIO

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

To: ngraham, #frameworks, #dolphin, broulik
Cc: pino, kde-frameworks-devel, michaelh, ngraham, bruns

D17595: Upstream Dolphin's file rename dialog

[Nathaniel Graham]

ngraham added a dependent revision: D17597: Use newly-upstreamed rename dialog 
from KIO.

REPOSITORY
  R241 KIO

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

To: ngraham, #frameworks, #dolphin, broulik
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D17595: Upstream Dolphin's file rename dialog

[Nathaniel Graham]

ngraham added a dependent revision: D17596: [KDirOperator] Allow renaming files 
from the context menu.

REPOSITORY
  R241 KIO

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

To: ngraham, #frameworks, #dolphin, broulik
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D17595: Upstream Dolphin's file rename dialog

[Nathaniel Graham]

ngraham created this revision.
ngraham added reviewers: Frameworks, Dolphin, broulik.
Herald added a project: Frameworks.
Herald added a subscriber: kde-frameworks-devel.
ngraham requested review of this revision.

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

TEST PLAN
  Apply [patch X] or [patch Y] and initiate a rename operation for one or more 
files. See that it works as expected.

REPOSITORY
  R241 KIO

BRANCH
  upstream-dolphins-rename-dialog (branched from master)

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

AFFECTED FILES
  src/widgets/CMakeLists.txt
  src/widgets/renamefiledialog.cpp
  src/widgets/renamefiledialog.h

To: ngraham, #frameworks, #dolphin, broulik
Cc: kde-frameworks-devel, michaelh, ngraham, bruns