Implement support for selected mime type filters
ClosedPublic

Authored by elvisangelaccio on Apr 14 2017, 8:17 AM.

Details

Summary

QFileDialog in Qt 5.9 has better support for mimetype filters and
introduces the QFileDialog::selectedMimeTypeFilter() method [1].

This change implements the required functions on the Plasma side. We
cannot depend on Qt 5.9 yet, so the unit test is compiled only when
building against Qt 5.9.
The manual test is also expanded.

[1]: http://code.qt.io/cgit/qt/qtbase.git/commit/?h=5.9&id=34f82b8abcb279542b6350e70609c549e39caafb

Test Plan

New unit tests pass when building against Qt >= 5.9.
Manual test also works as expected.

Diff Detail

Repository
R135 Integration for Qt applications in Plasma
Branch
master
Lint
No Linters Available
Unit
No Unit Test Coverage
Restricted Application added a project: Plasma. · View Herald TranscriptApr 14 2017, 8:17 AM
Restricted Application added a subscriber: plasma-devel. · View Herald Transcript
dfaure requested changes to this revision.Apr 15 2017, 8:00 AM
dfaure added inline comments.
autotests/kfiledialog_unittest.cpp
83

Why use QMimeDatabase, just to then use name()? You could just put "text/x-chdr" in the data row, no?

115

Surely you mean QCOMPARE(dialog.selectedMimeTypeFilter(), QString())

src/platformtheme/kdeplatformfiledialoghelper.cpp
182

Are you sure this ASSERT is wanted? See next comment.

tests/qfiledialogtest.cpp
116

this code looks ok to me, but it will hit the above assert when the -mimeTypeFilter option is absent, no?

This revision now requires changes to proceed.Apr 15 2017, 8:00 AM
autotests/kfiledialog_unittest.cpp
83

Leftover from the previous patch, yes doesn't make sense now.

115

No, selectedMimeTypeFilter() is only in 5.9 so that wouldn't compile

tests/qfiledialogtest.cpp
116

Good catch, I'll drop the assert then.

tests/qfiledialogtest.cpp
116

The problem is that if isMimeFilter() is false we get a name filter instead of a mimetype, which is not expected.
This could be fixed by something like:

if (m_fileWidget->filterWidget()->isMimeFilter()) {
    return m_fileWidget->filterWidget()->currentFilter();
} else {
    // detect mimetype of selectedFiles() using QMimeDatabase
    ...
}

Does it make sense?

dfaure added inline comments.Apr 15 2017, 9:06 AM
autotests/kfiledialog_unittest.cpp
115

Oh I see. The QCOMPARE is just so that a failure happens. Pretty pointless, it's not testing anything from Qt. I would just remove the whole #else block.

The point of QEXPECT_FAIL is to either mark a TODO (but you fixed it already) or detect unexpected passes, which can't happen here ;-)

elvisangelaccio edited edge metadata.
  • Removed pointless QMimeDatabase usage in the test
  • Only run unit test when building against 5.9
elvisangelaccio edited the summary of this revision. (Show Details)Apr 15 2017, 9:55 AM
elvisangelaccio marked 3 inline comments as done.
anthonyfieroni added inline comments.
src/platformtheme/kdeplatformfiledialoghelper.h
64–67 ↗(On Diff #13457)

Better overrites them, then ifdef in implementation

src/platformtheme/kdeplatformfiledialoghelper.h
64–67 ↗(On Diff #13457)

Not possible, only QPlatformFileDialogHelper >= 5.9 has those methods.

Properly implement selectedMimeTypeFilter(). KFileFilterCombo::currentFilter() might return
invalid mimetypes, so fall back on QMimeDatabase() if necessary.

dfaure accepted this revision.Apr 30 2017, 9:57 AM
This revision is now accepted and ready to land.Apr 30 2017, 9:57 AM
This revision was automatically updated to reflect the committed changes.