D14631: Adds a new RenameDialog to KIO with more options for batch renaming

[David Faure]

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


  >   "By consulting https://lxr.kde.org/search?_filestring=&_string=BatchRenameJob we can see that Dolphin is the only application to make use of this BatchRenameJob API."
  
  This isn't a valid reason to break BatchRenameJob. KF5 makes the promise to not break API/ABI for third-party applications, which lxr.kde.org wouldn't know about.
  Sorry, if I had realized that this meant "break existing API" I would have objected earlier to this line of thought.

INLINE COMMENTS

> batchrenamejob.cpp:45
> +    QList<KioBatchRenameItem> m_items;
> +    QList<KioBatchRenameItem>::const_iterator m_listIterator;
>      const JobFlags m_flags;

I suggest m_itemsIterator so it's independent from the type (which will be QVector instead of a QList)

> batchrenamejob.cpp:115
>  {
> -    return BatchRenameJobPrivate::newJob(src, newName, index, placeHolder, flags);
> +    return batchRenameFiles({}, flags);
> +}

So this breaks the existing functionality completely !?!?!

The new stuff should rather be a whole different job class, leaving old API untouched (deprecated, and to be removed in KF6).

> batchrenamejob.h:31
>  
> +struct KioBatchRenameItem {
> +    QUrl src;

Needs to be documented since it's public API, including @since 5.54

> batchrenamejob.h:94
>  
> +KIOCORE_EXPORT BatchRenameJob *batchRenameFiles(const QList<KioBatchRenameItem> &items, JobFlags flags = DefaultFlags);
> +

Needs to be documented + @since 5.54

QList of something bigger than the size of a pointer is a bad idea (see e.g. https://marcmutz.wordpress.com/effective-qt/containers/). Please make it a QVector.

> batchrenamedialog.cpp:43
> +#include <KGuiItem>
> +#include <KI18n/KLocalizedString>
> +#include <KIO/Job>

remove KI18n prefix, this isn't a namespaced header

> batchrenamedialog.cpp:53
> +
> +class BatchRenameDialogPrivate : public QDialog
> +{

This is very unusual. Any reason why the public class isn't the one that inherits from QDialog?

This would allow the caller to call show(), instead of show() happening automatically. Just for consistency with all other dialogs.

> batchrenamedialog.cpp:76
> +
> +    QList<QPair<QUrl, QString>> m_itemsToBeRenamed;
> +    QList<QUrl> m_renamedItems;

QVector

> batchrenamedialog.cpp:339
> +
> +    job->exec();
> +}

Better connect to the result() signal of the job and do the rest of btnOkClicked in the slot (or lambda) connected to that signal.
exec() opens many potential issues (re-entrancy).

> batchrenamedialogmodel.cpp:31
> +{
> +    itemData = new QList<BatchRenameDialogModelData>;
> +    itemData->reserve(items.count());

Why is this a pointer instead of just a value member?

> batchrenamedialogmodel_p.h:20
> +
> +#ifndef KIO_BATCHRENAMEDIALOGMODEL
> +#define KIO_BATCHRENAMEDIALOGMODEL

append _P_H, to avoid a clash with a public header of the same name

> batchrenamedialogmodel_p.h:43
> +private:
> +    QList<BatchRenameDialogModelData> *itemData;
> +

QVector

> batchrenametypes.cpp:33
> +
> +QStringList capturedGroups;
> +

static

(and can this global variable be avoided?)

> batchrenamevar_p.h:21
> +
> +#ifndef KIO_BATCHRENAMEVAR_H
> +#define KIO_BATCHRENAMEVAR_H

_P_H

> batchrenamedialogtest_gui.cpp:32
> +
> +    QList<QUrl> items = {};
> +

You can remove the `= {}` which serves no purpose.

REPOSITORY
  R241 KIO

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

To: emateli, #frameworks, dfaure, mlaurent
Cc: chinmoyr, mlaurent, asensi, rkflx, dfaure, aacid, ngraham, kde-frameworks-devel, michaelh, bruns
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-frameworks-devel/attachments/20181216/c373d1a4/attachment-0001.html>