[Differential] [Commented On] D1813: Fix selected name filter with multiple mimetypes

[dfaure (David Faure)]

dfaure added inline comments.

INLINE COMMENTS

> kdeplatformfiledialoghelper.cpp:78
>  /*
>   * Map a KDE filter string into a Qt one.
>   */

Can you explain and document here what this function does, i.e. input args and return value? It's a bit confusing.

"kde" is a mimetype name, e.g. application/zip right?
"list" is a list of .... mimetype names?

I'm confused because the calling code is

  return kde2QtFilter(options()->nameFilters(), m_dialog->selectedNameFilter());

which looks like "Plain Text (*.txt)" type filter to me, but I guess it can also be "Plain Text (text/plain)" ? Or is that wrong ?
It's strange because in the QFileDialog docu, name filters and mimetype filters are two different things...

Documenting examples here would be useful.

I see that the patch adds support for the (*.txt) case. Then the commit log is incomplete, because this isn't just about multiple mimetypes, but also about (non-mimetype-based) name filters, right?
The commit log also says "for some reason" which sounds like the problem isn't fully understood, and says "when we're about to return empty" which sounds too-last-minute and doesn't match this version of the patch.
Please revise the commit log and document the inputs/outputs of this method, then look at adding a unittest ;)
Thanks!

> kdeplatformfiledialoghelper.cpp:86
> +        const int pos = filter.indexOf(kde);
> +        if (pos > 0 && filter.length() >= kde.length() + pos) {
> +            const QChar prev = filter[pos - 1];

(pre-existing issue) the ">=" should actually be ">"      [and I would swap it around and make it < for readability]
otherwise when pos + kde.length() == filter.length() then the "succ" line below goes out of bounds.

REPOSITORY
  rPLASMAINTEGRATION Integration for Qt applications in Plasma

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

EMAIL PREFERENCES
  https://phabricator.kde.org/settings/panel/emailpreferences/

To: elvisangelaccio, #plasma
Cc: dfaure, graesslin, mart, plasma-devel, #plasma, jensreuterberg, abetts, sebas
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/plasma-devel/attachments/20160718/b6dab3cd/attachment-0001.html>