dfaure requested changes to this revision.
dfaure added inline comments.
This revision now requires changes to proceed.
INLINE COMMENTS
> krecentfilesmenu.cpp:33
> +
> + QString titleWithSensibleWidth()
> + {
... const?
> krecentfilesmenu.cpp:85
> + QString m_group = QStringLiteral("RecentFiles");
> + std::list<RecentFilesEntry *> m_entries;
> + QSettings *m_settings;
Why not std::vector?
std::list is a linked list, so this smells like pointers to nodes containing
pointers, lots of indirection.
> krecentfilesmenu.cpp:91
> +
> + std::list<RecentFilesEntry *>::const_iterator findEntry(const QUrl &url);
> +};
.... const;
> krecentfilesmenu.cpp:102
> +KRecentFilesMenu::KRecentFilesMenu(QWidget *parent)
> + : QMenu(tr("Recent Files"), parent)
> + , d(new KRecentFilesMenuPrivate)
This is a perfect use case for the constructor-delegation feature of C++11, BTW.
You could call the other ctor from here, and get rid of the init() method.
> krecentfilesmenu.cpp:119
> + setIcon(QIcon::fromTheme(QStringLiteral("document-open-recent")));
> + const QString fileName =
> QStringLiteral("%1/%2staterc").arg(QStandardPaths::writableLocation(QStandardPaths::AppDataLocation),
> QCoreApplication::applicationName());
> + d->m_settings = new QSettings(fileName, QSettings::Format::IniFormat,
> this);
I don't like that this uses the same file as KSharedConfig::openStateConfig().
Subtle parser differences between KConfig and QSettings will corrupt the file.
Maybe %2statesettings, or %2/recentfiles. I like the latter, it's clear what's
in it.
> krecentfilesmenu.cpp:142
> + d->m_settings->beginGroup(d->m_group);
> + int size = d->m_settings->beginReadArray(QStringLiteral("files"));
> +
This could be followed by `d->m_entries.reserve(size);` when using std::vector.
> krecentfilesmenu.h:25
> +
> +#include <list>
> +
not used in this header
> krecentfilesmenu.h:39
> +public:
> + KRecentFilesMenu(const QString &title, QWidget *parent = nullptr);
> + KRecentFilesMenu(QWidget *parent = nullptr);
explicit
> krecentfilesmenu.h:40
> + KRecentFilesMenu(const QString &title, QWidget *parent = nullptr);
> + KRecentFilesMenu(QWidget *parent = nullptr);
> + virtual ~KRecentFilesMenu();
explicit
> krecentfilesmenu.h:41
> + KRecentFilesMenu(QWidget *parent = nullptr);
> + virtual ~KRecentFilesMenu();
> +
remove `virtual`, add `override`
> krecentfilesmenu.h:68
> + /**
> + * Remove an URL from the recent files list.
> + *
I think it's "a URL"
> krecentfilesmenu.h:80
> + */
> + int maximumItems() const;
> +
mention the default value?
REPOSITORY
R236 KWidgetsAddons
REVISION DETAIL
https://phabricator.kde.org/D26448
To: nicolasfella, #frameworks, dfaure
Cc: elvisangelaccio, cfeck, kde-frameworks-devel, LeGast00n, GB_2, michaelh,
ngraham, bruns
