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