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

[David Faure]

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

INLINE COMMENTS

> batchrenametypestest.h:20
> +
> +#ifndef KIO_RENAMETYPESTEST_H
> +#define KIO_RENAMETYPESTEST_H

Doesn't match the header filename, and anyway, feel free to move this to the .cpp file, for qtestlib unittests.

> batchrenamedialog.cpp:193
> +
> +    params[QStringLiteral("timestamp")] = QDateTime::currentDateTime();
> +

params.insert(key, value) is faster than params[key] = value, see the book Effective C++ (or was it Effective STL?)

> batchrenamedialog.cpp:221
> +        bool valid = (newName == itemName) || !(newName.isEmpty() || exists);
> +        allItemsOk &= valid;
> +        generatedNames.append(newName);

bitfield operation on booleans can lead to unexpected behaviour.
Use `allItemsOk = allItemsOk && valid` instead, or `if (!valid) allItemsOk = false;`

> batchrenamedialog.cpp:257
> +
> +    if (!job->error()) {
> +        m_renamedItems << newUrl;

This makes no sense, you can't test for a job error before the job is done.
Connect to the result signal and do that in the slot (e.g. lambda) if it should really only be done on success -- after the move happened.

> batchrenamedialog.cpp:264
> +{
> +    for (const auto &pair: m_itemsToBeRenamed) {
> +        renameItem(pair.first, pair.second);

qAsConst(m_itemsToBeRenamed)

> batchrenamedialog.cpp:287
> +{
> +    Q_UNUSED(checked);
> +

Just remove the argument from the slot then.

> batchrenamedialog.cpp:365
> +    for (int i = 0; i < capturedGroups.length(); i++) {
> +        if (capturedGroups[i].length() == 0) {
> +            continue;

use .isEmpty()

> batchrenamedialog.cpp:377
> +}
> \ No newline at end of file


Please configure your text editor to add newlines at the end of files. Surprising that it doesn't do that, most do.

> batchrenamedialog.h:44
> +{
> +Q_OBJECT
> +

indent this line by 4 spaces please

> batchrenamedialogmodel.cpp:28
> +int BatchRenameDialogModel::rowCount(const QModelIndex &parent) const {
> +    Q_UNUSED(parent);
> +    return itemData->count();

Technically this should be `if (parent.isValid()) { return 0; }`
in case anyone plugs this model into a QTreeView one day, or plugs a proxy that supports trees.

> batchrenamedialogmodel.cpp:79
> +
> +BatchRenameDialogModel::BatchRenameDialogModel(QObject *parent, const KFileItemList &items) : QAbstractTableModel(parent) {
> +    itemData = new QList<BatchRenameDialogModelData>;

the '{' that opens a method implementation should go on a separate line (repeats below)

> batchrenamedialogmodel.h:35
> +
> +    BatchRenameDialogModelData(const KFileItem &item, QString newName, bool valid)
> +    {

This ctor isn't really needed, one could use aggregate initialization instead, no?
i.e. BatchRenameDialogModelData{item, newName, valid}

(Not 100% sure)

If you keep this ctor, then use member initialization syntax.

> batchrenamedialogmodel.h:47
> +Q_OBJECT
> +protected:
> +    QList<BatchRenameDialogModelData> *itemData;

private (AFAICS nothing derives from this class), and at the end please.

> batchrenametypes.h:32
> +
> +class KIOWIDGETS_EXPORT BatchRenameTypes
> +{

This looks more like a namespace than an actual class, given that everything is static.

Alternatively (and even better), make capturedGroups and the two associated methods non-static, meaning that one has to instanciate the class in order to use it. This is only used in the dialog, right? So there's no need for this "global" variable, it can just be a member of BatchRenameTypes which can be a member of the dialog, AFAICS.

> batchrenamevar.h:39
> +
> +    static const QString dateYear4Digit;
> +    static const QString dateYear2Digit;

Make all these functions rather than variables. It will speed up starting time (right now all those QStrings have to be created upfront).

> filenameutils.h:29
> +
> +class KIOWIDGETS_EXPORT FileNameUtils
> +{

This looks more like a namespace than a class, given that it only contains 2 static methods.

(We don't need a ctor / dtor to be generated)

REPOSITORY
  R241 KIO

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

To: emateli, #frameworks, dfaure
Cc: 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/20180805/51a02ea2/attachment-0001.html>