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

> batchrenamedialog.cpp:38
> +#include <KI18n/KLocalizedString>
> +#include <KIOCore/KIO/Job>
> +#include <KIOCore/KIO/CopyJob>

Remove all the framework prefixes. E.g. this should be <KIO/Job>.

This way, it fails to find the include if the include path (which comes from linking to an imported target, in cmake) for the dependency is missing.

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

My comment still stands: it makes no sense to test for error() before the job's `result` signal is emitted.

The comment was marked as done, but there's still no connect to `result`

> batchrenamedialog.h:27
> +
> +#include <QCheckBox>
> +#include <QDialog>

Forward declaration would be enough (`class QCheckBox`).

Same for anything used by pointer (or ref) in this header.

> batchrenamedialog.h:42
> +
> +class KIOWIDGETS_EXPORT BatchRenameDialog : public QDialog
> +{

If you export it in an installed header, you need to document it.

> batchrenamedialog.h:66
> +
> +    QList<QPair<KFileItem, QString>> m_itemsToBeRenamed;
> +    QList<QUrl> m_renamedItems;

If you make this class public, you need to move all members to a Private class, and keep only a "d" pointer here. See all other public classes...

> batchrenamedialogmodel_p.cpp:30
> +{
> +    itemData = new QList<BatchRenameDialogModelData>;
> +

itemData.reserve(items.count());

> batchrenamedialogmodel_p.cpp:40
> +{
> +    Q_UNUSED(parent);
> +

remove this line, it's a lie :)

> batchrenametypes_p.h:20
> +
> +#ifndef KIO_RENAMETYPES
> +#define KIO_RENAMETYPES

doesn't match the name of the header, please adjust

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

why exported? for unit tests? if so, add a comment. If not, remove.

But anyhow this isn't a class, everything is static. To avoid generating a constructor and a destructor, make it a namespace (like you did for BatchRenameVar) and (if needed) export the individual functions.

> batchrenametypes_p.h:35
> +private:
> +    static QStringList capturedGroups;
> +public:

move to .cpp file, it's not needed here at all

> batchrenamevar_p.h:25
> +
> +#include <QStringLiteral>
> +#include <QString>

not used here, remove

> filenameutils_p.h:20
> +
> +#ifndef KIO_FILENAMEUTILS
> +#define KIO_FILENAMEUTILS

_P_H

> batchrenamedialogtest_gui.cpp:32
> +
> +    auto dlg = new KIO::BatchRenameDialog(nullptr, KFileItemList({e1, e2}));
> +    dlg->show();

create it on stack to avoid leaking it

REPOSITORY
  R241 KIO

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

To: emateli, #frameworks, dfaure, mlaurent
Cc: 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/20181110/f7ccb6ae/attachment-0001.html>