Differential D26448

Add KRecentFilesMenu to replace KRecentFileAction
ClosedPublic
Actions

Authored by nicolasfella on Jan 6 2020, 3:10 AM.

Details

Reviewers
dfaure
Group Reviewers
Frameworks
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 (D26449)

Diff Detail

Branch
recentfilemenu
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 27192
Build 27210: arc lint + arc unit
There are a very large number of changes, so older changes are hidden. Show Older Changes
Restricted Application added a project: Frameworks. · View Herald TranscriptJan 6 2020, 3:10 AM
Restricted Application added a subscriber: kde-frameworks-devel. · View Herald Transcript
nicolasfella requested review of this revision.Jan 6 2020, 3:10 AM
nicolasfella edited the test plan for this revision. (Show Details)Jan 6 2020, 3:11 AM
cfeck added a subscriber: cfeck.Jan 6 2020, 5:26 AM

Nice :)

src/krecentfilesmenu.cpp
35

Here you switch from Type *var to Type* var

42

same

82

same

104

same

216

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

src/krecentfilesmenu.h
68

Missing reference

80

manimum url → maximum URL

105

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

Also, missing reference on url.

elvisangelaccio added a subscriber: elvisangelaccio.Jan 6 2020, 1:00 PM
elvisangelaccio added inline comments.
src/krecentfilesmenu.cpp
32

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

54

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

src/krecentfilesmenu.h
33

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

nicolasfella updated this revision to Diff 72928.Jan 6 2020, 8:06 PM
nicolasfella marked 8 inline comments as done.
  • Rework internals to do fewer allocations and file IO
nicolasfella marked 8 inline comments as not done.Jan 6 2020, 8:06 PM
nicolasfella added inline comments.
src/krecentfilesmenu.cpp
32

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

nicolasfella marked 8 inline comments as done.Jan 6 2020, 8:07 PM
nicolasfella added inline comments.Jan 6 2020, 8:10 PM
src/krecentfilesmenu.h
105

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

dfaure requested changes to this revision.Jan 6 2020, 10:34 PM
dfaure added inline comments.
src/krecentfilesmenu.cpp
34

... const?

86

Why not std::vector?

std::list is a linked list, so this smells like pointers to nodes containing pointers, lots of indirection.

92

.... const;

103

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.

120

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.

143

This could be followed by d->m_entries.reserve(size); when using std::vector.

src/krecentfilesmenu.h
26

not used in this header

40

explicit

41

explicit

42

remove virtual, add override

69

I think it's "a URL"

81

mention the default value?

This revision now requires changes to proceed.Jan 6 2020, 10:34 PM
broulik added a subscriber: broulik.Jan 7 2020, 9:32 AM
broulik added inline comments.
src/krecentfilesmenu.cpp
151

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.

nicolasfella added inline comments.Jan 7 2020, 8:02 PM
src/krecentfilesmenu.cpp
86

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

nicolasfella updated this revision to Diff 73170.Jan 10 2020, 1:44 AM
nicolasfella marked 4 inline comments as done.
  • Address comments
nicolasfella updated this revision to Diff 73172.Jan 10 2020, 1:49 AM
nicolasfella marked 6 inline comments as done.
  • More comments
nicolasfella marked an inline comment as not done.Jan 10 2020, 1:49 AM
dfaure requested changes to this revision.Jan 11 2020, 9:04 AM
dfaure added inline comments.
src/krecentfilesmenu.cpp
86

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 :)

100

maybe %2_recentfiles to improve readability?

151

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 :)

190

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.

216

^ this comment hasn't been addressed yet.

This revision now requires changes to proceed.Jan 11 2020, 9:04 AM
nicolasfella added inline comments.Jul 28 2020, 4:06 PM
src/krecentfilesmenu.cpp
151

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?

dfaure added inline comments.Aug 2 2020, 10:20 AM
src/krecentfilesmenu.cpp
151

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

nicolasfella updated this revision to Diff 83337.Aug 3 2020, 10:04 PM
nicolasfella marked an inline comment as done.
  • Make findEntry return non-const iterator
  • Use std::vector
  • Truncate entries when setting maximum
nicolasfella marked 6 inline comments as done.Aug 3 2020, 10:05 PM
nicolasfella updated this revision to Diff 83338.Aug 3 2020, 10:10 PM
nicolasfella marked an inline comment as done.
  • Fix @since
  • Add underscore to filename
  • Reserve vector
nicolasfella marked an inline comment as done.Aug 3 2020, 10:11 PM
nicolasfella added inline comments.
src/krecentfilesmenu.cpp
151

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

dfaure accepted this revision.Aug 8 2020, 8:59 AM

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?

src/krecentfilesmenu.cpp
46

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.

This revision is now accepted and ready to land.Aug 8 2020, 8:59 AM
nicolasfella updated this revision to Diff 83346.Aug 11 2020, 8:35 PM
  • Use widgets font metrics
dfaure added inline comments.Aug 13 2020, 9:58 AM
src/krecentfilesmenu.cpp
75

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.

nicolasfella updated this revision to Diff 83348.Aug 13 2020, 12:14 PM
  • Use menus font metric
dfaure accepted this revision.Aug 13 2020, 9:17 PM

Thanks :-)

nicolasfella closed this revision.Aug 17 2020, 1:39 PM

Revision Contents
Changeset List

PathPackages
Msrc/CMakeLists.txt (2 lines)
AMsrc/krecentfilesmenu.h (106 lines)
AMsrc/krecentfilesmenu.cpp (268 lines)

Diff 83348

View Options

src/CMakeLists.txt

Loading...
View Options

src/krecentfilesmenu.h

Loading...
View Options

src/krecentfilesmenu.cpp

Loading...