D26448: Add KRecentFilesMenu to replace KRecentFileAction

[David Faure]

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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-frameworks-devel/attachments/20200106/ccaced5b/attachment-0001.html>