Feel free to ship it.

You uploaded a wrong version of the patch. Please do a git rebase master and make sure you only have one commit on top of origin/master.

In D21312#675714, @Zren wrote:

I have created a Merge Request on GitLab with a a couple new commits.

https://invent.kde.org/system/dolphin/-/merge_requests/31

• It will only show on devices. It no longer show under mounted folders like Root in the user's favorite places.
• Adurol on Freenode pointed out that the KIO FilePicker now uses KIO::FileSystemFreeSpaceJob, so I've refactored the patch to use that API.

In D27710#627456, @elvisangelaccio wrote:

What about 1a98206873d77310a161e2ff7f39afe1060484a0 ? Is it not needed anymore?

@feverfew Does it look good now for you?

Superseded by https://invent.kde.org/system/dolphin/-/merge_requests/7

Superseded by https://invent.kde.org/system/dolphin/-/merge_requests/7

Please either copy the FindTaglib.cmake from ECM master or require KF5 >= 5.72 and don't copy FindTaglib.cmake here.

In D21695#675056, @elvisangelaccio wrote:

New attempt at https://invent.kde.org/frameworks/extra-cmake-modules/-/merge_requests/6

New attempt at https://invent.kde.org/frameworks/extra-cmake-modules/-/merge_requests/6

@ngraham Does the UI look good? Can we ship this?

Closing as discussed.

Sorry for the delay :(

In D29788#673711, @nikolaik wrote:

@elvisangelaccio Can I rename to buttonCancel/buttonOk instead of cancelButton/okButton? If variable name starts with UI element it's easier to use code completion.

Not sure if this should go to release/20.04, but I guess it shouldn't hurt.

Ah right, sorry :P

@meven Yes please. The patch may be small, but it's not trivial. Smaller atomic changes are always easier to review.

That's because the source is still compressed and we can't get a preview out of it.

Would it make sense to initialize devicePixelRatio to QGuiApplication::devicePixelRatio() and add an API to change it (if desired) ?

Very nice. Please push to release/20.04. Thanks!

In D29213#669068, @ngraham wrote:

Sounds to me like we should just hide the widget entirely when there's no size available, or a size calculation doesn't make sense given the context.

Use QVector instead of QQueue to avoid adding to the queue twice the same dir for subsequent scanning.

Add a priority queue for path that were not already present in cache.

Actually *\* currently works and with this patch won't work anymore, but IMHO this should be fixed in Qt.
QRegExp is deprecated anyway and will probably be removed in Qt 6. Let's get rid of it.

Yes, please push to 20.04.

@meven Can you please also fix these issues?

Wait, so the new feature does not work on Windows?

This also broke the Windows build on the CI:

Please fix the last two inline comments and then feel free to ship it.

LGTM now. Please improve the commit message and then feel free to ship it.

The feature looks reasonable to me.

In D29115#660130, @harogaston wrote:

In D29115#658465, @meven wrote:

About testing, you need to install the tool ctest then go to build dir and run the whole tests with ctest (it has many options to be more clever for instance ctest -R kfileitemmodeltest

Thank you for the info. I will try to add a test then.

In D29115#657937, @elvisangelaccio wrote:

In D29115#655601, @harogaston wrote:

Well for me it is pretty clear that this should be at least an option, if not the default. If you don't work with hidden files visible then this does nothing. But if you do, like myself, which view do you think is more useful/productive

Right, so let's make it the default without adding an option to get a worse behavior ;)

So do you all want me to remove all the clutter code regarding the new setting and just keep the logic as to make it the default and only behavior?
I would rather very much keep the extra option. But it is up to you I guess.

LGTM besides the inline comment.

In D26215#658092, @ahmadsamir wrote:

In D26215#657701, @elvisangelaccio wrote:

What about QRegularExpression::wildcardToRegularExpression() ? Could we use that? Note that the filter string comes from user input (i.e. from the filter bar).

I've just tested; it can be used, with the same caveat as the QRegExp docs have about WildcardUnix, a wildcard can't be escaped with a \:
*.txt would show all files with a .txt extension

• would show all files \* _won't_ show files with a literal "*" in their names
  
  Looks good enough to me, if you agree I'll update the diff.

Does PackageKit work on Windows/MacOS? I don't think so. We should probably run this code only on Unix, no?

In D29115#655601, @harogaston wrote:

Well for me it is pretty clear that this should be at least an option, if not the default. If you don't work with hidden files visible then this does nothing. But if you do, like myself, which view do you think is more useful/productive

Right, so let's make it the default without adding an option to get a worse behavior ;)

I think we should default to "number of items", because we can't know whether the user runs an SSD or an old HDD. Better safe than sorry.

What about "Size not available" ? I'm not sure I like the empty space there...

LGTM but I'd like input from @iasensio too.

In D27976#648403, @ngraham wrote:

Hmm, looks like someone else submitted a patch to do this: D28402. Perhaps we should proceed there instead?

Please add a #include <QCheckBox> in previewssettingspage.cpp, otherwise it doesn't build with Qt 5.15.

LGTM now. Ship it! :)