D26448: Add KRecentFilesMenu to replace KRecentFileAction

[Nicolas Fella]

nicolasfella closed this revision.

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

To: nicolasfella, #frameworks, dfaure
Cc: broulik, elvisangelaccio, cfeck, kde-frameworks-devel, LeGast00n, cblack, 
michaelh, ngraham, bruns

D26448: Add KRecentFilesMenu to replace KRecentFileAction

[David Faure]

dfaure accepted this revision.
dfaure added a comment.


  Thanks :-)

BRANCH
  recentfilemenu

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

To: nicolasfella, #frameworks, dfaure
Cc: broulik, elvisangelaccio, cfeck, kde-frameworks-devel, LeGast00n, cblack, 
michaelh, ngraham, bruns

D26448: Add KRecentFilesMenu to replace KRecentFileAction

[Nicolas Fella]

nicolasfella updated this revision to Diff 83348.
nicolasfella added a comment.


  - Use menus font metric

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D26448?vs=83346=83348

BRANCH
  recentfilemenu

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

AFFECTED FILES
  src/CMakeLists.txt
  src/krecentfilesmenu.cpp
  src/krecentfilesmenu.h

To: nicolasfella, #frameworks, dfaure
Cc: broulik, elvisangelaccio, cfeck, kde-frameworks-devel, LeGast00n, cblack, 
michaelh, ngraham, bruns

D26448: Add KRecentFilesMenu to replace KRecentFileAction

[David Faure]

dfaure added inline comments.

INLINE COMMENTS

> krecentfilesmenu.cpp:74
> +{
> +action = new QAction(titleWithSensibleWidth(qobject_cast *>(menu->parent(;
> +QObject::connect(action, ::triggered, action, [this, menu]() 
> {

QMenu is a QWidget. Why not just pass `menu` as argument?
It would use the menu's font which is more correct than the parent widget's 
font (the menu font could be set by a stylesheet or by 
`QApplication::setFont(font, "QMenu")`.

Also `menu` is never null, which simplifies the code in the method being called.

BRANCH
  recentfilemenu

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

To: nicolasfella, #frameworks, dfaure
Cc: broulik, elvisangelaccio, cfeck, kde-frameworks-devel, LeGast00n, cblack, 
michaelh, ngraham, bruns

D26448: Add KRecentFilesMenu to replace KRecentFileAction

[Nicolas Fella]

nicolasfella updated this revision to Diff 83346.
nicolasfella added a comment.


  - Use widgets font metrics

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D26448?vs=83338=83346

BRANCH
  recentfilemenu

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

AFFECTED FILES
  src/CMakeLists.txt
  src/krecentfilesmenu.cpp
  src/krecentfilesmenu.h

To: nicolasfella, #frameworks, dfaure
Cc: broulik, elvisangelaccio, cfeck, kde-frameworks-devel, LeGast00n, cblack, 
michaelh, ngraham, bruns

D26448: Add KRecentFilesMenu to replace KRecentFileAction

[David Faure]

dfaure accepted this revision.
dfaure added a comment.
This revision is now accepted and ready to land.


  A unittest would be useful too, especially if we then refactor the loading to 
use KIO jobs.
  
  But after 6 months, let's land this and keep working on it. Are you 
interested in writing the unittest and/or porting to KIO jobs, or do you need 
some help?

INLINE COMMENTS

> krecentfilesmenu.cpp:45
> +}
> +const QFontMetrics fontMetrics = QFontMetrics(QFont());
> +

Can be simplified to

  const QFontMetrics fontMetrics(QFont());

But this uses the application font, not this widget's font. The right thing to 
do would be to pass a QWidget* and use w->fontMetrics() here.

BRANCH
  recentfilemenu

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

To: nicolasfella, #frameworks, dfaure
Cc: broulik, elvisangelaccio, cfeck, kde-frameworks-devel, LeGast00n, cblack, 
michaelh, ngraham, bruns

D26448: Add KRecentFilesMenu to replace KRecentFileAction

[Nicolas Fella]

nicolasfella marked an inline comment as done.
nicolasfella added inline comments.

INLINE COMMENTS

> dfaure wrote in krecentfilesmenu.cpp:150
> Depending on QtConcurrent is just fine. However QtConcurrent::filtered is for 
> CPU-intensive operations, not for I/O operations. 1) you don't want to put 
> blocking runnables in the global thread pool [can be solved by assigning to a 
> different threadpool], 2) I don't think you really want to parallelize 16 
> calls to QFile::exists, for the case of a local physical harddisk? Not sure 
> it would really harm (we're not reading 16 files, at least), but for sure the 
> overhead of dispatching runnables to 16 threads would make it slower... One 
> solution here might be just a dedicated thread iterating over the list.
> 
> However: note that the usual KIO way to do file operations async isn't 
> multithreading, it's rather KIO jobs.
> This would move the "5 minutes waiting for an NFS server" horror case into a 
> kioslave rather than a thread, both achieve the desired outcome which is to 
> not block the GUI thread.
> 
> Keeping the order of the entries is going to be interesting. I guess we need 
> a temporary data structure which stores "exists | does not exist" for each 
> entry, and once all jobs are done, we go through that and fill the menu. Note 
> that remote URLs should just be assumed to exist, we don't want to actually 
> check (slow; might prompt for password; might error on different networks; 
> etc.).
> 
> Unlike Kai-Uwe, I think this should be a separate merge request though, it's 
> all quite orthogonal to your work. As long as you confirm that filling the 
> menu "later" has no negative impact on the API (i.e. the user of the class 
> cannot assume that the menu is filled in immediately).

> As long as you confirm that filling the menu "later" has no negative impact 
> on the API (i.e. the user of the class cannot assume that the menu is filled 
> in immediately).

Should be fine

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

To: nicolasfella, #frameworks, dfaure
Cc: broulik, elvisangelaccio, cfeck, kde-frameworks-devel, LeGast00n, cblack, 
michaelh, ngraham, bruns

D26448: Add KRecentFilesMenu to replace KRecentFileAction

[Nicolas Fella]

nicolasfella updated this revision to Diff 83338.
nicolasfella marked an inline comment as done.
nicolasfella added a comment.


  - Fix @since
  - Add underscore to filename
  - Reserve vector

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D26448?vs=83337=83338

BRANCH
  recentfilemenu

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

AFFECTED FILES
  src/CMakeLists.txt
  src/krecentfilesmenu.cpp
  src/krecentfilesmenu.h

To: nicolasfella, #frameworks, dfaure
Cc: broulik, elvisangelaccio, cfeck, kde-frameworks-devel, LeGast00n, cblack, 
michaelh, ngraham, bruns

D26448: Add KRecentFilesMenu to replace KRecentFileAction

[Nicolas Fella]

nicolasfella marked 6 inline comments as done.

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

To: nicolasfella, #frameworks, dfaure
Cc: broulik, elvisangelaccio, cfeck, kde-frameworks-devel, LeGast00n, cblack, 
michaelh, ngraham, bruns

D26448: Add KRecentFilesMenu to replace KRecentFileAction

[Nicolas Fella]

nicolasfella updated this revision to Diff 83337.
nicolasfella marked an inline comment as done.
nicolasfella added a comment.


  - Make findEntry return non-const iterator
  - Use std::vector
  - Truncate entries when setting maximum

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D26448?vs=73172=83337

BRANCH
  recentfilemenu

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

AFFECTED FILES
  src/CMakeLists.txt
  src/krecentfilesmenu.cpp
  src/krecentfilesmenu.h

To: nicolasfella, #frameworks, dfaure
Cc: broulik, elvisangelaccio, cfeck, kde-frameworks-devel, LeGast00n, cblack, 
michaelh, ngraham, bruns

D26448: Add KRecentFilesMenu to replace KRecentFileAction

[David Faure]

dfaure added inline comments.

INLINE COMMENTS

> nicolasfella wrote in krecentfilesmenu.cpp:150
> I'm wondering how to best make this async without selling my soul to the 
> mulitthreading devil. QtConcurrent::filtered looks interesting, but depending 
> on QtConcurrent wouldn't be feasible, would it?

Depending on QtConcurrent is just fine. However QtConcurrent::filtered is for 
CPU-intensive operations, not for I/O operations. 1) you don't want to put 
blocking runnables in the global thread pool [can be solved by assigning to a 
different threadpool], 2) I don't think you really want to parallelize 16 calls 
to QFile::exists, for the case of a local physical harddisk? Not sure it would 
really harm (we're not reading 16 files, at least), but for sure the overhead 
of dispatching runnables to 16 threads would make it slower... One solution 
here might be just a dedicated thread iterating over the list.

However: note that the usual KIO way to do file operations async isn't 
multithreading, it's rather KIO jobs.
This would move the "5 minutes waiting for an NFS server" horror case into a 
kioslave rather than a thread, both achieve the desired outcome which is to not 
block the GUI thread.

Keeping the order of the entries is going to be interesting. I guess we need a 
temporary data structure which stores "exists | does not exist" for each entry, 
and once all jobs are done, we go through that and fill the menu. Note that 
remote URLs should just be assumed to exist, we don't want to actually check 
(slow; might prompt for password; might error on different networks; etc.).

Unlike Kai-Uwe, I think this should be a separate merge request though, it's 
all quite orthogonal to your work. As long as you confirm that filling the menu 
"later" has no negative impact on the API (i.e. the user of the class cannot 
assume that the menu is filled in immediately).

REPOSITORY
  R236 KWidgetsAddons

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

To: nicolasfella, #frameworks, dfaure
Cc: broulik, elvisangelaccio, cfeck, kde-frameworks-devel, LeGast00n, cblack, 
michaelh, ngraham, bruns

D26448: Add KRecentFilesMenu to replace KRecentFileAction

[Nicolas Fella]

nicolasfella added inline comments.

INLINE COMMENTS

> 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.

I'm wondering how to best make this async without selling my soul to the 
mulitthreading devil. QtConcurrent::filtered looks interesting, but depending 
on QtConcurrent wouldn't be feasible, would it?

REPOSITORY
  R236 KWidgetsAddons

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

To: nicolasfella, #frameworks, dfaure
Cc: broulik, elvisangelaccio, cfeck, kde-frameworks-devel, LeGast00n, cblack, 
michaelh, ngraham, bruns

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

D26448: Add KRecentFilesMenu to replace KRecentFileAction

[Nicolas Fella]

nicolasfella marked an inline comment as not done.

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

D26448: Add KRecentFilesMenu to replace KRecentFileAction

[Nicolas Fella]

nicolasfella updated this revision to Diff 73172.
nicolasfella marked 6 inline comments as done.
nicolasfella added a comment.


  - More comments

REPOSITORY
  R236 KWidgetsAddons

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D26448?vs=73170=73172

BRANCH
  recentfilemenu

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

AFFECTED FILES
  src/CMakeLists.txt
  src/krecentfilesmenu.cpp
  src/krecentfilesmenu.h

To: nicolasfella, #frameworks, dfaure
Cc: broulik, elvisangelaccio, cfeck, kde-frameworks-devel, LeGast00n, GB_2, 
michaelh, ngraham, bruns

D26448: Add KRecentFilesMenu to replace KRecentFileAction

[Nicolas Fella]

nicolasfella updated this revision to Diff 73170.
nicolasfella marked 4 inline comments as done.
nicolasfella added a comment.


  - Address comments

REPOSITORY
  R236 KWidgetsAddons

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D26448?vs=72928=73170

BRANCH
  recentfilemenu

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

AFFECTED FILES
  src/CMakeLists.txt
  src/krecentfilesmenu.cpp
  src/krecentfilesmenu.h

To: nicolasfella, #frameworks, dfaure
Cc: broulik, elvisangelaccio, cfeck, kde-frameworks-devel, LeGast00n, GB_2, 
michaelh, ngraham, bruns

D26448: Add KRecentFilesMenu to replace KRecentFileAction

[Nicolas Fella]

nicolasfella added inline comments.

INLINE COMMENTS

> dfaure wrote in krecentfilesmenu.cpp:85
> Why not std::vector?
> 
> std::list is a linked list, so this smells like pointers to nodes containing 
> pointers, lots of indirection.

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

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

D26448: Add KRecentFilesMenu to replace KRecentFileAction

[Kai Uwe Broulik]

broulik added inline comments.

INLINE COMMENTS

> krecentfilesmenu.cpp:150
> +// Don't restore if file doesn't exist anymore
> +if (url.isLocalFile() && !QFile::exists(url.toLocalFile())) {
> +continue;

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.

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

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 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::const_iterator findEntry(const QUrl );
> +};

 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 
> +

not used in this header

> krecentfilesmenu.h:39
> +public:
> +KRecentFilesMenu(const QString , QWidget *parent = nullptr);
> +KRecentFilesMenu(QWidget *parent = nullptr);

explicit

> krecentfilesmenu.h:40
> +KRecentFilesMenu(const QString , 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

D26448: Add KRecentFilesMenu to replace KRecentFileAction

[Nicolas Fella]

nicolasfella added inline comments.

INLINE COMMENTS

> cfeck wrote in krecentfilesmenu.h:104
> Weren't there ABI issues with std::list?
> 
> Also, missing reference on url.

We use std::list in other frameworks and IIRC no one complained

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

D26448: Add KRecentFilesMenu to replace KRecentFileAction

[Nicolas Fella]

nicolasfella marked 8 inline comments as done.

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

D26448: Add KRecentFilesMenu to replace KRecentFileAction

[Nicolas Fella]

nicolasfella marked 8 inline comments as not done.
nicolasfella added inline comments.

INLINE COMMENTS

> elvisangelaccio wrote in krecentfilesmenu.cpp:31
> Why not `int` since it's what we expose in the API anyway?

I'm getting a warning about signed/unsigned compare otherwise

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

D26448: Add KRecentFilesMenu to replace KRecentFileAction

[Nicolas Fella]

nicolasfella updated this revision to Diff 72928.
nicolasfella marked 8 inline comments as done.
nicolasfella added a comment.


  - Rework internals to do fewer allocations and file IO

REPOSITORY
  R236 KWidgetsAddons

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D26448?vs=72844=72928

BRANCH
  recentfilemenu

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

AFFECTED FILES
  src/CMakeLists.txt
  src/krecentfilesmenu.cpp
  src/krecentfilesmenu.h

To: nicolasfella, #frameworks, dfaure
Cc: elvisangelaccio, cfeck, kde-frameworks-devel, LeGast00n, GB_2, michaelh, 
ngraham, bruns

D26448: Add KRecentFilesMenu to replace KRecentFileAction

[Elvis Angelaccio]

elvisangelaccio added inline comments.

INLINE COMMENTS

> krecentfilesmenu.cpp:31
> +QSettings *m_settings;
> +size_t m_maximumItems = 10;
> +};

Why not `int` since it's what we expose in the API anyway?

> krecentfilesmenu.cpp:53
> +setIcon(QIcon::fromTheme(QStringLiteral("document-open-recent")));
> +const QString fileName = 
> QStandardPaths::writableLocation(QStandardPaths::AppDataLocation) + 
> QLatin1String("/") + QCoreApplication::applicationName() + 
> QLatin1String("staterc");
> +d->m_settings = new QSettings(fileName, QSettings::Format::IniFormat, 
> this);

`QStringLiteral("...").arg()` would be more readable imho.

> krecentfilesmenu.h:32
> + *
> + * Replaces KRecentFilesAction from KConfigWidgets.
> + *

This is a porting guide, not sure it belongs in the main apidox of the class. 
(users of this new API may not know/care about KRecentFilesAction).

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

D26448: Add KRecentFilesMenu to replace KRecentFileAction

[Christoph Feck]

cfeck added a comment.


  Nice :)

INLINE COMMENTS

> krecentfilesmenu.cpp:34
> +
> +KRecentFilesMenu::KRecentFilesMenu(const QString& title, QWidget* parent)
> +: QMenu(title, parent)

Here you switch from `Type *var` to `Type* var`

> krecentfilesmenu.cpp:41
> +
> +KRecentFilesMenu::KRecentFilesMenu(QWidget* parent)
> +: QMenu(tr("Recent Files"), parent)

same

> krecentfilesmenu.cpp:81
> +
> +void KRecentFilesMenu::addUrl(const QUrl url, const QString& name)
> +{

same

> krecentfilesmenu.cpp:103
> +
> +void KRecentFilesMenu::removeUrl(const QUrl& url)
> +{

same

> krecentfilesmenu.cpp:215
> +{
> +d->m_maximumItems = maximumItems;
> +}

Should we truncate the current list if the new max items is smaller?

> krecentfilesmenu.h:67
> + */
> +void addUrl(const QUrl url, const QString  = QString());
> +

Missing reference

> krecentfilesmenu.h:79
> + *
> + * When the manimum url count is reached and a new URL is added the
> + * oldest will be replaced.

manimum url → maximum URL

> krecentfilesmenu.h:104
> +void rebuildMenu();
> +std::list>::const_iterator findUrl(const QUrl url);
> +

Weren't there ABI issues with std::list?

Also, missing reference on url.

REPOSITORY
  R236 KWidgetsAddons

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

To: nicolasfella, #frameworks, dfaure
Cc: cfeck, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D26448: Add KRecentFilesMenu to replace KRecentFileAction

[Nicolas Fella]

nicolasfella updated this revision to Diff 72844.
nicolasfella added a comment.


  - Docs

REPOSITORY
  R236 KWidgetsAddons

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D26448?vs=72842=72844

BRANCH
  recentfilemenu

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

AFFECTED FILES
  src/CMakeLists.txt
  src/krecentfilesmenu.cpp
  src/krecentfilesmenu.h

To: nicolasfella, #frameworks, dfaure
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D26448: Add KRecentFilesMenu to replace KRecentFileAction

[Nicolas Fella]

nicolasfella edited the test plan for this revision.

REPOSITORY
  R236 KWidgetsAddons

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

To: nicolasfella, #frameworks, dfaure
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D26448: Add KRecentFilesMenu to replace KRecentFileAction

[Nicolas Fella]

nicolasfella created this revision.
nicolasfella added reviewers: Frameworks, dfaure.
Herald added a project: Frameworks.
Herald added a subscriber: kde-frameworks-devel.
nicolasfella requested review of this revision.

REVISION SUMMARY
  The way KRecentFilesAction is used has a number of problems. It lives in a 
relatively high tier and is used e.g. by KStandardAction, forcing it to the 
same high tier. Furthermore the de-facto usage has the problem of mixing 
configuration and state information (which is not a problem of the API itself). 
The API is quite flexible, but most of that flexibility isn't acutally used by 
applications.
  
  KRecentFilesMenu is different in the following ways:
  
  - It lives in KWidgetAddons and thus in Tier 1. With Qt6 and 
QGuiAction/QGuiMenu it could even go to KGuiAddons
  - In order to do this it uses QSettings instead of KConfig which should be 
fine since we don't need stuff like cascading and immutability
  - Instead of using KSelectAction it uses/is a QMenu
  - It forces to store the data in XDG_DATA_DIR instead of XDG_CONFIG_DIR like 
it's done in practice. Going through the users of KRecentFilesAction I did not 
find any use case for allowing other file paths. Apps can still save multiple 
sets of recent docs by leveraging groups.
  
  See T12246  for context.

TEST PLAN
  Tested with a patch for Ark

REPOSITORY
  R236 KWidgetsAddons

BRANCH
  recentfilemenu

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

AFFECTED FILES
  src/CMakeLists.txt
  src/krecentfilesmenu.cpp
  src/krecentfilesmenu.h

To: nicolasfella, #frameworks, dfaure
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns