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
