Differential D2365

Add failing test case for selected filter from mimetype
AbandonedPublic
Actions

Authored by elvisangelaccio on Aug 7 2016, 12:01 PM.

Details

Reviewers
dfaure
graesslin
Summary

As discussed in D1813, I'm trying to understand how exactly the filters are handled and where the actual problem is. For now I've come up with this failing test case, which looks like this:

FAIL!  : KFileDialog_UnitTest::testSelectedNameFilterFromMime(single mime filter (C header file)) Compared values are not the same
   Actual   (dialog.selectedNameFilter()): ""
   Expected (expectedSelectedNameFilter) : "C header (*.h)"

QFileDialog::selectMimeTypeFilter() calls KDEPlatformFileDialogHelper::selectNameFilter() which in turn calls qt2KdeFilter(), so it seems that my patch in D1813 was at the wrong place/level, as it was changing kde2QtFilter()...

Diff Detail

Repository
R135 Integration for Qt applications in Plasma
Lint
Lint Skipped
Unit
Unit Tests Skipped
elvisangelaccio updated this revision to Diff 5711.Aug 7 2016, 12:01 PM
elvisangelaccio retitled this revision from to Add failing test case for selected filter from mimetype.
elvisangelaccio updated this object.
elvisangelaccio edited the test plan for this revision. (Show Details)
elvisangelaccio added reviewers: dfaure, graesslin.
elvisangelaccio set the repository for this revision to R135 Integration for Qt applications in Plasma.
elvisangelaccio added a project: Plasma.
Restricted Application added a subscriber: plasma-devel. · View Herald TranscriptAug 7 2016, 12:01 PM
elvisangelaccio added a comment.Aug 7 2016, 3:43 PM

I think I figured out what's going on. This seems to be a regression introduced by 25be75542. If I revert this commit, this test passes.

So, this is what's happening. In this example I'm considering the text/x-chdr test case:

  1. The test calls dialog.setMimeTypeFilters("text/x-chdr") and then dialog.show()
  2. KDEPlatformFileDialogHelper::show() calls initializeDialog()
  3. In initiliazeDialog() we end up appending text/x-chdr to the filters of m_fileWidget's KFileFilterCombo (d->m_filters in kio.git/src/filewidgets/kfilefiltercombo.cpp)
  4. In the test we call dialog.selectMimeTypeFilter("text/x-chdr")
  5. QFileDialog calls selectNameFilter("C header (*.h)")
  6. KDEPlatformFileDialogHelper calls selectNameFilter(qt2KdeFilter(QStringList("C header (*.h)")))
  7. KDEPlatformFileDialog calls filterWidget()->setCurrentFilter("*.h|C header ")
  8. The KFileFilterCombo ends up calling setCurrentIndex(d->m_filters.indexOf("*.h|C header ")) which fails because d->m_filters contains only text/x-chdr.
  9. dialog.selectedNameFilter() returns an empty string because of step 8.

So the bug seems to be in the step 3. Before of commit 25be75542, d->m_filters would also receive *.h|C header which would make the test pass.

But I'm still not sure how to properly fix this...

graesslin edited edge metadata.Aug 15 2016, 8:39 AM

As it's an expected failure I'm fine with it going in. I cannot really comment on the actual issue, because I have no clue about the file dialog.

dfaure edited edge metadata.Aug 15 2016, 10:16 AM

The thing is, mimetype filters should be preferred above name filters. So the bug is in QFileDialog::selectMimeTypeFilter which "falls back" to selectNameFilter. Instead it should call some selectInitiallySelectedMimeTypeFilter in the options, like selectNameFilter does with setInitiallySelectedNameFilter, and QPlatformFileDialogHelper is missing a selectMimeTypeFilter.

The fallback to selectNameFilter should only happen if d->usingWidgets() or d->selectMimeTypeFilter_sys() (to be added) fails.

Feel like making a patch for Qt ?

elvisangelaccio added a comment.Aug 15 2016, 4:03 PM
In D2365#45684, @dfaure wrote:

The thing is, mimetype filters should be preferred above name filters. So the bug is in QFileDialog::selectMimeTypeFilter which "falls back" to selectNameFilter. Instead it should call some selectInitiallySelectedMimeTypeFilter in the options, like selectNameFilter does with setInitiallySelectedNameFilter, and QPlatformFileDialogHelper is missing a selectMimeTypeFilter.

The fallback to selectNameFilter should only happen if d->usingWidgets() or d->selectMimeTypeFilter_sys() (to be added) fails.

Feel like making a patch for Qt ?

Sounds like something that I can try to do at Akademy :)
In the meantime, does this test still makes sense? Or maybe (if we are going to patch QFileDialog) would be better to add a new QFileDialog::selectedMimeTypeFilter()? (and use it instead of selectedNameFilter() in the last QCOMPARE)

dfaure added a comment.Aug 16 2016, 6:51 AM

Yes, and yes.

elvisangelaccio added a comment.Sep 6 2016, 3:47 PM
In D2365#45684, @dfaure wrote:

The thing is, mimetype filters should be preferred above name filters. So the bug is in QFileDialog::selectMimeTypeFilter which "falls back" to selectNameFilter. Instead it should call some selectInitiallySelectedMimeTypeFilter in the options, like selectNameFilter does with setInitiallySelectedNameFilter, and QPlatformFileDialogHelper is missing a selectMimeTypeFilter.

The fallback to selectNameFilter should only happen if d->usingWidgets() or d->selectMimeTypeFilter_sys() (to be added) fails.

Feel like making a patch for Qt ?

Patch up for review at https://codereview.qt-project.org/#/c/170332/

elvisangelaccio added a comment.Jan 6 2017, 9:28 PM

The patch has been merged: http://code.qt.io/cgit/qt/qtbase.git/commit/?h=dev&id=34f82b8abcb279542b6350e70609c549e39caafb

I'll get back to this review once Qt 5.9 is released...

elvisangelaccio abandoned this revision.Apr 14 2017, 8:19 AM

Superseded by D5446.

Revision Contents
Changeset List

PathPackages
Mautotests/kfiledialog_unittest.cpp (53 lines)

Diff 5711

View Options

autotests/kfiledialog_unittest.cpp

Loading...