D25572: [KFileWidget] Allow double quotes in filenames

[David Faure]

dfaure requested changes to this revision.
dfaure added a comment.
This revision now requires changes to proceed.


  I think this needs more work, we lost some tolerance here, AFAICS.

INLINE COMMENTS

> kfilewidget.cpp:1751
>  
> -    const int count = line.count(QLatin1Char('"'));
> -    if (count == 0) {   // no " " -> assume one single file
> +    if (!line.contains(QStringLiteral("\" \""))) {
>          if (!QDir::isAbsolutePath(line)) {

What if I had `"file1.txt" "file2.txt"` and I manually edit that to remove the second file, ending up with just `"file1.txt"`? The double-quotes will no longer be removed, because of this modified condition.

> kfilewidget.cpp:1767
>      while (true) {
> -        index1 = line.indexOf(QLatin1Char('"'), start);
> -        index2 = line.indexOf(QLatin1Char('"'), index1 + 1);
> +        index1 = line.indexOf(QStringLiteral("\""), index1);
> +        // find next word separator

Isn't there a `"` at this position, always?
Initially, at position 0, and then later, we know we hit `" "` so `index1 = index2+2` could be done at the end of the loop (instead of +1), and here we don't need an `indexOf` call anymore.
I assume this was done in order to be tolerant to things like two spaces or something, but now the separator is hardcoded to `" "` so we *know* there's a quote at `index2`+2 after finding a quote at `index2`.

Well, I guess we can keep an initial indexOf (outside the loop) in case there's a leading space (because a user manually removed the first file).

So this needs additional unittests for leading space, trailing space, single-quoted-file .... and I wonder if we are OK with no longer supporting two spaces (e.g. because the user manually removed some intermediate file in the list).

The separator is in fact `" +"` in regexp syntax, then?

> kfilewidget.cpp:1769
> +        // find next word separator
> +        index2 = line.indexOf(QStringLiteral("\" \""), index1 + 1);
>  

this could move to after the if() block on the next line.
If index1 < 0, there's no point in calculating index2.

> kfilewidget.cpp:1777
> +            // grab everything left until last character
> +            index2 = line.length() -1;
> +        }

Isn't the last character a double-quote (in the normal case)? Don't we want to stop just before it, to avoid grabbing it?

Of course this opens the question of what to do if the last character is NOT a double-quote, but
`"file1.txt" "file2.txt` looks weird. Well, getting `file2.tx` as a result would be even weirder, so better go backwards from the end of the string until "not space and not double-quote", for some tolerance. (another case for the unittest)

REPOSITORY
  R241 KIO

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

To: meven, #frameworks, ngraham, dfaure
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-frameworks-devel/attachments/20191201/7e255475/attachment-0001.html>