| ngraham | |
| dfaure |
| Frameworks |
If KFileWidget's filter list has two matching filters for an extension,
it will always select the first filter, even if the current filter
already matches the file name.
This is fine, if you auto-select the filter to match the file name, but
breaks, if you want to auto-change the file name's extension via the
selected filter.
So this checks, if the current filter already matches the file name
before trying to find a matching filter and select it.
BUG: 407642
OBSERVED RESULT
Filter is "Word 2003 XML (.xml)"
EXPECTED RESULT
Filter is "DocBook (.xml)"
| Automatic diff as part of commit; lint not applicable. |
| Automatic diff as part of commit; unit tests not applicable. |
This patch was developed on KIO v5.44 (git tag) in an Ubuntu 18.04 chroot (because that's my LibreOffice development environment) and rebased on master.
The test-program was run on Debian Buster via LD_PRELOAD=./git_kio/build/bin/libKF5KIOFileWidgets.so, which has otherwise KIO v5.54.
I tried to build master on Ubuntu 18.04, but that failed. The git log for kfilewidget.cpp looked unsuspicious enough.
There were some conflicts in the rebase, which I manually fixed, but as always, this might result in typos, as I couldn't even compile master.
Sorry, but this doesn't compile:
/home/dev/kde/src/kio/src/filewidgets/kfilewidget.cpp: In member function ‘bool KFileWidgetPrivate::matchFilter(const QString&, const QString&, bool)’:
/home/dev/kde/src/kio/src/filewidgets/kfilewidget.cpp:2459:25: error: ‘p’ declared as reference but not initialized
for (const QString &p, patterns) {
^
/home/dev/kde/src/kio/src/filewidgets/kfilewidget.cpp:2459:36: error: expected ‘;’ before ‘)’ token
for (const QString &p, patterns) {
^
/home/dev/kde/src/kio/src/filewidgets/kfilewidget.cpp:2459:36: error: expected primary-expression before ‘)’ token
/home/dev/kde/src/kio/src/filewidgets/kfilewidget.cpp:2459:36: error: expected ‘;’ before ‘)’ tokenNeeds to be for (const QString &p : patterns) {
Hope this compiles now. Since I can't test the rebased patch, I hope that is the last update.
The original working version is based on v5.44. See my previous comment.
| src/filewidgets/kfilewidget.cpp | ||
|---|---|---|
| 132 | Nitpick: I'd make filter the first parameter. And we usually don't use const boll in signatures, but just bool. bUpdate doesn't tell the reader what this variable is used for. How about calling it updateCurrentFilter or similar? | |
| src/filewidgets/kfilewidget.cpp | ||
|---|---|---|
| 2463 | Is the bUpdate bool necessary? Without it, we'd call setCurrentFilter(currentFilter()) which should be a no-op, right? Alternatively, the method could return a QString, and the (second) caller could call setCurrentFilter. I just don't like a method that's sometimes a getter and sometimes a setter (basically). | |
| 2495 | The added '|' isn't needed, is it? str.left(-1) returns the whole string. | |
Changes:
Remarks:
| src/filewidgets/kfilewidget.cpp | ||
|---|---|---|
| 132 | Kind of Done. | |
| 2463 |
void KFileFilterCombo::setCurrentFilter(const QString &filter)
{
setCurrentIndex(d->m_filters.indexOf(filter));
emit filterChanged();
}filterChanged will unconditionally start the "cycle", which will set the wrong filter.
Also had that. It "felt" strange, but I don't care much. | |
Yes, naming is hard because the method is dual-purposed ;-)
If the method only tried to match (but not to set), then the naming would be much simpler.
QString findMatchingFilter(....) const;
if (!findMatchingFilter(...).isEmpty())
return;
foreach(....) {
const QString filter = findMatchingFilter(....);
if (!filter.isEmpty()) {
setCurrentFilter(filter);
return;
}
}It "felt" strange, but I don't care much.
I do care, because others will try to understand and possibly modify this code later, so it should not "feel strange".
| src/filewidgets/kfilewidget.cpp | ||
|---|---|---|
| 2495 | You wrote "done", but it's still there. | |
For a manual test I added a "Raw (*)" filter to the bug test program.
After playing with it, I decided to not test the currentFilter() against QLatin1String("*"), so if a user selects such an entry, it'll disable auto-filter and -extension selection.
Since we never auto-select the "*" filter, this honors the users filter selection.
Took me a while to write the unit test without QDialog. I was a bit tricked by Q_ASSERT used in other tests ;-)
Done
…
Done
It "felt" strange, but I don't care much.
I do care, because others will try to understand and possibly modify this code later, so it should not "feel strange".
I should have written. "Fine with me.". No offense intended.
| src/filewidgets/kfilewidget.cpp | ||
|---|---|---|
| 2495 | Should have been - will do. | |
Thanks!
One minor issue: unittests (those based on QTEST_MAIN like yours) go into the autotests directory.
tests/ is for interactive test programs.
Can you move it there?
It could even be just a new method in the existing autotests/kfilewidgettest.cpp
[if you do keep it separate for some reason, add a copyright header, but my recommendation is to merge it anyway, to keep the usual structure of one test file per class being tested]
Technically the '*' filter just makes sense as the last filter in the list and it's auto-selected if it's the first filter entry.
That's out of scope here. Not sure if there should / could be a warning for a developer.
Thanks for your patience.
Almost there. Thanks for *your* patience :-)
| autotests/kfilewidgettest.cpp | ||
|---|---|---|
| 88 | ... use setUrls *to* auto-select ODT filter? ("to" is missing) | |
| src/filewidgets/kfilewidget.cpp | ||
| 2456 | This method could (and should) be marked as const, now. | |
| 2492 | I don't really understand what this comment is doing here, it seems unrelated to the next line of code. Is it just a longer version of the comment on line 2499? | |
| autotests/kfilewidgettest.cpp | ||
|---|---|---|
| 88 | No. The first line is missing a point or semicolon, if you read the comment like a sentence. I had two independent comments in mind. Or some kind of annotation (*) to make it look more like a list. The first line is a general comment, the 2nd describes the actual test case.. I know the blocking is documented, but it's still not expected behavior. At least I was puzzled and even read the code before the API doc, as I suspected a bug somewhere (that didn't exclude my code). | |
| 94 | From your POV this probably has the same problem then the first comment. I'll change it to: // select 2nd duplicate XML filter (see bug 407642) | |
| src/filewidgets/kfilewidget.cpp | ||
| 2456 | Will do. | |
| 2492 | Kind of a reverse comment of line 2499. My original code tested for '*' and I found it none-obvious to omit that match test here. I even tested both variants with the bug program to be sure. The comment is a little disturbing, as there wasn't any code handling '*' yet. I'll change it to: // accept any match to honor the user's selection; see later code handling the "*" match Or I drop it. I would keep it. Other suggestions? | |
Actually looks like we've got compilation errors now:
/home/dev/kde/src/kio/src/filewidgets/kfilewidget.cpp:2456:9: error: prototype for ‘QString KFileWidgetPrivate::findMatchingFilter(const QString&, const QString&) const’ does not match any in class ‘KFileWidgetPrivate’
QString KFileWidgetPrivate::findMatchingFilter(const QString &filter, const QString &filename) const
^~~~~~~~~~~~~~~~~~
/home/dev/kde/src/kio/src/filewidgets/kfilewidget.cpp:132:10: error: candidate is: bool KFileWidgetPrivate::findMatchingFilter(const QString&, const QString&) const
bool findMatchingFilter(const QString &filter, const QString &filename) const;
^~~~~~~~~~~~~~~~~~
/home/dev/kde/src/kio/src/filewidgets/kfilewidget.cpp: In member function ‘void KFileWidgetPrivate::updateFilter()’:
/home/dev/kde/src/kio/src/filewidgets/kfilewidget.cpp:2493:78: error: request for member ‘isEmpty’ in ‘((KFileWidgetPrivate*)this)->KFileWidgetPrivate::findMatchingFilter(KFileFilterCombo::currentFilter() const(), filename)’, which is of non-class type ‘bool’
if (!findMatchingFilter(filterWidget->currentFilter(), filename).isEmpty()) {
^~~~~~~
/home/dev/kde/src/kio/src/filewidgets/kfilewidget.cpp:2497:51: error: conversion from ‘bool’ to non-scalar type ‘QString’ requested
QString match = findMatchingFilter(filter, filename);
~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~
src/filewidgets/CMakeFiles/KF5KIOFileWidgets.dir/build.make:413: recipe for target 'src/filewidgets/CMakeFiles/KF5KIOFileWidgets.dir/kfilewidget.cpp.o' failedNot sure how that exactly slipped in :-(