D21249: Test current filter before setting a new one

[Jan-Marek Glogowski]

jglogowski added inline comments.

INLINE COMMENTS

> dfaure wrote in kfilewidgettest.cpp:88
> ... use setUrls *to* auto-select ODT filter?
> 
> ("to" is missing)

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

> kfilewidgettest.cpp:94
> +        // select 2nd duplicate XML filter
> +        // see bug 407642
> +        fw.filterWidget()->setCurrentFilter("*.xml *.b|DocBook (.xml)");

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)

> dfaure wrote in kfilewidget.cpp:2456
> This method could (and should) be marked as const, now.

Will do.

> dfaure wrote in kfilewidget.cpp: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?

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?

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D21249

To: jglogowski, #frameworks, ngraham, dfaure
Cc: elvisangelaccio, ngraham, michaelweghorn, kde-frameworks-devel, michaelh, bruns
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-frameworks-devel/attachments/20190521/5ed8b5d8/attachment.html>