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:99
> +    setIcon(QIcon::fromTheme(QStringLiteral("document-open-recent")));
> +    const QString fileName = QStringLiteral("%1/%2recentfiles").arg(QStandardPaths::writableLocation(QStandardPaths::AppDataLocation), QCoreApplication::applicationName());
> +    d->m_settings = new QSettings(fileName, QSettings::Format::IniFormat, this);

maybe %2_recentfiles to improve readability?

> krecentfilesmenu.cpp:189
> +    delete *it;
> +    d->m_entries.erase(it);
> +    rebuildMenu();

How does this work, given that `it` is a const_iterator?
I smell compilation errors on some compilers/OS/debug-or-release mode.

findEntry is only used for add/remove, I guess it shouldn't return a const_iterator but an iterator.

> nicolasfella wrote in krecentfilesmenu.cpp:85
> New entries are pushed to the front, which works in constant time for std::list and linear time for std::vector
> When the maximum is reached the last element is removed which works in constant time for both.
> 
> We could turn it around and add new items to the back and iterate reverse, but then removing the front item would take linear time for std::vector
> 
> Given that n is small (<10 by default) it shouldn't matter much anyway

That's the theory as it was taught in schools years ago.

In practice, moving N pointers that are in the same CPU cache line (because they are in a std::vector) is faster than findEntry accessing nodes all over memory (because of std::list), which means loading N different areas of memory into the CPU cache.
Because the element type is a pointer (and not a complex value class), that "linear time" is a single memmove, hopefully.

Anyway, as you say, it doesn't really matter so I'm happy to let it go, I just wanted to say that I still do believe std::vector is better suited :)

> broulik wrote in krecentfilesmenu.cpp:150
> If we're rewriting this thing anyway, can we please make sure it does not block application startup checking if those files exist, as can happen if you had opened files on an NFS mount before.
> 
> Either do it only when the menu is opened (aboutToShow) and/or asynchronously.

Blocking in aboutToShow is even worse in terms of user experience, isn't it?

If the NFS mount was manual, this will just fail and not block, anyway.
But with automounting this can go very wrong indeed.

You know what I think.... don't mount network drives, use KIO :)

> cfeck wrote in krecentfilesmenu.cpp:215
> Should we truncate the current list if the new max items is smaller?

^ this comment hasn't been addressed yet.

REPOSITORY
  R236 KWidgetsAddons

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

To: nicolasfella, #frameworks, dfaure
Cc: broulik, 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/20200111/b1de1a8f/attachment-0001.html>