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
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
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
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
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
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
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
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
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
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
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,
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.
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 =
>
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
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
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
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
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
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 =
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
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
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
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
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
> +
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
> +
>
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
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
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
28 matches
Mail list logo
